linux-kernel - Re: [PATCH 3/4] staging: dgnc: dgnc_neo: Fix conditional part of if statement

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: <20140517193927.GD15585@mwanda>
Date:	Sat, 17 May 2014 22:39:27 +0300
From:	Dan Carpenter <dan.carpenter@...cle.com>
To:	Masaru Nomura <massa.nomura@...il.com>
Cc:	lidza.louina@...il.com, markh@...pro.net,
	devel@...verdev.osuosl.org, gregkh@...uxfoundation.org,
	driverdev-devel@...uxdriverproject.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/4] staging: dgnc: dgnc_neo: Fix conditional part of if
 statement

On Sat, May 17, 2014 at 05:30:53PM +0100, Masaru Nomura wrote:
> diff --git a/drivers/staging/dgnc/dgnc_neo.c b/drivers/staging/dgnc/dgnc_neo.c
> index 8988346..c211f9f 100644
> --- a/drivers/staging/dgnc/dgnc_neo.c
> +++ b/drivers/staging/dgnc/dgnc_neo.c
> @@ -838,9 +838,11 @@ static void neo_param(struct tty_struct *tty)
>  	 * Have the UART interrupt on modem signal changes ONLY when
>  	 * we are in hardware flow control mode, or CLOCAL/FORCEDCD is not set.
>  	 */
> -	if ((ch->ch_digi.digi_flags & CTSPACE) || (ch->ch_digi.digi_flags & RTSPACE) ||
> -		(ch->ch_c_cflag & CRTSCTS) || !(ch->ch_digi.digi_flags & DIGI_FORCEDCD) ||
> -		!(ch->ch_c_cflag & CLOCAL)) {
> +	if ((ch->ch_digi.digi_flags & CTSPACE)
> +			|| (ch->ch_digi.digi_flags & RTSPACE)
> +			|| (ch->ch_c_cflag & CRTSCTS)
> +			|| !(ch->ch_digi.digi_flags & DIGI_FORCEDCD)
> +			|| !(ch->ch_c_cflag & CLOCAL)) {

This isn't the right way.  Write it like this:

	if ((ch->ch_digi.digi_flags & CTSPACE) ||
	    (ch->ch_digi.digi_flags & RTSPACE) ||
	    (ch->ch_c_cflag & CRTSCTS) ||
	    !(ch->ch_digi.digi_flags & DIGI_FORCEDCD) ||
	    !(ch->ch_c_cflag & CLOCAL)) {
		ier |= UART_IER_MSI;
	else
		ier &= ~UART_IER_MSI;

1) The "||" operation goes at the end of the line.
2) The conditions are all in a line.  The indenting is

[tab][space][space][space][space](ch->ch_digi.digi_f...

Also just fold this patch and [patch 2/4] together into one patch.  We
don't need two patches to fix one if statement.

The one thing per patch rule is a bit tricky.  It means that you have to
say which one thing you are fixing.  Don't say "I am fixing three
things."  Say "I am fixing one if statement".

regards,
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/