Messages in this thread | | | Date | Thu, 11 Aug 2022 20:42:22 +0200 | From | Stefano Garzarella <> | Subject | Re: [PATCH] Modify the return value ret to EOPNOTSUPP when initialized to reduce repeated assignment of errno |
| |
On Thu, Aug 11, 2022 at 09:41:38AM -0600, Jens Axboe wrote: >On 8/11/22 9:02 AM, Stefano Garzarella wrote: >> On Thu, Aug 11, 2022 at 03:56:38PM +0800, Zhang chunchao wrote: >>> Remove unnecessary initialization assignments. >>> >>> Signed-off-by: Zhang chunchao <chunchao@nfschina.com> >>> --- >>> io_uring/io_uring.c | 3 +-- >>> 1 file changed, 1 insertion(+), 2 deletions(-) >>> >>> diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c >>> index b54218da075c..8c267af06401 100644 >>> --- a/io_uring/io_uring.c >>> +++ b/io_uring/io_uring.c >>> @@ -3859,14 +3859,13 @@ SYSCALL_DEFINE4(io_uring_register, unsigned int, fd, unsigned int, opcode, >>> void __user *, arg, unsigned int, nr_args) >>> { >>> struct io_ring_ctx *ctx; >>> - long ret = -EBADF; >>> + long ret = -EOPNOTSUPP; >>> struct fd f; >>> >>> f = fdget(fd); >>> if (!f.file) >>> return -EBADF; >>> >>> - ret = -EOPNOTSUPP; >>> if (!io_is_uring_fops(f.file)) >>> goto out_fput; >>> >> >> What about remove the initialization and assign it in the if branch? >> I find it a bit easier to read. >> >> I mean something like this: >> >> --- a/io_uring/io_uring.c >> +++ b/io_uring/io_uring.c >> @@ -3859,16 +3859,17 @@ SYSCALL_DEFINE4(io_uring_register, unsigned int, fd, unsigned int, opcode, >> void __user *, arg, unsigned int, nr_args) >> { >> struct io_ring_ctx *ctx; >> - long ret = -EBADF; >> + long ret; >> struct fd f; >> >> f = fdget(fd); >> if (!f.file) >> return -EBADF; >> >> - ret = -EOPNOTSUPP; >> - if (!io_is_uring_fops(f.file)) >> + if (!io_is_uring_fops(f.file)) { >> + ret = -EOPNOTSUPP; >> goto out_fput; >> + } >> >> ctx = f.file->private_data; >> >> >> Otherwise remove the initialization, but leave the assignment as it is now. > >Generally the kernel likes to do: > >err = -EFOO; >if (something) > goto err_out; > >rather than put it inside the if clause. I guess the rationale is it >makes it harder to forget to init the error value. I don't feel too
ah, thanks for pointing this out! Make sense to me, but I hope recent compilers can spot that kind of issue :-)
>strongly, I'm fine with your patch too. Can you send it as a real patch?
@Zhang: if you want, feel free to change your patch following the suggestions and send a new version, otherwise I can send mine of course.
Thanks, Stefano
| |