Re: [RFC PATCH v1 5/5] Add io_uring data structure build assertion

[Pavel Begunkov <asml.silence@gmail.com>]

On 6/6/22 07:12, Ammar Faizi wrote:
> Ensure io_uring data structure consistent between the kernel and user
> space. These assertions are taken from io_uring.c in the kernel.

I don't see why would we do that. io_uring.h is only intended to be
copied from the kernel's uapi without some weird changes in structs,
I really really hope nobody will be trying to modify it separately.

But the real downside is that we'll need to maintain a full
duplicate of it and keep updating both for no good reason.
What do I miss?

> 
> Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
> ---
>   src/Makefile       |  3 ++-
>   src/build_assert.h | 57 ++++++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 59 insertions(+), 1 deletion(-)
>   create mode 100644 src/build_assert.h
> 
> diff --git a/src/Makefile b/src/Makefile
> index 12cf49f..aed3c40 100644
> --- a/src/Makefile
> +++ b/src/Makefile
> @@ -7,7 +7,8 @@ libdevdir ?= $(prefix)/lib
>   
>   CPPFLAGS ?=
>   override CPPFLAGS += -D_GNU_SOURCE \
> -	-Iinclude/ -include ../config-host.h
> +	-Iinclude/ -include ../config-host.h \
> +	-include build_assert.h
>   CFLAGS ?= -g -O2 -Wall -Wextra -fno-stack-protector
>   override CFLAGS += -Wno-unused-parameter -Wno-sign-compare -DLIBURING_INTERNAL
>   SO_CFLAGS=-fPIC $(CFLAGS)
> diff --git a/src/build_assert.h b/src/build_assert.h
> new file mode 100644
> index 0000000..5b2a9c6
> --- /dev/null
> +++ b/src/build_assert.h
> @@ -0,0 +1,57 @@
> +/* SPDX-License-Identifier: MIT */
> +
> +#ifndef LIBURING_BUILD_ASSERT_H
> +#define LIBURING_BUILD_ASSERT_H
> +
> +#include "liburing/io_uring.h"
> +#include "lib.h"
> +
> +static inline __attribute__((__unused__)) void io_uring_build_assert(void)
> +{
> +#define __BUILD_BUG_VERIFY_ELEMENT(stype, eoffset, etype, ename) do { \
> +	BUILD_BUG_ON(offsetof(stype, ename) != eoffset); \
> +	BUILD_BUG_ON(sizeof(etype) != sizeof_field(stype, ename)); \
> +} while (0)
> +
> +#define BUILD_BUG_SQE_ELEM(eoffset, etype, ename) \
> +	__BUILD_BUG_VERIFY_ELEMENT(struct io_uring_sqe, eoffset, etype, ename)
> +	BUILD_BUG_ON(sizeof(struct io_uring_sqe) != 64);
> +	BUILD_BUG_SQE_ELEM(0,  __u8,   opcode);
> +	BUILD_BUG_SQE_ELEM(1,  __u8,   flags);
> +	BUILD_BUG_SQE_ELEM(2,  __u16,  ioprio);
> +	BUILD_BUG_SQE_ELEM(4,  __s32,  fd);
> +	BUILD_BUG_SQE_ELEM(8,  __u64,  off);
> +	BUILD_BUG_SQE_ELEM(8,  __u64,  addr2);
> +	BUILD_BUG_SQE_ELEM(16, __u64,  addr);
> +	BUILD_BUG_SQE_ELEM(16, __u64,  splice_off_in);
> +	BUILD_BUG_SQE_ELEM(24, __u32,  len);
> +	BUILD_BUG_SQE_ELEM(28,     __kernel_rwf_t, rw_flags);
> +	BUILD_BUG_SQE_ELEM(28, /* compat */   int, rw_flags);
> +	BUILD_BUG_SQE_ELEM(28, /* compat */ __u32, rw_flags);
> +	BUILD_BUG_SQE_ELEM(28, __u32,  fsync_flags);
> +	BUILD_BUG_SQE_ELEM(28, /* compat */ __u16,  poll_events);
> +	BUILD_BUG_SQE_ELEM(28, __u32,  poll32_events);
> +	BUILD_BUG_SQE_ELEM(28, __u32,  sync_range_flags);
> +	BUILD_BUG_SQE_ELEM(28, __u32,  msg_flags);
> +	BUILD_BUG_SQE_ELEM(28, __u32,  timeout_flags);
> +	BUILD_BUG_SQE_ELEM(28, __u32,  accept_flags);
> +	BUILD_BUG_SQE_ELEM(28, __u32,  cancel_flags);
> +	BUILD_BUG_SQE_ELEM(28, __u32,  open_flags);
> +	BUILD_BUG_SQE_ELEM(28, __u32,  statx_flags);
> +	BUILD_BUG_SQE_ELEM(28, __u32,  fadvise_advice);
> +	BUILD_BUG_SQE_ELEM(28, __u32,  splice_flags);
> +	BUILD_BUG_SQE_ELEM(32, __u64,  user_data);
> +	BUILD_BUG_SQE_ELEM(40, __u16,  buf_index);
> +	BUILD_BUG_SQE_ELEM(40, __u16,  buf_group);
> +	BUILD_BUG_SQE_ELEM(42, __u16,  personality);
> +	BUILD_BUG_SQE_ELEM(44, __s32,  splice_fd_in);
> +	BUILD_BUG_SQE_ELEM(44, __u32,  file_index);
> +	BUILD_BUG_SQE_ELEM(48, __u64,  addr3);
> +
> +	BUILD_BUG_ON(sizeof(struct io_uring_files_update) !=
> +		     sizeof(struct io_uring_rsrc_update));
> +	BUILD_BUG_ON(sizeof(struct io_uring_rsrc_update) >
> +		     sizeof(struct io_uring_rsrc_update2));
> +}
> +
> +#endif

-- 
Pavel Begunkov