From: Dan Williams <dcbw@redhat.com>
To: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
Cc: Julia Lawall <Julia.Lawall@lip6.fr>,
"John W. Linville" <linville@tuxdriver.com>,
kernel-janitors@vger.kernel.org,
libertas-dev@lists.infradead.org, linux-wireless@vger.kernel.org,
netdev@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/13] libertas: make return of 0 explicit
Date: Mon, 19 May 2014 12:25:41 -0500 [thread overview]
Message-ID: <1400520341.3819.15.camel@dcbw.local> (raw)
In-Reply-To: <5379FCD0.2030506@cogentembedded.com>
On Mon, 2014-05-19 at 16:45 +0400, Sergei Shtylyov wrote:
> Hello.
>
> On 19-05-2014 8:31, Julia Lawall wrote:
>
> > From: Julia Lawall <Julia.Lawall@lip6.fr>
>
> > Delete unnecessary local variable whose value is always 0 and that hides
> > the fact that the result is always 0.
>
> > A simplified version of the semantic patch that fixes this problem is as
> > follows: (http://coccinelle.lip6.fr/)
>
> > // <smpl>
> > @r exists@
> > local idexpression ret;
> > expression e;
> > position p;
> > @@
> >
> > -ret = 0;
> > ... when != ret = e
> > return
> > - ret
> > + 0
> > ;
> > // </smpl>
>
> > Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
>
> > ---
> > Alternatively, was an error code intended in the bad length case, as is
> > done in process_brxed_802_11_packet?
>
> > drivers/net/wireless/libertas/rx.c | 7 ++-----
> > 1 file changed, 2 insertions(+), 5 deletions(-)
>
> > diff --git a/drivers/net/wireless/libertas/rx.c b/drivers/net/wireless/libertas/rx.c
> > index c7366b0..807c5b8 100644
> > --- a/drivers/net/wireless/libertas/rx.c
> > +++ b/drivers/net/wireless/libertas/rx.c
> [...]
> > @@ -154,10 +152,9 @@ int lbs_process_rxed_packet(struct lbs_private *priv, struct sk_buff *skb)
> > else
> > netif_rx_ni(skb);
> >
> > - ret = 0;
> > done:
> > - lbs_deb_leave_args(LBS_DEB_RX, "ret %d", ret);
> > - return ret;
> > + lbs_deb_leave_args(LBS_DEB_RX, "ret %d", 0);
>
> Why not just "ret 0"?
I think instead we want:
diff --git a/drivers/net/wireless/libertas/rx.c b/drivers/net/wireless/libertas/rx.c
index c7366b0..e446fed 100644
--- a/drivers/net/wireless/libertas/rx.c
+++ b/drivers/net/wireless/libertas/rx.c
@@ -67,30 +67,32 @@ int lbs_process_rxed_packet(struct lbs_private *priv, struct sk_buff *skb)
lbs_deb_enter(LBS_DEB_RX);
BUG_ON(!skb);
skb->ip_summed = CHECKSUM_NONE;
- if (priv->wdev->iftype == NL80211_IFTYPE_MONITOR)
- return process_rxed_802_11_packet(priv, skb);
+ if (priv->wdev->iftype == NL80211_IFTYPE_MONITOR) {
+ ret = process_rxed_802_11_packet(priv, skb);
+ goto done;
+ }
p_rx_pd = (struct rxpd *) skb->data;
p_rx_pkt = (struct rxpackethdr *) ((u8 *)p_rx_pd +
le32_to_cpu(p_rx_pd->pkt_ptr));
dev = lbs_mesh_set_dev(priv, dev, p_rx_pd);
lbs_deb_hex(LBS_DEB_RX, "RX Data: Before chop rxpd", skb->data,
min_t(unsigned int, skb->len, 100));
if (skb->len < (ETH_HLEN + 8 + sizeof(struct rxpd))) {
lbs_deb_rx("rx err: frame received with bad length\n");
dev->stats.rx_length_errors++;
- ret = 0;
+ ret = -EINVAL;
dev_kfree_skb(skb);
goto done;
}
lbs_deb_rx("rx data: skb->len - pkt_ptr = %d-%zd = %zd\n",
skb->len, (size_t)le32_to_cpu(p_rx_pd->pkt_ptr),
skb->len - (size_t)le32_to_cpu(p_rx_pd->pkt_ptr));
Since that (a) preserves the enter/leave balance when monitor mode is
enabled, and (b) fixes the return code that has been 0 since the driver
was added to the tree in early 2007. Dave Woodhouse fixed the
process_rxed_802_11_packet() return code for EINVAL case in commit
7bf02c29 from late 2007, and I think the normal codepath should do
EINVAL too. (though it turns out nothing cares about the return code
anyway, we should still fix it)
Dan
next prev parent reply other threads:[~2014-05-19 17:26 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-05-19 4:31 [PATCH 0/13] make return of 0 explicit Julia Lawall
2014-05-19 4:31 ` [PATCH 1/13] libertas: " Julia Lawall
2014-05-19 7:43 ` walter harms
2014-05-19 12:45 ` Sergei Shtylyov
2014-05-19 17:25 ` Dan Williams [this message]
2014-05-22 12:32 ` [PATCH] libertas: fix return value when processing invalid packet Dan Williams
2014-05-22 21:57 ` James Cameron
2014-05-20 0:30 ` [PATCH 1/13] libertas: make return of 0 explicit Julia Lawall
2014-05-20 0:36 ` Julia Lawall
2014-05-19 4:31 ` [PATCH 12/13] brcmsmac: " Julia Lawall
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1400520341.3819.15.camel@dcbw.local \
--to=dcbw@redhat.com \
--cc=Julia.Lawall@lip6.fr \
--cc=kernel-janitors@vger.kernel.org \
--cc=libertas-dev@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-wireless@vger.kernel.org \
--cc=linville@tuxdriver.com \
--cc=netdev@vger.kernel.org \
--cc=sergei.shtylyov@cogentembedded.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).