Re: [PATCH 1/3] io_uring: add io_uring_get_opcode

[Jens Axboe <axboe@kernel.dk>]

On 4/25/22 7:21 AM, Dylan Yudaken wrote:
> On Mon, 2022-04-25 at 06:38 -0600, Jens Axboe wrote:
>> On 4/25/22 3:36 AM, Dylan Yudaken wrote:
>>> In some debug scenarios it is useful to have the text representation
>>> of
>>> the opcode. Add this function in preparation.
>>>
>>> Signed-off-by: Dylan Yudaken <dylany@fb.com>
>>> ---
>>>  fs/io_uring.c            | 91
>>> ++++++++++++++++++++++++++++++++++++++++
>>>  include/linux/io_uring.h |  5 +++
>>>  2 files changed, 96 insertions(+)
>>>
>>> diff --git a/fs/io_uring.c b/fs/io_uring.c
>>> index e57d47a23682..326695f74b93 100644
>>> --- a/fs/io_uring.c
>>> +++ b/fs/io_uring.c
>>> @@ -1255,6 +1255,97 @@ static struct kmem_cache *req_cachep;
>>>  
>>>  static const struct file_operations io_uring_fops;
>>>  
>>> +const char *io_uring_get_opcode(u8 opcode)
>>> +{
>>> +       switch (opcode) {
>>> +       case IORING_OP_NOP:
>>> +               return "NOP";
>>> +       case IORING_OP_READV:
>>> +               return "READV";
>>> +       case IORING_OP_WRITEV:
>>> +               return "WRITEV";
>>> +       case IORING_OP_FSYNC:
>>> +               return "FSYNC";
>>> +       case IORING_OP_READ_FIXED:
>>> +               return "READ_FIXED";
>>> +       case IORING_OP_WRITE_FIXED:
>>> +               return "WRITE_FIXED";
>>> +       case IORING_OP_POLL_ADD:
>>> +               return "POLL_ADD";
>>> +       case IORING_OP_POLL_REMOVE:
>>> +               return "POLL_REMOVE";
>>> +       case IORING_OP_SYNC_FILE_RANGE:
>>> +               return "SYNC_FILE_RANGE";
>>> +       case IORING_OP_SENDMSG:
>>> +               return "SENDMSG";
>>> +       case IORING_OP_RECVMSG:
>>> +               return "RECVMSG";
>>> +       case IORING_OP_TIMEOUT:
>>> +               return "TIMEOUT";
>>> +       case IORING_OP_TIMEOUT_REMOVE:
>>> +               return "TIMEOUT_REMOVE";
>>> +       case IORING_OP_ACCEPT:
>>> +               return "ACCEPT";
>>> +       case IORING_OP_ASYNC_CANCEL:
>>> +               return "ASYNC_CANCEL";
>>> +       case IORING_OP_LINK_TIMEOUT:
>>> +               return "LINK_TIMEOUT";
>>> +       case IORING_OP_CONNECT:
>>> +               return "CONNECT";
>>> +       case IORING_OP_FALLOCATE:
>>> +               return "FALLOCATE";
>>> +       case IORING_OP_OPENAT:
>>> +               return "OPENAT";
>>> +       case IORING_OP_CLOSE:
>>> +               return "CLOSE";
>>> +       case IORING_OP_FILES_UPDATE:
>>> +               return "FILES_UPDATE";
>>> +       case IORING_OP_STATX:
>>> +               return "STATX";
>>> +       case IORING_OP_READ:
>>> +               return "READ";
>>> +       case IORING_OP_WRITE:
>>> +               return "WRITE";
>>> +       case IORING_OP_FADVISE:
>>> +               return "FADVISE";
>>> +       case IORING_OP_MADVISE:
>>> +               return "MADVISE";
>>> +       case IORING_OP_SEND:
>>> +               return "SEND";
>>> +       case IORING_OP_RECV:
>>> +               return "RECV";
>>> +       case IORING_OP_OPENAT2:
>>> +               return "OPENAT2";
>>> +       case IORING_OP_EPOLL_CTL:
>>> +               return "EPOLL_CTL";
>>> +       case IORING_OP_SPLICE:
>>> +               return "SPLICE";
>>> +       case IORING_OP_PROVIDE_BUFFERS:
>>> +               return "PROVIDE_BUFFERS";
>>> +       case IORING_OP_REMOVE_BUFFERS:
>>> +               return "REMOVE_BUFFERS";
>>> +       case IORING_OP_TEE:
>>> +               return "TEE";
>>> +       case IORING_OP_SHUTDOWN:
>>> +               return "SHUTDOWN";
>>> +       case IORING_OP_RENAMEAT:
>>> +               return "RENAMEAT";
>>> +       case IORING_OP_UNLINKAT:
>>> +               return "UNLINKAT";
>>> +       case IORING_OP_MKDIRAT:
>>> +               return "MKDIRAT";
>>> +       case IORING_OP_SYMLINKAT:
>>> +               return "SYMLINKAT";
>>> +       case IORING_OP_LINKAT:
>>> +               return "LINKAT";
>>> +       case IORING_OP_MSG_RING:
>>> +               return "MSG_RING";
>>> +       case IORING_OP_LAST:
>>> +               return "LAST";
>>> +       }
>>> +       return "UNKNOWN";
>>> +}
>>
>> My only worry here is that it's another place to touch when adding an
>> opcode. I'm assuming the compiler doesn't warn if you're missing one
>> since it's not strongly typed?
> 
> It doesn't complain, but we could strongly type it to get it to? I
> don't think it will break anything (certainly does not locally). What
> about something like this:

I think this would be fine. Would probably be cleaner if you just make
io_uring_get_opcode() take an enum io_uring_op and just fwd declare it
in io_uring.h?

-- 
Jens Axboe