liburing icon indicating copy to clipboard operation
liburing copied to clipboard

Anonymous Enum = wrong value

Open YoSTEALTH opened this issue 1 year ago • 13 comments

@axboe Would you mind giving an name to enum? There is a problem with anonymous enum type that has values > int providing wrong value. e.g. IORING_REGISTER_USE_REGISTERED_RING returns -2147483648 vs 2147483648

Read more: https://github.com/cython/cython/issues/3240#issuecomment-1954917584

Edit: something like enum io_uring_register_op { for https://github.com/axboe/liburing/blob/master/src/include/liburing/io_uring.h#L524

YoSTEALTH avatar Feb 20 '24 23:02 YoSTEALTH

Sure, we can certainly do that. Remind me next week if I don't get to it, gone this week.

axboe avatar Feb 21 '24 14:02 axboe

Thanks, trying to make things easier for you :)


  UNSTAGED CHANGES

--
 src/include/liburing/io_uring.h | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/src/include/liburing/io_uring.h b/src/include/liburing/io_uring.h
index bde1199..e60bc35 100644
--- a/src/include/liburing/io_uring.h
+++ b/src/include/liburing/io_uring.h
@@ -115,7 +115,7 @@ struct io_uring_sqe {
  */
 #define IORING_FILE_INDEX_ALLOC		(~0U)
 
-enum {
+enum __io_uring_bit_op {
 	IOSQE_FIXED_FILE_BIT,
 	IOSQE_IO_DRAIN_BIT,
 	IOSQE_IO_LINK_BIT,
@@ -369,7 +369,7 @@ enum io_uring_op {
 /*
  * IORING_OP_MSG_RING command types, stored in sqe->addr
  */
-enum {
+enum io_uring_msg_op {
 	IORING_MSG_DATA,	/* pass sqe->len as 'res' and off as user_data */
 	IORING_MSG_SEND_FD,	/* send a registered fd to another ring */
 };
@@ -420,7 +420,7 @@ struct io_uring_cqe {
 #define IORING_CQE_F_SOCK_NONEMPTY	(1U << 2)
 #define IORING_CQE_F_NOTIF		(1U << 3)
 
-enum {
+enum io_uring_cqe_op {
 	IORING_CQE_BUFFER_SHIFT		= 16,
 };
 
@@ -521,7 +521,7 @@ struct io_uring_params {
 /*
  * io_uring_register(2) opcodes and arguments
  */
-enum {
+enum io_uring_register_op {
 	IORING_REGISTER_BUFFERS			= 0,
 	IORING_UNREGISTER_BUFFERS		= 1,
 	IORING_REGISTER_FILES			= 2,
@@ -578,7 +578,7 @@ enum {
 };
 
 /* io-wq worker categories */
-enum {
+enum io_uring_wq_op {
 	IO_WQ_BOUND,
 	IO_WQ_UNBOUND,
 };
@@ -683,7 +683,7 @@ struct io_uring_buf_ring {
  *			IORING_OFF_PBUF_RING | (bgid << IORING_OFF_PBUF_SHIFT)
  *			to get a virtual mapping for the ring.
  */
-enum {
+enum io_uring_buf_op {
 	IOU_PBUF_RING_MMAP	= 1,
 };
 
@@ -714,7 +714,7 @@ struct io_uring_napi {
 /*
  * io_uring_restriction->opcode values
  */
-enum {
+enum io_uring_restriction_op {
 	/* Allow an io_uring_register(2) opcode */
 	IORING_RESTRICTION_REGISTER_OP		= 0,
 
@@ -768,7 +768,7 @@ struct io_uring_recvmsg_out {
 /*
  * Argument for IORING_OP_URING_CMD when file is a socket
  */
-enum {
+enum io_uring_socket_op {
 	SOCKET_URING_OP_SIOCINQ		= 0,
 	SOCKET_URING_OP_SIOCOUTQ,
 	SOCKET_URING_OP_GETSOCKOPT,

YoSTEALTH avatar Feb 23 '24 11:02 YoSTEALTH

@axboe ^

YoSTEALTH avatar Feb 27 '24 21:02 YoSTEALTH

@axboe reminder

YoSTEALTH avatar Mar 26 '24 20:03 YoSTEALTH

@YoSTEALTH i'm gonna turn this into a patch and send to the list. You wanna send me your name to add a suggested-by tag?

krisman avatar Mar 27 '24 22:03 krisman

@krisman sure, its "Ritesh", but you can just use your name (doesn't matter to me) since you are added the patch, Thanks ;)

YoSTEALTH avatar Mar 27 '24 23:03 YoSTEALTH

https://lore.kernel.org/io-uring/[email protected]/T/#u

krisman avatar Mar 28 '24 00:03 krisman

Is this meant to be inside enum? https://github.com/axboe/liburing/blob/master/src/include/liburing/io_uring.h#L577 seem odd its after IORING_REGISTER_LAST

YoSTEALTH avatar Mar 28 '24 02:03 YoSTEALTH

It is in the enum to make it clear it is used in the same field with the rest of the enum. It is after _LAST because it is a flag modifying the register operation, and not an operation itself. It is a bit ugly, but not wrong or harmful.

krisman avatar Mar 28 '24 15:03 krisman

@axboe I am waiting on this patch to merge so I can pre-release the project I been working on. It uses this library as submodule!

YoSTEALTH avatar Apr 02 '24 22:04 YoSTEALTH

@krisman actually there an error with enum io_uring_register_restrictions { as there is a function that already exists with the same name https://github.com/axboe/liburing/blob/master/src/include/liburing.h#L218

edit: could enum io_uring_register_restrictions be changed to enum io_uring_restriction_op

YoSTEALTH avatar Apr 03 '24 02:04 YoSTEALTH

yes. I had it fixed when pushing the kernel patch and the v2 has it synced

https://lore.kernel.org/io-uring/[email protected]/T/#u

krisman avatar Apr 03 '24 16:04 krisman

Thanks @krisman,

It might be better to change enum io_uring_register_restriction_op to enum io_uring_restriction_op as its not register specific but more to do with restriction https://github.com/axboe/liburing/blob/master/src/include/liburing/io_uring.h#L714-L731

YoSTEALTH avatar Apr 03 '24 19:04 YoSTEALTH