Re: [PATCH v4 1/3] io_uring: use an enumeration for io_uring_register(2) opcodes
On Wed, Aug 26, 2020 at 01:52:38PM -0600, Andreas Dilger wrote:
> On Aug 26, 2020, at 1:43 PM, Kees Cook wrote:
> >
> > On Thu, Aug 13, 2020 at 05:32:52PM +0200, Stefano Garzarella wrote:
> >> The enumeration allows us to keep track of the last
> >> io_uring_register(2) opcode available.
> >>
> >> Behaviour and opcodes names don't change.
> >>
> >> Signed-off-by: Stefano Garzarella
> >> ---
> >> include/uapi/linux/io_uring.h | 27 ---
> >> 1 file changed, 16 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h
> >> index d65fde732518..cdc98afbacc3 100644
> >> --- a/include/uapi/linux/io_uring.h
> >> +++ b/include/uapi/linux/io_uring.h
> >> @@ -255,17 +255,22 @@ struct io_uring_params {
> >> /*
> >> * io_uring_register(2) opcodes and arguments
> >> */
> >> -#define IORING_REGISTER_BUFFERS 0
> >> -#define IORING_UNREGISTER_BUFFERS 1
> >> -#define IORING_REGISTER_FILES 2
> >> -#define IORING_UNREGISTER_FILES 3
> >> -#define IORING_REGISTER_EVENTFD 4
> >> -#define IORING_UNREGISTER_EVENTFD 5
> >> -#define IORING_REGISTER_FILES_UPDATE 6
> >> -#define IORING_REGISTER_EVENTFD_ASYNC 7
> >> -#define IORING_REGISTER_PROBE 8
> >> -#define IORING_REGISTER_PERSONALITY 9
> >> -#define IORING_UNREGISTER_PERSONALITY 10
> >> +enum {
> >> + IORING_REGISTER_BUFFERS,
> >
> > Actually, one *tiny* thought. Since this is UAPI, do we want to be extra
> > careful here and explicitly assign values? We can't change the meaning
> > of a number (UAPI) but we can add new ones, etc? This would help if an
> > OP were removed (to stop from triggering a cascade of changed values)...
> >
> > for example:
> >
> > enum {
> > IORING_REGISTER_BUFFERS = 0,
> > IORING_UNREGISTER_BUFFERS = 1,
> > ...
>
> Definitely that is preferred, IMHO, for enums used as part of UAPI,
> as it avoids accidental changes to the values, and it also makes it
> easier to see what the actual values are.
>
Sure, I agree.
I'll put the values in the enumerations in the v5.
Thanks,
Stefano
Re: [PATCH v4 1/3] io_uring: use an enumeration for io_uring_register(2) opcodes
On Aug 26, 2020, at 1:43 PM, Kees Cook wrote:
>
> On Thu, Aug 13, 2020 at 05:32:52PM +0200, Stefano Garzarella wrote:
>> The enumeration allows us to keep track of the last
>> io_uring_register(2) opcode available.
>>
>> Behaviour and opcodes names don't change.
>>
>> Signed-off-by: Stefano Garzarella
>> ---
>> include/uapi/linux/io_uring.h | 27 ---
>> 1 file changed, 16 insertions(+), 11 deletions(-)
>>
>> diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h
>> index d65fde732518..cdc98afbacc3 100644
>> --- a/include/uapi/linux/io_uring.h
>> +++ b/include/uapi/linux/io_uring.h
>> @@ -255,17 +255,22 @@ struct io_uring_params {
>> /*
>> * io_uring_register(2) opcodes and arguments
>> */
>> -#define IORING_REGISTER_BUFFERS 0
>> -#define IORING_UNREGISTER_BUFFERS 1
>> -#define IORING_REGISTER_FILES 2
>> -#define IORING_UNREGISTER_FILES 3
>> -#define IORING_REGISTER_EVENTFD 4
>> -#define IORING_UNREGISTER_EVENTFD 5
>> -#define IORING_REGISTER_FILES_UPDATE6
>> -#define IORING_REGISTER_EVENTFD_ASYNC 7
>> -#define IORING_REGISTER_PROBE 8
>> -#define IORING_REGISTER_PERSONALITY 9
>> -#define IORING_UNREGISTER_PERSONALITY 10
>> +enum {
>> +IORING_REGISTER_BUFFERS,
>
> Actually, one *tiny* thought. Since this is UAPI, do we want to be extra
> careful here and explicitly assign values? We can't change the meaning
> of a number (UAPI) but we can add new ones, etc? This would help if an
> OP were removed (to stop from triggering a cascade of changed values)...
>
> for example:
>
> enum {
> IORING_REGISTER_BUFFERS = 0,
> IORING_UNREGISTER_BUFFERS = 1,
> ...
Definitely that is preferred, IMHO, for enums used as part of UAPI,
as it avoids accidental changes to the values, and it also makes it
easier to see what the actual values are.
Cheers, Andreas
signature.asc
Description: Message signed with OpenPGP
Re: [PATCH v4 1/3] io_uring: use an enumeration for io_uring_register(2) opcodes
On Thu, Aug 13, 2020 at 05:32:52PM +0200, Stefano Garzarella wrote:
> The enumeration allows us to keep track of the last
> io_uring_register(2) opcode available.
>
> Behaviour and opcodes names don't change.
>
> Signed-off-by: Stefano Garzarella
> ---
> include/uapi/linux/io_uring.h | 27 ---
> 1 file changed, 16 insertions(+), 11 deletions(-)
>
> diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h
> index d65fde732518..cdc98afbacc3 100644
> --- a/include/uapi/linux/io_uring.h
> +++ b/include/uapi/linux/io_uring.h
> @@ -255,17 +255,22 @@ struct io_uring_params {
> /*
> * io_uring_register(2) opcodes and arguments
> */
> -#define IORING_REGISTER_BUFFERS 0
> -#define IORING_UNREGISTER_BUFFERS1
> -#define IORING_REGISTER_FILES2
> -#define IORING_UNREGISTER_FILES 3
> -#define IORING_REGISTER_EVENTFD 4
> -#define IORING_UNREGISTER_EVENTFD5
> -#define IORING_REGISTER_FILES_UPDATE 6
> -#define IORING_REGISTER_EVENTFD_ASYNC7
> -#define IORING_REGISTER_PROBE8
> -#define IORING_REGISTER_PERSONALITY 9
> -#define IORING_UNREGISTER_PERSONALITY10
> +enum {
> + IORING_REGISTER_BUFFERS,
Actually, one *tiny* thought. Since this is UAPI, do we want to be extra
careful here and explicitly assign values? We can't change the meaning
of a number (UAPI) but we can add new ones, etc? This would help if an
OP were removed (to stop from triggering a cascade of changed values)...
for example:
enum {
IORING_REGISTER_BUFFERS = 0,
IORING_UNREGISTER_BUFFERS = 1,
...
--
Kees Cook
Re: [PATCH v4 1/3] io_uring: use an enumeration for io_uring_register(2) opcodes
On Thu, Aug 13, 2020 at 05:32:52PM +0200, Stefano Garzarella wrote: > The enumeration allows us to keep track of the last > io_uring_register(2) opcode available. > > Behaviour and opcodes names don't change. > > Signed-off-by: Stefano Garzarella Reviewed-by: Kees Cook -- Kees Cook
[PATCH v4 1/3] io_uring: use an enumeration for io_uring_register(2) opcodes
The enumeration allows us to keep track of the last
io_uring_register(2) opcode available.
Behaviour and opcodes names don't change.
Signed-off-by: Stefano Garzarella
---
include/uapi/linux/io_uring.h | 27 ---
1 file changed, 16 insertions(+), 11 deletions(-)
diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h
index d65fde732518..cdc98afbacc3 100644
--- a/include/uapi/linux/io_uring.h
+++ b/include/uapi/linux/io_uring.h
@@ -255,17 +255,22 @@ struct io_uring_params {
/*
* io_uring_register(2) opcodes and arguments
*/
-#define IORING_REGISTER_BUFFERS0
-#define IORING_UNREGISTER_BUFFERS 1
-#define IORING_REGISTER_FILES 2
-#define IORING_UNREGISTER_FILES3
-#define IORING_REGISTER_EVENTFD4
-#define IORING_UNREGISTER_EVENTFD 5
-#define IORING_REGISTER_FILES_UPDATE 6
-#define IORING_REGISTER_EVENTFD_ASYNC 7
-#define IORING_REGISTER_PROBE 8
-#define IORING_REGISTER_PERSONALITY9
-#define IORING_UNREGISTER_PERSONALITY 10
+enum {
+ IORING_REGISTER_BUFFERS,
+ IORING_UNREGISTER_BUFFERS,
+ IORING_REGISTER_FILES,
+ IORING_UNREGISTER_FILES,
+ IORING_REGISTER_EVENTFD,
+ IORING_UNREGISTER_EVENTFD,
+ IORING_REGISTER_FILES_UPDATE,
+ IORING_REGISTER_EVENTFD_ASYNC,
+ IORING_REGISTER_PROBE,
+ IORING_REGISTER_PERSONALITY,
+ IORING_UNREGISTER_PERSONALITY,
+
+ /* this goes last */
+ IORING_REGISTER_LAST
+};
struct io_uring_files_update {
__u32 offset;
--
2.26.2
