On Fri. 22 Jul. 2022 at 04:10, Oliver Hartkopp <socketcan@xxxxxxxxxxxx> wrote: > On 21.07.22 10:14, Vincent Mailhol wrote: > > On Thu. 21 Jul. 2022 at 16:53, Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx> wrote: > >> On 21.07.2022 09:36:21, Oliver Hartkopp wrote: > >>> Btw. How should we finally name the 'non data' header of CAN XL? > >>> > >>> 1. CANXL_HEADER_SIZE > >>> 2. CANXL_HEAD_SIZE > >>> 3. CANXL_HDR_SIZE > >>> 4. CANXL_HDR_SZ <- currently in the patches > >>> 5. CANXL_HD_SZ > >>> > >>> I think it has to be 'head' and not 'header'. > >> > >> Header! Header is in front of data. > > > > I am also part of the header team! By analogy with: > > https://en.wikipedia.org/wiki/IPv4#Header > > > >>> In skbs we also have head and tail. > >> > >> Yes, but they point at the head or tail of the buffer allocated with the > >> skb. > >> > >>> So I would vote for 2 or 5 with a tendency to 5. > >> > >> 3, 1, 4 > > > > My top vote goes to: > > 6. No macro, instead use flexible array member and do sizeof(struct canxl_frame) > > This is no sizeof(struct canxl_frame) anymore with the use of a flexible > array because a valid "CAN XL frame" has data (at least one byte and up > to 2048 byte). > > You might name it > > struct canxl_header { > canid_t prio; /* 11 bit priority for arbitration (canid_t) */ > __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[]; > }; > > But then you can't build a struct canxl_frame anymore in the way that > you can access the elements as you can do it today: > > struct canxl_frame { > struct canxl_header xldhr; > data[CANXL_MAX_DLEN]; > }; > > struct canxl_frame cfx; > > => cfx.xlhdr.len > > Which is not cfx.len anymore what is a known pattern from struct > can[fd]_frame from CAN application programmers and simple to use. > > The only new thing is the possibility to handle a truncated data[] > section. That should be feasible to learn. I do not buy your argument that you can not do sizeof(struct canxl_frame) because of naming. With a flexible array member, I can do: struct canxl_frame { canid_t prio; /* 11 bit priority for arbitration (canid_t) */ __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[]; }; void foo (int s) { struct canxl_frame cxl_hdr = { 0 }; struct canxl_frame *cxl1, *cxl2; size_t cxl2_data_len, cxl2_frame_len; /* read header */ assert(read(s, &cxl_hdr, sizeof(cxl_hdr)) == sizeof(cxl_hdr)); cxl1 = malloc(sizeof(*cxl1) + cxl_hdr.len); memcpy(cxl1, &cxl_hdr, sizeof(cxl_hdr)); /* read remaining data */ assert(read(s, cxl1->data, cxl1->len) == cxl1->len); cxl2_data_len = 512; /* arbitrary value for the example */ cxl2_frame_len = sizeof(*cxl2) + cxl2_data_len; cxl2 = malloc(cxl2_frame_len); memset(cxl2, cxl2_frame_len, 0); cxl2->len = cxl2_data_len; cxl2->flags = CANXL_XLF; /* fill prio, data, ect... */ assert(write(s, cxl2, cxl2_frame_len) == cxl2_frame_len); } This is not a fantasy from myself. Flexible array members are used a lot in the uapi and do not define a XXX_HDR_SIZE macro. I can send again the exemple from ethtool which I shared before: * kernel definition: https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/ethtool.h#L1163 * userland code: https://kernel.googlesource.com/pub/scm/network/ethtool/ethtool/+/refs/heads/ethtool-3.4.y/ethtool.c#2989 In the userland code of the above example, you can see that the same structure (without a "header" suffix) is used to manage both headers and full messages. Extract: struct ethtool_rxfh_indir indir_head; struct ethtool_rxfh_indir *indir; Even the ones which add the "header" suffix do not declare the XXX_HDR_SIZE macro. Example: struct ip_auth_hdr from linux/ip.h: https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/ip.h#L109 Furthermore, in this struct ip_auth_hdr, the auth_data field is specified to be at least 4 bytes, which clearly contradict your argument that "This is no sizeof(struct canxl_frame) anymore with the use of a flexible array because a valid CAN XL frame has data". The flexible array member is the standard method to handle structure of variable length. And by standard, I literally mean so (c.f. ISO C99, section 6.7.2.1 paragraph 16). And people have been happily using sizeof(struct foo) to get the header size for decades. I do not see why canxl_frame should be different. This is basically what I meant by saying that the flexible array members are more idiomatic. It is closer to what other people have been doing in the uapi so far. As you said, CANXL is closer to Ethernet than CAN. So I do not see the point to force some similarities between the struct can(fd)_frame and the struct canxl_frame. I looked for counterexamples in the uapi headers that would prove me wrong, but didn't find any. Yours sincerely, Vincent Mailhol
