linux-kernel - Re: [PATCH 1/4 v2] silicom: checkpatch: assignments in if conditions

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: <20130617212320.GN5008@mwanda>
Date:	Tue, 18 Jun 2013 00:23:20 +0300
From:	Dan Carpenter <dan.carpenter@...cle.com>
To:	Lorenz Haspel <lorenz@...gers.com>
Cc:	devel@...uxdriverproject.org, gregkh@...uxfoundation.org,
	puff65537@...sheeslibrary.com, viro@...iv.linux.org.uk,
	michael.banken@...he.stud.uni-erlangen.de,
	devel@...verdev.osuosl.org, linux-kernel@...r.kernel.org,
	linux-kernel@...informatik.uni-erlangen.de
Subject: Re: [PATCH 1/4 v2] silicom: checkpatch: assignments in if conditions

On Mon, Jun 17, 2013 at 09:15:39PM +0200, Lorenz Haspel wrote:
> Fixes checkpatch error:
> There were assignments in if conditions, so I extracted them.
> 
> Signed-off-by: Lorenz Haspel <lorenz@...gers.com>
> Signed-off-by: Michael Banken <michael.banken@...he.stud.uni-erlangen.de>
> ---
> v2: removed some buggy extra lines and fixed white space issues

Gar....  This isn't right either.  Now it has *too many* blank
lines.  It's only between declarations and code that I was
complaining about.  You've added them between assignments and error
checks.


> @@ -1224,7 +1237,9 @@ static int wdt_pulse(bpctl_dev_t *pbpctl_dev)
>  		return -1;
>  #endif
>  	if (pbpctl_dev->bp_10g9) {
> -		if (!(pbpctl_dev_c = get_status_port_fn(pbpctl_dev)))
> +		pbpctl_dev_c = get_status_port_fn(pbpctl_dev);
> +

This blank line is harmful.

> +		if (!pbpctl_dev_c)
>  			return -1;
>  	}
>  
> @@ -1742,9 +1757,9 @@ static void write_data_port_int(bpctl_dev_t *pbpctl_dev,
>  
>  static int write_data_int(bpctl_dev_t *pbpctl_dev, unsigned char value)
>  {
> -	bpctl_dev_t *pbpctl_dev_b = NULL;
> +	bpctl_dev_t *pbpctl_dev_b = get_status_port_fn(pbpctl_dev);
>  

This blank line is required.

So what you have here is fine, but if you wanted you could re-write
this like:
{
	bpctl_dev_t *pbpctl_dev_b;

	pbpctl_dev_b = get_status_port_fn(pbpctl_dev);
	if (!pbpctl_dev_b)
		return -1;

Generally, you shouldn't put anything complicated in the initializer
statement.  People don't read that code as thouroughly and
initializers are sometimes a source of bugs.  But what you have here
is also perfectly acceptable.

> -	if (!(pbpctl_dev_b = get_status_port_fn(pbpctl_dev)))
> +	if (!pbpctl_dev_b)
>  		return -1;
>  	atomic_set(&pbpctl_dev->wdt_busy, 1);
>  	write_data_port_int(pbpctl_dev, value & 0x3);

rergards,
dan carpenter
--
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/