linux-kernel - Re: [PATCH 1/3 v3] Staging: rtl8192u: Simplify error check code at prism2_wep

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: <20150520090351.GF4150@mwanda>
Date:	Wed, 20 May 2015 12:03:51 +0300
From:	Dan Carpenter <dan.carpenter@...cle.com>
To:	pmarzo <marzo.pedro@...il.com>
Cc:	devel@...verdev.osuosl.org, gregkh@...uxfoundation.org,
	haticeerturk27@...il.com, linux-kernel@...r.kernel.org,
	joe@...ches.com, dilekuzulmez@...il.com, navyasri.tech@...il.com
Subject: Re: [PATCH 1/3 v3] Staging: rtl8192u: Simplify error check code at
 prism2_wep_init

On Wed, May 20, 2015 at 10:26:30AM +0200, pmarzo wrote:
> On mié, 2015-05-20 at 00:46 +0300, Dan Carpenter wrote:
> > I was planning to leave this one for Greg but since you asked me to
> > comment...
> > 
> > This patch is ok as one patch.  The subject is "clean up
> > prism2_wep_init()" and that's one thing.  Breaking it up into tiny
> > patches would probably make it harder to review.
> > 
> > On Tue, May 19, 2015 at 01:32:22AM +0200, Pedro Marzo Perez wrote:
> > > Merge two pr_debug lines with literal strings splitted across several lines
> > > into one single line, simplifying prism2_wep_init error check code.
> > > 
> > > Signed-off-by: Pedro Marzo Perez <marzo.pedro@...il.com>
> > > ---
> > >  .../rtl8192u/ieee80211/ieee80211_crypt_wep.c       | 22 +++++++++-------------
> > >  1 file changed, 9 insertions(+), 13 deletions(-)
> > > 
> > > diff --git a/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_wep.c b/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_wep.c
> > > index 0a17f84..388ed3c 100644
> > > --- a/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_wep.c
> > > +++ b/drivers/staging/rtl8192u/ieee80211/ieee80211_crypt_wep.c
> > > @@ -9,6 +9,8 @@
> > >   * more details.
> > >   */
> > >  
> > > +#define pr_fmt(fmt) "ieee80211_crypt_wep: " fmt
> > 
> > Greg doesn't like pr_fmt() in driver code, use dev_dbg() and friends
> > instead.  I don't like debug output generally so I say just delete it.
> > Especially in this case the debug output is pretty useless.
> dev_dbg needs the device parameter, I don't see a way to get the device
> pointer within this function. Anyway I think you are right, this debug
> output is not very useful, if Greg agrees I will delete the line.

Greg doesn't care.  Don't wait for Greg.  He is a busy guy.

Greg is very predictable so I can normally tell you which patches will
be merged and which will not.

> I am curious, why you dont like debug output? I usually have to adapt
> the kernel to run in ARM based boards and I find debug output really
> useful.

These debug statements are never going to be printed in real life.  They
are a waste of RAM for no reason.  They make the code more complicated
which is why we are debating "how should we handle this thing?"  They
make the code slightly messier.  They can introduce NULL deref bugs in
code which never gets tested.

It's not that all debug code is bad, but so often people do it without
thinking because they think it's something they must do for every
function.  They think they are doing the right thing without realizing
that sometimes they are making the code worse.

Low level functions like crypto_alloc_blkcipher() should have debug code
and stack dumps on error.  It mostly does I think.  I think it assumes
that /lib/modules/ is not corrupt...

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/