axboe / liburing

Library providing helpers for the Linux kernel io_uring support
MIT License
2.72k stars 393 forks source link

Anonymous Enum = wrong value #1069

Open YoSTEALTH opened 5 months ago

YoSTEALTH commented 5 months ago

@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

axboe commented 5 months ago

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

YoSTEALTH commented 5 months ago

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 commented 5 months ago

@axboe ^

YoSTEALTH commented 4 months ago

@axboe reminder

krisman commented 4 months ago

@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?

YoSTEALTH commented 4 months ago

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

krisman commented 4 months ago

https://lore.kernel.org/io-uring/20240328001653.31124-1-krisman@suse.de/T/#u

YoSTEALTH commented 4 months ago

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

krisman commented 4 months ago

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.

YoSTEALTH commented 3 months ago

@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 commented 3 months ago

@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

krisman commented 3 months ago

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

https://lore.kernel.org/io-uring/20240329215718.25048-1-krisman@suse.de/T/#u

YoSTEALTH commented 3 months ago

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