Re: [PATCH net-next 3/4] 6lowpan: the two bytes of u16 tag needs to be stored/accessed the other way around

[Tony Cheneau <tony.cheneau@amnesiak.org>]

Hello Alexander,

Please see my response inline.

Le Tue, 12 Jun 2012 22:07:12 +0400,
Alexander Smirnov <alex.bluesman.smirnov@gmail.com> a écrit :

> 2012/6/11 Tony Cheneau <tony.cheneauh@amnesiak.org>:
> > Or else, tag values, as displayed with a trafic analyser, such a
> > Wireshark, are not properly displayed (e.g. 0x01 00 insted of 0x00
> > 01, and so on). ---
> >  net/ieee802154/6lowpan.c |    6 +++---
> >  1 files changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/net/ieee802154/6lowpan.c b/net/ieee802154/6lowpan.c
> > index af4f29b..af2f12e 100644
> > --- a/net/ieee802154/6lowpan.c
> > +++ b/net/ieee802154/6lowpan.c
> > @@ -307,7 +307,7 @@ static u16 lowpan_fetch_skb_u16(struct sk_buff
> > *skb)
> >
> >        BUG_ON(!pskb_may_pull(skb, 2));
> >
> > -       ret = skb->data[0] | (skb->data[1] << 8);
> > +       ret = (skb->data[0] << 8) | skb->data[1];
> 
> This function aimed to obtain u16 from skb, why did you change the
> byte-order here instead of 'tag' variable byte-shifting? 
This probably reflects my lack of practice in writing/reading network
code. I witnessed in my network that the byte order was wrong when
sending/receiving packet (using Wireshark) and though it would be a
quick fix. Also, I assumed skb->data would store data using network
ordering, so that skb->data[0] would have been the MSB and
skb->data[1] would have been the LSB. I'll look more into that.

> Will this
> code work properly on the both little and big-endian machines?
I haven't checked that. But, just for clarification, if my changes are
not working properly on both architectures, it means that the original
code doesn't either, right?

> Please
> rework this to keep order properly for all the architectures.
I will read some more network code to see how it is handled in other
part of the kernel and rewrite the patch accordingly (if at all
needed). 

Thank you for your advises.

> >        skb_pull(skb, 2);
> >        return ret;
> >  }
> > @@ -1002,8 +1002,8 @@ lowpan_skb_fragmentation(struct sk_buff *skb)
> >        /* first fragment header */
> >        head[0] = LOWPAN_DISPATCH_FRAG1 | (payload_length & 0x7);
> >        head[1] = (payload_length >> 3) & 0xff;
> > -       head[2] = tag & 0xff;
> > -       head[3] = tag >> 8;
> > +       head[2] = tag >> 8;
> > +       head[3] = tag & 0xff;
> >
> >        err = lowpan_fragment_xmit(skb, head, header_length, 0, 0);
> >
> > --
> > 1.7.3.4
> >
> >
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Regards,
	Tony