* [PATCH wpan-next] ieee802154: cleanup for lowpan_rcv
@ 2014-10-01 6:01 Varka Bhadram
2014-10-01 6:33 ` Alexander Aring
0 siblings, 1 reply; 4+ messages in thread
From: Varka Bhadram @ 2014-10-01 6:01 UTC (permalink / raw)
To: linux-wpan; +Cc: Varka Bhadram
instead of collecting the return value into 'ret' varible and
checking the perticular return value specifically, this patch
remove these by directly including function within 'if' statement
Signed-off-by: Varka Bhadram <varkab@cdac.in>
---
net/ieee802154/6lowpan_rtnl.c | 21 ++++++---------------
1 file changed, 6 insertions(+), 15 deletions(-)
diff --git a/net/ieee802154/6lowpan_rtnl.c b/net/ieee802154/6lowpan_rtnl.c
index 4413629..657940e 100644
--- a/net/ieee802154/6lowpan_rtnl.c
+++ b/net/ieee802154/6lowpan_rtnl.c
@@ -506,7 +506,6 @@ static int lowpan_rcv(struct sk_buff *skb, struct net_device *dev,
struct packet_type *pt, struct net_device *orig_dev)
{
struct ieee802154_hdr hdr;
- int ret;
skb = skb_share_check(skb, GFP_ATOMIC);
if (!skb)
@@ -529,31 +528,23 @@ static int lowpan_rcv(struct sk_buff *skb, struct net_device *dev,
/* Pull off the 1-byte of 6lowpan header. */
skb_pull(skb, 1);
- ret = lowpan_give_skb_to_devices(skb, NULL);
- if (ret == NET_RX_DROP)
+ if (lowpan_give_skb_to_devices(skb, NULL))
goto drop;
} else {
switch (skb->data[0] & 0xe0) {
case LOWPAN_DISPATCH_IPHC: /* ipv6 datagram */
- ret = process_data(skb, &hdr);
- if (ret == NET_RX_DROP)
+ if (process_data(skb, &hdr))
goto drop;
break;
case LOWPAN_DISPATCH_FRAG1: /* first fragment header */
- ret = lowpan_frag_rcv(skb, LOWPAN_DISPATCH_FRAG1);
- if (ret == 1) {
- ret = process_data(skb, &hdr);
- if (ret == NET_RX_DROP)
+ if (lowpan_frag_rcv(skb, LOWPAN_DISPATCH_FRAG1))
+ if (process_data(skb, &hdr))
goto drop;
- }
break;
case LOWPAN_DISPATCH_FRAGN: /* next fragments headers */
- ret = lowpan_frag_rcv(skb, LOWPAN_DISPATCH_FRAGN);
- if (ret == 1) {
- ret = process_data(skb, &hdr);
- if (ret == NET_RX_DROP)
+ if (lowpan_frag_rcv(skb, LOWPAN_DISPATCH_FRAGN))
+ if (process_data(skb, &hdr))
goto drop;
- }
break;
default:
break;
--
1.7.9.5
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH wpan-next] ieee802154: cleanup for lowpan_rcv
2014-10-01 6:01 [PATCH wpan-next] ieee802154: cleanup for lowpan_rcv Varka Bhadram
@ 2014-10-01 6:33 ` Alexander Aring
2014-10-01 6:38 ` Varka Bhadram
0 siblings, 1 reply; 4+ messages in thread
From: Alexander Aring @ 2014-10-01 6:33 UTC (permalink / raw)
To: Varka Bhadram; +Cc: linux-wpan, Varka Bhadram, martin.townsend
On Wed, Oct 01, 2014 at 11:31:46AM +0530, Varka Bhadram wrote:
> instead of collecting the return value into 'ret' varible and
s/varible/variable
> checking the perticular return value specifically, this patch
> remove these by directly including function within 'if' statement
>
> Signed-off-by: Varka Bhadram <varkab@cdac.in>
> ---
> net/ieee802154/6lowpan_rtnl.c | 21 ++++++---------------
> 1 file changed, 6 insertions(+), 15 deletions(-)
>
> diff --git a/net/ieee802154/6lowpan_rtnl.c b/net/ieee802154/6lowpan_rtnl.c
> index 4413629..657940e 100644
> --- a/net/ieee802154/6lowpan_rtnl.c
> +++ b/net/ieee802154/6lowpan_rtnl.c
> @@ -506,7 +506,6 @@ static int lowpan_rcv(struct sk_buff *skb, struct net_device *dev,
> struct packet_type *pt, struct net_device *orig_dev)
> {
> struct ieee802154_hdr hdr;
> - int ret;
>
> skb = skb_share_check(skb, GFP_ATOMIC);
> if (!skb)
> @@ -529,31 +528,23 @@ static int lowpan_rcv(struct sk_buff *skb, struct net_device *dev,
> /* Pull off the 1-byte of 6lowpan header. */
> skb_pull(skb, 1);
>
> - ret = lowpan_give_skb_to_devices(skb, NULL);
> - if (ret == NET_RX_DROP)
> + if (lowpan_give_skb_to_devices(skb, NULL))
> goto drop;
> } else {
> switch (skb->data[0] & 0xe0) {
> case LOWPAN_DISPATCH_IPHC: /* ipv6 datagram */
> - ret = process_data(skb, &hdr);
> - if (ret == NET_RX_DROP)
> + if (process_data(skb, &hdr))
> goto drop;
> break;
> case LOWPAN_DISPATCH_FRAG1: /* first fragment header */
> - ret = lowpan_frag_rcv(skb, LOWPAN_DISPATCH_FRAG1);
> - if (ret == 1) {
> - ret = process_data(skb, &hdr);
> - if (ret == NET_RX_DROP)
> + if (lowpan_frag_rcv(skb, LOWPAN_DISPATCH_FRAG1))
> + if (process_data(skb, &hdr))
> goto drop;
> - }
> break;
> case LOWPAN_DISPATCH_FRAGN: /* next fragments headers */
> - ret = lowpan_frag_rcv(skb, LOWPAN_DISPATCH_FRAGN);
> - if (ret == 1) {
> - ret = process_data(skb, &hdr);
> - if (ret == NET_RX_DROP)
> + if (lowpan_frag_rcv(skb, LOWPAN_DISPATCH_FRAGN))
> + if (process_data(skb, &hdr))
> goto drop;
> - }
> break;
> default:
> break;
Mainly I would ack this, I detect no change. But this handling is
currently complete broken and Martin already work on this. [0]
I will not apply it. First fix the broken error handling, then you can
send cleanups for everything in 6LoWPAN receive handling.
Martin:
ping ping ping, this needs a fix by you! I also have the next header
compression framework and I begin to forget what I did there. ;-)
Otherwise I could try to send another version for this issue, when you
are fine with that? Or do you nearly at the end to send a new version?
- Alex
[0] http://marc.info/?l=linux-bluetooth&m=141079021702990&w=2
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH wpan-next] ieee802154: cleanup for lowpan_rcv
2014-10-01 6:33 ` Alexander Aring
@ 2014-10-01 6:38 ` Varka Bhadram
2014-10-01 7:03 ` Alexander Aring
0 siblings, 1 reply; 4+ messages in thread
From: Varka Bhadram @ 2014-10-01 6:38 UTC (permalink / raw)
To: Alexander Aring; +Cc: linux-wpan, Varka Bhadram, martin.townsend
Alex,
On 10/01/2014 12:03 PM, Alexander Aring wrote:
>> checking the perticular return value specifically, this patch
>> remove these by directly including function within 'if' statement
>>
>> Signed-off-by: Varka Bhadram <varkab@cdac.in>
>> ---
>> net/ieee802154/6lowpan_rtnl.c | 21 ++++++---------------
>> 1 file changed, 6 insertions(+), 15 deletions(-)
>>
>> diff --git a/net/ieee802154/6lowpan_rtnl.c b/net/ieee802154/6lowpan_rtnl.c
>> index 4413629..657940e 100644
>> --- a/net/ieee802154/6lowpan_rtnl.c
>> +++ b/net/ieee802154/6lowpan_rtnl.c
>> @@ -506,7 +506,6 @@ static int lowpan_rcv(struct sk_buff *skb, struct net_device *dev,
>> struct packet_type *pt, struct net_device *orig_dev)
>> {
>> struct ieee802154_hdr hdr;
>> - int ret;
>>
>> skb = skb_share_check(skb, GFP_ATOMIC);
>> if (!skb)
>> @@ -529,31 +528,23 @@ static int lowpan_rcv(struct sk_buff *skb, struct net_device *dev,
>> /* Pull off the 1-byte of 6lowpan header. */
>> skb_pull(skb, 1);
>>
>> - ret = lowpan_give_skb_to_devices(skb, NULL);
>> - if (ret == NET_RX_DROP)
>> + if (lowpan_give_skb_to_devices(skb, NULL))
>> goto drop;
>> } else {
>> switch (skb->data[0] & 0xe0) {
>> case LOWPAN_DISPATCH_IPHC: /* ipv6 datagram */
>> - ret = process_data(skb, &hdr);
>> - if (ret == NET_RX_DROP)
>> + if (process_data(skb, &hdr))
>> goto drop;
>> break;
>> case LOWPAN_DISPATCH_FRAG1: /* first fragment header */
>> - ret = lowpan_frag_rcv(skb, LOWPAN_DISPATCH_FRAG1);
>> - if (ret == 1) {
>> - ret = process_data(skb, &hdr);
>> - if (ret == NET_RX_DROP)
>> + if (lowpan_frag_rcv(skb, LOWPAN_DISPATCH_FRAG1))
>> + if (process_data(skb, &hdr))
>> goto drop;
>> - }
>> break;
>> case LOWPAN_DISPATCH_FRAGN: /* next fragments headers */
>> - ret = lowpan_frag_rcv(skb, LOWPAN_DISPATCH_FRAGN);
>> - if (ret == 1) {
>> - ret = process_data(skb, &hdr);
>> - if (ret == NET_RX_DROP)
>> + if (lowpan_frag_rcv(skb, LOWPAN_DISPATCH_FRAGN))
>> + if (process_data(skb, &hdr))
>> goto drop;
>> - }
>> break;
>> default:
>> break;
> Mainly I would ack this, I detect no change. But this handling is
> currently complete broken and Martin already work on this. [0]
>
> I will not apply it. First fix the broken error handling, then you can
> send cleanups for everything in 6LoWPAN receive handling.
>
Where is error handling is broken...?
--
Regards,
Varka Bhadram.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH wpan-next] ieee802154: cleanup for lowpan_rcv
2014-10-01 6:38 ` Varka Bhadram
@ 2014-10-01 7:03 ` Alexander Aring
0 siblings, 0 replies; 4+ messages in thread
From: Alexander Aring @ 2014-10-01 7:03 UTC (permalink / raw)
To: Varka Bhadram; +Cc: linux-wpan, Varka Bhadram, martin.townsend
On Wed, Oct 01, 2014 at 12:08:56PM +0530, Varka Bhadram wrote:
> Alex,
>
> On 10/01/2014 12:03 PM, Alexander Aring wrote:
>
> >>checking the perticular return value specifically, this patch
> >>remove these by directly including function within 'if' statement
> >>
> >>Signed-off-by: Varka Bhadram <varkab@cdac.in>
> >>---
> >> net/ieee802154/6lowpan_rtnl.c | 21 ++++++---------------
> >> 1 file changed, 6 insertions(+), 15 deletions(-)
> >>
> >>diff --git a/net/ieee802154/6lowpan_rtnl.c b/net/ieee802154/6lowpan_rtnl.c
> >>index 4413629..657940e 100644
> >>--- a/net/ieee802154/6lowpan_rtnl.c
> >>+++ b/net/ieee802154/6lowpan_rtnl.c
> >>@@ -506,7 +506,6 @@ static int lowpan_rcv(struct sk_buff *skb, struct net_device *dev,
> >> struct packet_type *pt, struct net_device *orig_dev)
> >> {
> >> struct ieee802154_hdr hdr;
> >>- int ret;
> >> skb = skb_share_check(skb, GFP_ATOMIC);
> >> if (!skb)
> >>@@ -529,31 +528,23 @@ static int lowpan_rcv(struct sk_buff *skb, struct net_device *dev,
> >> /* Pull off the 1-byte of 6lowpan header. */
> >> skb_pull(skb, 1);
> >>- ret = lowpan_give_skb_to_devices(skb, NULL);
> >>- if (ret == NET_RX_DROP)
> >>+ if (lowpan_give_skb_to_devices(skb, NULL))
> >> goto drop;
> >> } else {
> >> switch (skb->data[0] & 0xe0) {
> >> case LOWPAN_DISPATCH_IPHC: /* ipv6 datagram */
> >>- ret = process_data(skb, &hdr);
> >>- if (ret == NET_RX_DROP)
> >>+ if (process_data(skb, &hdr))
> >> goto drop;
> >> break;
> >> case LOWPAN_DISPATCH_FRAG1: /* first fragment header */
> >>- ret = lowpan_frag_rcv(skb, LOWPAN_DISPATCH_FRAG1);
> >>- if (ret == 1) {
> >>- ret = process_data(skb, &hdr);
> >>- if (ret == NET_RX_DROP)
> >>+ if (lowpan_frag_rcv(skb, LOWPAN_DISPATCH_FRAG1))
> >>+ if (process_data(skb, &hdr))
> >> goto drop;
> >>- }
> >> break;
> >> case LOWPAN_DISPATCH_FRAGN: /* next fragments headers */
> >>- ret = lowpan_frag_rcv(skb, LOWPAN_DISPATCH_FRAGN);
> >>- if (ret == 1) {
> >>- ret = process_data(skb, &hdr);
> >>- if (ret == NET_RX_DROP)
> >>+ if (lowpan_frag_rcv(skb, LOWPAN_DISPATCH_FRAGN))
> >>+ if (process_data(skb, &hdr))
> >> goto drop;
> >>- }
> >> break;
> >> default:
> >> break;
> >Mainly I would ack this, I detect no change. But this handling is
> >currently complete broken and Martin already work on this. [0]
> >
> >I will not apply it. First fix the broken error handling, then you can
> >send cleanups for everything in 6LoWPAN receive handling.
> >
> Where is error handling is broken...?
everywhere at this position, see what Martin does and the review notes.
There is a mixed return values of NET_RX_FOO and -EFOO and I am pretty
sure, we double freeing sk_buff's when error occurs. Martin will make
this behaviour easier and easy to understand. At the moment it's a mess.
- Alex
[0] http://marc.info/?l=linux-bluetooth&m=141079021702990&w=2
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2014-10-01 7:03 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-10-01 6:01 [PATCH wpan-next] ieee802154: cleanup for lowpan_rcv Varka Bhadram
2014-10-01 6:33 ` Alexander Aring
2014-10-01 6:38 ` Varka Bhadram
2014-10-01 7:03 ` Alexander Aring
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).