| Message ID | 20200106182425.20312-45-danielhb413@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | trivial unneeded labels cleanup | expand |
On Montag, 6. Januar 2020 19:24:10 CET Daniel Henrique Barboza wrote: > 'err_out' can be replaced by 'return ret' in the error conditions > the jump was being made. > > CC: Greg Kurz <groug@kaod.org> > Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> > --- > hw/9pfs/9p-local.c | 12 +++++------- > 1 file changed, 5 insertions(+), 7 deletions(-) > > diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c > index ca641390fb..f9bdd2ad7c 100644 > --- a/hw/9pfs/9p-local.c > +++ b/hw/9pfs/9p-local.c > @@ -1094,12 +1094,12 @@ static int local_unlinkat_common(FsContext *ctx, int > dirfd, const char *name, > > fd = openat_dir(dirfd, name); > if (fd == -1) { > - goto err_out; > + return ret; > } > ret = unlinkat(fd, VIRTFS_META_DIR, AT_REMOVEDIR); > close_preserve_errno(fd); > if (ret < 0 && errno != ENOENT) { > - goto err_out; > + return ret; > } > } > map_dirfd = openat_dir(dirfd, VIRTFS_META_DIR); > @@ -1107,16 +1107,14 @@ static int local_unlinkat_common(FsContext *ctx, int > dirfd, const char *name, ret = unlinkat(map_dirfd, name, 0); > close_preserve_errno(map_dirfd); > if (ret < 0 && errno != ENOENT) { > - goto err_out; > + return ret; > } > } else if (errno != ENOENT) { > - goto err_out; > + return ret; > } > } > > - ret = unlinkat(dirfd, name, flags); > -err_out: > - return ret; > + return unlinkat(dirfd, name, flags); > } > > static int local_remove(FsContext *ctx, const char *path) Well, personally I don't see any improvement by these changes. It probably makes the code slightly more elegant, but IMO not more readable. And return constructed functions vs. jump to label constructed functions are more likely to gather missing-cleanup bugs. At least this patch does not cause any behaviour change, so I leave that up to you Greg to decide. ;-) Best regards, Christian Schoenebeck
On Mon, 6 Jan 2020 15:24:10 -0300 Daniel Henrique Barboza <danielhb413@gmail.com> wrote: > 'err_out' can be replaced by 'return ret' in the error conditions > the jump was being made. > > CC: Greg Kurz <groug@kaod.org> > Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> > --- > hw/9pfs/9p-local.c | 12 +++++------- > 1 file changed, 5 insertions(+), 7 deletions(-) > > diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c > index ca641390fb..f9bdd2ad7c 100644 > --- a/hw/9pfs/9p-local.c > +++ b/hw/9pfs/9p-local.c > @@ -1094,12 +1094,12 @@ static int local_unlinkat_common(FsContext *ctx, int dirfd, const char *name, > > fd = openat_dir(dirfd, name); > if (fd == -1) { > - goto err_out; > + return ret; > } > ret = unlinkat(fd, VIRTFS_META_DIR, AT_REMOVEDIR); > close_preserve_errno(fd); > if (ret < 0 && errno != ENOENT) { > - goto err_out; > + return ret; > } > } > map_dirfd = openat_dir(dirfd, VIRTFS_META_DIR); > @@ -1107,16 +1107,14 @@ static int local_unlinkat_common(FsContext *ctx, int dirfd, const char *name, > ret = unlinkat(map_dirfd, name, 0); > close_preserve_errno(map_dirfd); > if (ret < 0 && errno != ENOENT) { > - goto err_out; > + return ret; > } > } else if (errno != ENOENT) { > - goto err_out; > + return ret; Ouch... I now realize we can get there with ret == 0 when unlinking a directory in mapped-file mode. The function will wrongly return success despite the failure.... Since this function is supposed to return -1 on error, I suggest to do that instead of return ret, and to drop the initialization of ret to -1, which wouldn't be needed anymore. Since this would fix a bug it makes sense to post it separately from this series. Rewrite the title/changelog accordingly and I'll merge it via the 9p tree. > } > } > > - ret = unlinkat(dirfd, name, flags); > -err_out: > - return ret; > + return unlinkat(dirfd, name, flags); > } > > static int local_remove(FsContext *ctx, const char *path)
On Tue, 07 Jan 2020 13:04:20 +0100 Christian Schoenebeck <qemu_oss@crudebyte.com> wrote: > On Montag, 6. Januar 2020 19:24:10 CET Daniel Henrique Barboza wrote: > > 'err_out' can be replaced by 'return ret' in the error conditions > > the jump was being made. > > > > CC: Greg Kurz <groug@kaod.org> > > Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> > > --- > > hw/9pfs/9p-local.c | 12 +++++------- > > 1 file changed, 5 insertions(+), 7 deletions(-) > > > > diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c > > index ca641390fb..f9bdd2ad7c 100644 > > --- a/hw/9pfs/9p-local.c > > +++ b/hw/9pfs/9p-local.c > > @@ -1094,12 +1094,12 @@ static int local_unlinkat_common(FsContext *ctx, int > > dirfd, const char *name, > > > > fd = openat_dir(dirfd, name); > > if (fd == -1) { > > - goto err_out; > > + return ret; > > } > > ret = unlinkat(fd, VIRTFS_META_DIR, AT_REMOVEDIR); > > close_preserve_errno(fd); > > if (ret < 0 && errno != ENOENT) { > > - goto err_out; > > + return ret; > > } > > } > > map_dirfd = openat_dir(dirfd, VIRTFS_META_DIR); > > @@ -1107,16 +1107,14 @@ static int local_unlinkat_common(FsContext *ctx, int > > dirfd, const char *name, ret = unlinkat(map_dirfd, name, 0); > > close_preserve_errno(map_dirfd); > > if (ret < 0 && errno != ENOENT) { > > - goto err_out; > > + return ret; > > } > > } else if (errno != ENOENT) { > > - goto err_out; > > + return ret; > > } > > } > > > > - ret = unlinkat(dirfd, name, flags); > > -err_out: > > - return ret; > > + return unlinkat(dirfd, name, flags); > > } > > > > static int local_remove(FsContext *ctx, const char *path) > > Well, personally I don't see any improvement by these changes. It probably > makes the code slightly more elegant, but IMO not more readable. And return > constructed functions vs. jump to label constructed functions are more likely > to gather missing-cleanup bugs. > > At least this patch does not cause any behaviour change, so I leave that up to > you Greg to decide. ;-) > I don't care that much but in the present case, the function has a bug that can be fixed with a variant of this patch if 'goto err_out' is turned into 'return -1'. I've hence asked Daniel to move this patch out of the series and to turn it into a real fix that I'll merge directly. > Best regards, > Christian Schoenebeck > >
On 1/7/20 10:53 AM, Greg Kurz wrote: > On Mon, 6 Jan 2020 15:24:10 -0300 > Daniel Henrique Barboza <danielhb413@gmail.com> wrote: > >> 'err_out' can be replaced by 'return ret' in the error conditions >> the jump was being made. >> >> CC: Greg Kurz <groug@kaod.org> >> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> >> --- >> hw/9pfs/9p-local.c | 12 +++++------- >> 1 file changed, 5 insertions(+), 7 deletions(-) >> >> diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c >> index ca641390fb..f9bdd2ad7c 100644 >> --- a/hw/9pfs/9p-local.c >> +++ b/hw/9pfs/9p-local.c >> @@ -1094,12 +1094,12 @@ static int local_unlinkat_common(FsContext *ctx, int dirfd, const char *name, >> >> fd = openat_dir(dirfd, name); >> if (fd == -1) { >> - goto err_out; >> + return ret; >> } >> ret = unlinkat(fd, VIRTFS_META_DIR, AT_REMOVEDIR); >> close_preserve_errno(fd); >> if (ret < 0 && errno != ENOENT) { >> - goto err_out; >> + return ret; >> } >> } >> map_dirfd = openat_dir(dirfd, VIRTFS_META_DIR); >> @@ -1107,16 +1107,14 @@ static int local_unlinkat_common(FsContext *ctx, int dirfd, const char *name, >> ret = unlinkat(map_dirfd, name, 0); >> close_preserve_errno(map_dirfd); >> if (ret < 0 && errno != ENOENT) { >> - goto err_out; >> + return ret; >> } >> } else if (errno != ENOENT) { >> - goto err_out; >> + return ret; > > Ouch... I now realize we can get there with ret == 0 when unlinking a > directory in mapped-file mode. The function will wrongly return success > despite the failure.... Since this function is supposed to return -1 > on error, I suggest to do that instead of return ret, and to drop the > initialization of ret to -1, which wouldn't be needed anymore. > > Since this would fix a bug it makes sense to post it separately from > this series. Rewrite the title/changelog accordingly and I'll merge > it via the 9p tree. Got it. I'll repost it separately with an updated title and commit msg. Thanks, Daniel > >> } >> } >> >> - ret = unlinkat(dirfd, name, flags); >> -err_out: >> - return ret; >> + return unlinkat(dirfd, name, flags); >> } >> >> static int local_remove(FsContext *ctx, const char *path) >
diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c index ca641390fb..f9bdd2ad7c 100644 --- a/hw/9pfs/9p-local.c +++ b/hw/9pfs/9p-local.c @@ -1094,12 +1094,12 @@ static int local_unlinkat_common(FsContext *ctx, int dirfd, const char *name, fd = openat_dir(dirfd, name); if (fd == -1) { - goto err_out; + return ret; } ret = unlinkat(fd, VIRTFS_META_DIR, AT_REMOVEDIR); close_preserve_errno(fd); if (ret < 0 && errno != ENOENT) { - goto err_out; + return ret; } } map_dirfd = openat_dir(dirfd, VIRTFS_META_DIR); @@ -1107,16 +1107,14 @@ static int local_unlinkat_common(FsContext *ctx, int dirfd, const char *name, ret = unlinkat(map_dirfd, name, 0); close_preserve_errno(map_dirfd); if (ret < 0 && errno != ENOENT) { - goto err_out; + return ret; } } else if (errno != ENOENT) { - goto err_out; + return ret; } } - ret = unlinkat(dirfd, name, flags); -err_out: - return ret; + return unlinkat(dirfd, name, flags); } static int local_remove(FsContext *ctx, const char *path)
'err_out' can be replaced by 'return ret' in the error conditions the jump was being made. CC: Greg Kurz <groug@kaod.org> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- hw/9pfs/9p-local.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-)