linux-kernel - Re: s390/net: Deletion of unnecessary checks before two function calls

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: <20141103171625.GU6890@mwanda>
Date:	Mon, 3 Nov 2014 20:16:25 +0300
From:	Dan Carpenter <dan.carpenter@...cle.com>
To:	SF Markus Elfring <elfring@...rs.sourceforge.net>
Cc:	Ursula Braun <ursula.braun@...ibm.com>,
	Martin Schwidefsky <schwidefsky@...ibm.com>,
	Heiko Carstens <heiko.carstens@...ibm.com>,
	Frank Blaschka <blaschka@...ux.vnet.ibm.com>,
	linux390@...ibm.com, linux-s390@...r.kernel.org,
	linux-kernel@...r.kernel.org, kernel-janitors@...r.kernel.org,
	trivial@...nel.org, Coccinelle <cocci@...teme.lip6.fr>
Subject: Re: s390/net: Deletion of unnecessary checks before two function
 calls

On Mon, Nov 03, 2014 at 05:50:48PM +0100, SF Markus Elfring wrote:
> > After your patch then it will print warning messages.
> 
> To which messages do you refer to?
> 

Open your eyeballs up and read the code.

> 
> > The truth is I think that all these patches are bad and they make the
> > code harder to read.
> > 
> > Before:  The code is clear and there is no NULL dereference.
> 
> Where do you stumble on a null pointer access?
> 

I'm not talking about bugs, I'm talking about code clarity.  Before is
more clear than after.

> 
> >  After:  You have to remember that rtw_free_netdev() accepts NULL
> > 	 pointers but free_netdev() does not accept NULL pointers.
> 
> Are any improvements needed for the corresponding documentation to make it
> better accessible besides the source code?
> 

Documentation doesn't reduce the number of things to remember it just
documents it.  Meanwhile if we leave the code as-is there is no need for
documentation because the code is clear.

> 
> > The if statements are there for *human* readers to understand and you are
> > making it harder for humans to understand the code.
> 
> Is there a target conflict between source code understandability
> and software efficiency?

If you can benchmark the code and the new code is faster then, yes, this
patch is good and we will apply it.  If you have no benchmarks then do
not send the patch.

> 
> > Even for kfree(), just removing the if statement is not really the right
> > fix.  We do it because everyone knows kfree(), but what Julia Lawall
> > said is the real correct way change the code and make it simpler for
> > people to understand:
> > 
> > https://lkml.org/lkml/2014/10/31/452
> 
> You refer to another update suggestion for the software area
> "staging: rtl8188eu".
> Do you find adjustments for jump labels easier to accept than the simple
> deletion of specific null pointer checks?

Yes.

> 
> 
> > I know it's fun to send automated patches but these make the code worse
> > and they waste reviewer time.
> 
> I hope that small automated changes can also help to improve affected
> source files.

No.  The changes make the code less clear.

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/