Re: [PATCH net v3 2/4] net: add rx_nohandler stat counter

[Jamal Hadi Salim <jhs@mojatatu.com>]

On 16-02-09 03:40 AM, David Miller wrote:
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Mon, 08 Feb 2016 14:57:40 -0800
>
>> Whole point of TLV is that it allows us to add new fields at the end of
>> the structures.
>   ...
>> Look at iproute2, you were the one adding in 2004 code to cope with
>> various tcp_info sizes.
>>
>> So 12 years later, you cannot say it does not work anymore.
>
> +1
>

The TLV L should be canonical way to determine length. i.e should be
sufficient to just look at L and understand that content has changed.
But:
Using sizeof could be dangerous unless the data is packed to be
32-bit aligned. Looking INET_DIAG_INFO check for sizeof
there is a small 8 bit hole in tcp_info I think between
these two fields:

----
__u8    tcpi_snd_wscale : 4, tcpi_rcv_wscale : 4;
__u32   tcpi_rto;
---

The kernel will pad to make sure the TLV data is 32-bit aligned.
I am not sure if that will be the same length as sizeof() in all
hardware + compilers... For this case,
it is almost safe to just add a version field - probably in the hole.
Or have a #define to say what the expected length should be. Or add
an 8 bit pad.

In general adding new fields that are non-optional is problematic. i.e
by non-optional i mean always expected to be present.
I think a good test is old kernel with new iproute2. If the new field
is non-optional, it will fail (example iproute2 may try to print a value
that it expects but because old kernel doesnt understand it; it is 
non-existent).

cheers,
jamal