Messages in this thread | | | Subject | Re: [PATCH net v3 2/4] net: add rx_nohandler stat counter | From | Jamal Hadi Salim <> | Date | Tue, 9 Feb 2016 05:56:28 -0500 |
| |
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
| |