| Message ID | 20221115212614.1308132-3-ammar.faizi@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | io_uring uapi updates | expand |
On 11/15/22 2:29 PM, Ammar Faizi wrote: > From: Ammar Faizi <ammarfaizi2@gnuweeb.org> > > Don't use a zero-size array because it doesn't allow the user to > compile an app that uses liburing with the `-pedantic-errors` flag: > > io_uring.h:611:28: error: zero size arrays are an extension [-Werror,-Wzero-length-array] > > Replace the array size from 0 to 1. > > - No functional change is intended. > - No struct/union size change. The only reason why they don't grow the struct, is because it's in a union. I don't like this patch, as the zero sized array is a clear sign that this struct has data past it. If it's a single entry, that's very different. Yes that apparently makes pendantic errors unhappy, but I care more about the readability of it.
On 11/15/22 21:29, Ammar Faizi wrote: > From: Ammar Faizi <ammarfaizi2@gnuweeb.org> > > Don't use a zero-size array because it doesn't allow the user to > compile an app that uses liburing with the `-pedantic-errors` flag: Ammar, I'd strongly encourage you to at least compile your patches or even better actually test them. There is an explicit BUILD_BUG_ON() violated by this change.
On 11/16/22 5:14 PM, Pavel Begunkov wrote: > On 11/15/22 21:29, Ammar Faizi wrote: >> From: Ammar Faizi <ammarfaizi2@gnuweeb.org> >> >> Don't use a zero-size array because it doesn't allow the user to >> compile an app that uses liburing with the `-pedantic-errors` flag: > > Ammar, I'd strongly encourage you to at least compile your > patches or even better actually test them. There is an explicit > BUILD_BUG_ON() violated by this change. Oh yeah, I didn't realize that. This patch breaks this assertion: BUILD_BUG_ON failed: sizeof_field(struct io_uring_sqe, cmd) != 0 This assertion wants the size of cmd[] to be zero. Which is obviously violated in this patch. I only tested a liburing app that uses this header and validated that the struct size is the same, but not its field. That's my mistake. I'm *not* going to send a v2 per Jens' comment.
diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h index 77027cbaf786..0890784fcc9e 100644 --- a/include/uapi/linux/io_uring.h +++ b/include/uapi/linux/io_uring.h @@ -85,25 +85,25 @@ struct io_uring_sqe { __u16 __pad3[1]; }; }; union { struct { __u64 addr3; __u64 __pad2[1]; }; /* * If the ring is initialized with IORING_SETUP_SQE128, then * this field is used for 80 bytes of arbitrary command data */ - __u8 cmd[0]; + __u8 cmd[1]; }; }; /* * If sqe->file_index is set to this for opcodes that instantiate a new * direct descriptor (like openat/openat2/accept), then io_uring will allocate * an available direct descriptor instead of having the application pass one * in. The picked direct descriptor will be returned in cqe->res, or -ENFILE * if the space is full. */ #define IORING_FILE_INDEX_ALLOC (~0U) @@ -599,25 +599,25 @@ struct io_uring_buf { struct io_uring_buf_ring { union { /* * To avoid spilling into more pages than we need to, the * ring tail is overlaid with the io_uring_buf->resv field. */ struct { __u64 resv1; __u32 resv2; __u16 resv3; __u16 tail; }; - struct io_uring_buf bufs[0]; + struct io_uring_buf bufs[1]; }; }; /* argument for IORING_(UN)REGISTER_PBUF_RING */ struct io_uring_buf_reg { __u64 ring_addr; __u32 ring_entries; __u16 bgid; __u16 pad; __u64 resv[3]; };