Re: [PATCH nft] proto: fix VLAN header definition

[Patrick McHardy <kaber@trash.net>]

On 27.11, Florian Westphal wrote:
> Patrick McHardy <kaber@trash.net> wrote:
> > Yes, I also did that and it looks correct. I think we probably have a
> > discrepancy with bit numbering:
> > 
> > Looking at an older patch of you:
> > 
> > -               [IPHDR_VERSION]         = HDR_BITFIELD("version", &integer_type, 0, 4),
> > -               [IPHDR_HDRLENGTH]       = HDR_BITFIELD("hdrlength", &integer_type, 4, 4),
> > +               [IPHDR_VERSION]         = HDR_BITFIELD("version", &integer_type, 4, 4),
> > +               [IPHDR_HDRLENGTH]       = HDR_BITFIELD("hdrlength", &integer_type, 0, 4),
> > 
> > So you seem to assume a numbering which corresponds to how you would express
> > it in C. My patch assumes numbering as used in the RFCs/IEEE standards, which
> > is basically the opposite direction.
> 
> Right, there is a general problem with all sub-byte fields.
> 
> I just noticed that decoding of ip version/hdrlen doesn't work either.
> (ip hdrlength 4 ip version 5).
> 
> I am sure that I tested matching on ip version/hdrlen on both
> x86-64 and a MSB machine (don't recall architecture, ppc i think).

Yeah, the actual variant chosen doesn't matter, I guess we just need to make
sure it is handled consistently. I chose the other way because you can simply
use the offsets defined in the standards. That's how I (think I) expressed
all the protocol definitions, so if we're using the other way around, we'll
possibly have some more bitfields which are not correct with the current code.

I'll do a audit of the code and the other definitions and see which way would
be easier to fix.