netdev - Re: [PATCH] mwifiex: drop condition with no effect

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]

Message-ID: <20150709055747.GA22159@opentech.at>
Date:	Thu, 9 Jul 2015 07:57:47 +0200
From:	Nicholas Mc Guire <der.herr@...r.at>
To:	Avinash Patil <avinashapatil@...il.com>
Cc:	Nicholas Mc Guire <hofrat@...dl.org>,
	Amitkumar Karwar <akarwar@...vell.com>,
	Kalle Valo <kvalo@...eaurora.org>,
	"linux-wireless@...r.kernel.org" <linux-wireless@...r.kernel.org>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	Nishant Sarmukadam <nishants@...vell.com>
Subject: Re: [PATCH] mwifiex: drop condition with no effect

On Wed, 08 Jul 2015, Avinash Patil wrote:

> Hi Nicholas,
> 
> On Wed, Jul 8, 2015 at 7:15 AM, Nicholas Mc Guire <hofrat@...dl.org> wrote:
> > scanning for trivial bug-patters with coccinelle spatches returned:
> > ./drivers/net/wireless/mwifiex/sta_cmdresp.c:895
> >         WARNING: condition with no effect (if branch == else)
> >
> > originally added in 'commit d8d2f19feb16 ("mwifiex: silence TDLS link
> > delete failure for nonexistent link")' with dev_dbg/dev_err (though
> > with the same message) to differentiate severity and then in 'commit
> > acebe8c10a6e ("mwifiex: change dbg print func to mwifiex_dbg")' all
> > dev_dbg,dev_warn and dev_err got converted to mwifiex_dbg which should
> > thus probably drop this if/else as well.
> >
> > Signed-off-by: Nicholas Mc Guire <hofrat@...dl.org>
> > ---
> >
> > If dropping the if/else is not the right thing to do then commit
> > acebe8c10a6e "mwifiex: change dbg print func to mwifiex_dbg" probably
> > needs a review as well.
> >
> > Patch was compile tested with x86_64_defconfig + CONFIG_MWIFIEX=m
> >
> > Patch is against 4.2-rc1 (localversion-next is -next-20150708)
> >
> >  drivers/net/wireless/mwifiex/sta_cmdresp.c |   11 +++--------
> >  1 file changed, 3 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/net/wireless/mwifiex/sta_cmdresp.c b/drivers/net/wireless/mwifiex/sta_cmdresp.c
> > index b645884..e58f900 100644
> > --- a/drivers/net/wireless/mwifiex/sta_cmdresp.c
> > +++ b/drivers/net/wireless/mwifiex/sta_cmdresp.c
> > @@ -892,14 +892,9 @@ static int mwifiex_ret_tdls_oper(struct mwifiex_private *priv,
> >         switch (action) {
> >         case ACT_TDLS_DELETE:
> >                 if (reason) {
> > -                       if (!node || reason == TDLS_ERR_LINK_NONEXISTENT)
> > -                               mwifiex_dbg(priv->adapter, ERROR,
> > -                                           "TDLS link delete for %pM failed: reason %d\n",
> > -                                           cmd_tdls_oper->peer_mac, reason);
> > -                       else
> > -                               mwifiex_dbg(priv->adapter, ERROR,
> > -                                           "TDLS link delete for %pM failed: reason %d\n",
> > -                                           cmd_tdls_oper->peer_mac, reason);
> > +                       mwifiex_dbg(priv->adapter, ERROR,
> > +                                   "TDLS link delete for %pM failed: reason %d\n",
> > +                                   cmd_tdls_oper->peer_mac, reason);
> 
> I think reason why we had these 2 different prints is for first
> occurrence in "if" which may be not so serious we used to print with
> dev_dbg and second occurrence in "else" is more serious issue which
> was under dev_err.
> It would be better to use mwifiex_dbg with DBG/MSG in"if" condition
> instead of removing whole if/else.
>

ok - it seemed to me that since the reason was being printed it would be
clear from the error message which case was triggering this message but
you are right that the initial intent to differentiate severity levels
(from 'commit d8d2f19feb16 ("mwifiex: silence TDLS link delete failure
for nonexistent link")' would be better served with your suggestion - I 
just was not clear if this initial reason still holds.

will fix it up and resend.

thx!
hofrat
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html