| Message ID | 20220811075638.36450-1-chunchao@nfschina.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Modify the return value ret to EOPNOTSUPP when initialized to reduce repeated assignment of errno | expand |
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. Thanks, Stefano
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 strongly, I'm fine with your patch too. Can you send it as a real patch?
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
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;
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(-)