On Feb 12, 2024, at 7:26 AM, Oliver Hartkopp <socketcan@xxxxxxxxxxxx> wrote: > I'm currently in discussion with Guy Harris who's working on the Linux-CAN support in Wireshark. *AND* in libpcap. Note that anything done with the metadata headers of SocketCAN packets affects both of them - and any programs *other* than Wireshark that capture on SocketCAN adapters or read pcap or pcapng files with SocketCAN packets. > We current have two approaches how to integrate the VCID into the struct canxl_frame: > > 1. Add the VCID at a specific 16 bit offset (above the 11 bit Prio ID) > 2. Create a endian-dependent data structure with a separate uint8 VCID > > 1: https://lore.kernel.org/linux-can/20240212151404.60828-1-socketcan@xxxxxxxxxxxx/ > 2: https://lore.kernel.org/linux-can/20240128183758.68401-1-socketcan@xxxxxxxxxxxx/ > > The endian-dependent data structure looks like this: > > struct canxl_frame { > #if defined(__LITTLE_ENDIAN) > __u16 prio; /* 11 bit priority for arbitration */ > __u8 vcid; /* virtual CAN network identifier */ > __u8 __res0; /* reserved / padding must be set to zero */ > #elif defined(__BIG_ENDIAN) > __u8 __res0; /* reserved / padding must be set to zero */ > __u8 vcid; /* virtual CAN network identifier */ > __u16 prio; /* 11 bit priority for arbitration */ > #else > #error "Unknown endianness" > #endif > __u8 flags; /* additional flags for CAN XL */ > __u8 sdt; /* SDU (service data unit) type */ > __u16 len; /* frame payload length in byte */ > __u32 af; /* acceptance field */ > __u8 data[CANXL_MAX_DLEN]; > }; > > @Guy: Besides the fact that suggestion 2 does not win a design prize I'm not sure whether solution 1 or 2 are better to maintain over lifetime. The first question to ask is "which solution breaks less existing source and binary code"? Currently, the SocketCAN metadata header for CAN XL frames begins with a 4-byte integral-valued field. I'm not seeing code in the tip of the main branch, in either the git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git repository or the git://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can.git repository, that sets or gets the prio field of the canxl_frame structure, so I'm guessing that the prio field in a canal_frame structure would be in *host* byte order when received or sent. Presumably any software that sees that structure would either 1) treat the prio field as containing the priority in the low-order 11 bits and zeroes in the upper 21 bits or 2) tread the prio field as containing the priority in the low-order 11 bits and would extract the priority by ANDing the prio field with CANXL_PRIO_MASK. At the binary level, code that does 1) would be broken by putting the VCID in that field; the only way not to break it would be to add an additional field for the VCID, but *that* would change the length of the metadata header, which would break, at the binary level, any code that thinks the header is 12 bytes long, so there's nothing you can to to add the VCID without breaking that software, so that code shouldn't be taken into account. At the *source* level, code that does 1) would be broken by putting the VCID in the 8 bits above the lower 16 bits, and wouldn't be broken by changing the structure to have the priority in a 16-bit field, with additional 8-bit vcid and reserved fields. Code that does 2) would not be broken by *either* solution, at the source or binary level. Having that as a 32-bit field named "priority" gives the field a name that doesn't indicate that it also includes the VCID. Renaming of the field as "priority_vcid" would break source compatibility; adding #define priority priority_vcid would fix that, at the expense of possibly breaking or, at least, uglifying debugging of code that has a variable or function or structure name or... named "priority". Having that as a 16-bit field named "priority", an 8-bit field named "vcid", and an 8-bit reserved field involves, for structure layout compatibility, two different sets of structure members, with byte-order tests at compile time, which is a bit ugly, but does make it clear that one field contains the priority (and 5 reserved bits) and another field contains the VCID. *Both* solutions result in the metadata header's first 4 bytes being: byte 0: lower 8 bits of the priority; byte 1: 5 reserved bits and upper 3 bits of the priority; byte 2: VCID byte 3: 8 reserved bits on little-endian machines and byte 0: 8 reserved bits byte 1: VCID byte 3: 5 reserved bits and upper 3 bits of the priority; byte 4: lower 8 bits of the priority on big-endian machines, i.e. the layout of the data in memory is the same for *both* structures.
