linux-kernel - Re: [PATCH 2/4] staging: hv: Fix the code depending on struct netvsc_driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <alpine.LFD.2.00.1102232128100.2701@localhost6.localdomain6>
Date:	Wed, 23 Feb 2011 21:53:05 +0100 (CET)
From:	Thomas Gleixner <tglx@...utronix.de>
To:	Haiyang Zhang <haiyangz@...rosoft.com>
cc:	hjanssen@...rosoft.com, kys@...rosoft.com, v-abkane@...rosoft.com,
	gregkh@...e.de, linux-kernel@...r.kernel.org,
	devel@...uxdriverproject.org, virtualization@...ts.osdl.org
Subject: Re: [PATCH 2/4] staging: hv: Fix the code depending on struct
 netvsc_driver_context data order

On Wed, 23 Feb 2011, Haiyang Zhang wrote:
> @@ -137,8 +135,8 @@ static int netvsc_start_xmit(struct sk_buff *skb, struct net_device *net)
>  	struct net_device_context *net_device_ctx = netdev_priv(net);
>  	struct driver_context *driver_ctx =
>  	    driver_to_driver_context(net_device_ctx->device_ctx->device.driver);
> -	struct netvsc_driver_context *net_drv_ctx =
> -		(struct netvsc_driver_context *)driver_ctx;
> +	struct netvsc_driver_context *net_drv_ctx = container_of(driver_ctx,
> +		struct netvsc_driver_context, drv_ctx);

While the code is correct, it's still horrible to read.

  	struct net_device_context *dev_ctx = netdev_priv(net);
	struct netvsc_drv_ctx *net_drv_ctx;
  	struct netvsc_driver *net_drv_obj;
  	struct driver_context *drv_ctx;
  	struct hv_netvsc_packet *packet;
  	int ret;

	drv_ctx = driver_to_driver_context(dev_ctx->device_ctx->device.driver);
	net_drv_ctx = container_of(driver_ctx, struct netvsc_drv_ctx, drv_ctx);
	net_drv_obj = &net_drv_ctx->drv_obj;

Line breaking code just to fulfil the 80 chars requirement is a
horrible idea.

I just explained somewhere else, that these line breaks are equaly
inefficient to parse by the human eye as the extra wide lines. Have
you ever seen a newspaper which uses a column width which spawns the
whole size of the paper? Probably not, simply because nobody can read
it fluently and fast.

So just mechanically converting existing code which was written based
on that horrible though popular codingstyle which requires wide screen
monitors is not going to result in readable code.

You need to do more than just inserting random line breaks as you see fit.

	struct netvsc_driver_context *net_drv_ctx = container_of(driver_ctx,
		struct netvsc_driver_context, drv_ctx);

is a total horrible construct.

So
     	struct netvsc_driver_context *net_drv_ctx;

	net_drv_ctx = container_of(driver_ctx, struct netvsc_driver_context,
                                   drv_ctx);

is way better, as you can cleary see, that the extra line belongs to
the arguments of container_of(). Try to decode that in the above
original code w/o looking twice or more.

It's still not perfect but way better to read. So when converting (or
writing new) code think whether your struct names or variable names
need to be that long

     	struct netvsc_driver_context *net_drv_ctx;
vs.
	struct netsvc_drv_ctx *net_drv_ctx;

carries exact the same amount of information, but more condensed and lets

	net_drv_ctx = container_of(driver_ctx, struct netvsc_drv_ctx, drv_ctx);

fit into one line.

So yes, it's more work for you in the first place, but in the end it's
better readable code and you make it way easier for those who are
spending their (quality) time to review it.

Thanks,

	tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/