netdev - Re: [PATCH net-next v2 2/2] net: qed_dev: fix redundant check of rc and against -EINVAL

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] [day] [month] [year] [list]

Date:   Sat, 23 Oct 2021 03:26:51 -0600
From:   Jεan Sacren <sakiwit@...il.com>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     Ariel Elior <aelior@...vell.com>, GR-everest-linux-l2@...vell.com,
        davem@...emloft.net, netdev@...r.kernel.org
Subject: Re: [PATCH net-next v2 2/2] net: qed_dev: fix redundant check of rc
 and against -EINVAL

From: Jakub Kicinski <kuba@...nel.org>
Date: Fri, 22 Oct 2021 14:47:20 -0700
>
> On Thu, 21 Oct 2021 21:37:41 -0600 Jεan Sacren wrote:
> > From: Jean Sacren <sakiwit@...il.com>
> > 
> > We should first check rc alone and then check it against -EINVAL to
> > avoid repeating the same operation multiple times.
> > 
> > We should also remove the check of !rc in this expression since it is
> > always true:
> > 
> > 	(!rc && !resc_lock_params.b_granted)
> > 
> > Signed-off-by: Jean Sacren <sakiwit@...il.com>
> 
> The code seems to be written like this on purpose. You're adding
> indentation levels, and making the structure less readable IMO.
> 
> If you want to avoid checking rc / !rc multiple times you can just
> code it as:
> 
> 	if (rc == -EINVAL)
> 		...
> 	else if (rc)
> 		...
> 	else if (!granted)
> 		...
> 	else
> 		...
> 
> I'm not sure I see the point of the re-factoring.

Agreed.