Message ID | 20210409100627.451573-1-groug@kaod.org |
---|---|
State | New |
Headers | show |
Series | virtiofsd: Fix side-effect in assert() | expand |
On 4/9/21 12:06 PM, Greg Kurz wrote: > It is bad practice to put an expression with a side-effect in > assert() because the side-effect won't happen if the code is > compiled with -DNDEBUG. > > Use an intermediate variable. Consolidate this in an macro to > have proper line numbers when the assertion is hit. > > virtiofsd: ../../tools/virtiofsd/passthrough_ll.c:2797: lo_getxattr: > Assertion `fchdir_res == 0' failed. > Aborted > > 2796 /* fchdir should not fail here */ > =>2797 FCHDIR_NOFAIL(lo->proc_self_fd); > 2798 ret = getxattr(procname, name, value, size); > 2799 FCHDIR_NOFAIL(lo->root.fd); > > Fixes: bdfd66788349 ("virtiofsd: Fix xattr operations") > Cc: misono.tomohiro@jp.fujitsu.com > Signed-off-by: Greg Kurz <groug@kaod.org> > --- > tools/virtiofsd/passthrough_ll.c | 21 +++++++++++++-------- > 1 file changed, 13 insertions(+), 8 deletions(-) Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
On Fri, Apr 09, 2021 at 12:06:27PM +0200, Greg Kurz wrote: > It is bad practice to put an expression with a side-effect in > assert() because the side-effect won't happen if the code is > compiled with -DNDEBUG. > > Use an intermediate variable. Consolidate this in an macro to > have proper line numbers when the assertion is hit. > > virtiofsd: ../../tools/virtiofsd/passthrough_ll.c:2797: lo_getxattr: > Assertion `fchdir_res == 0' failed. > Aborted > > 2796 /* fchdir should not fail here */ > =>2797 FCHDIR_NOFAIL(lo->proc_self_fd); > 2798 ret = getxattr(procname, name, value, size); > 2799 FCHDIR_NOFAIL(lo->root.fd); > > Fixes: bdfd66788349 ("virtiofsd: Fix xattr operations") > Cc: misono.tomohiro@jp.fujitsu.com > Signed-off-by: Greg Kurz <groug@kaod.org> Looks good to me. Reviewed-by: Vivek Goyal <vgoyal@redhat.com> Vivek > --- > tools/virtiofsd/passthrough_ll.c | 21 +++++++++++++-------- > 1 file changed, 13 insertions(+), 8 deletions(-) > > diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c > index 1553d2ef454f..6592f96f685e 100644 > --- a/tools/virtiofsd/passthrough_ll.c > +++ b/tools/virtiofsd/passthrough_ll.c > @@ -2723,6 +2723,11 @@ static int xattr_map_server(const struct lo_data *lo, const char *server_name, > return -ENODATA; > } > > +#define FCHDIR_NOFAIL(fd) do { \ > + int fchdir_res = fchdir(fd); \ > + assert(fchdir_res == 0); \ > + } while (0) > + > static void lo_getxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name, > size_t size) > { > @@ -2789,9 +2794,9 @@ static void lo_getxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name, > ret = fgetxattr(fd, name, value, size); > } else { > /* fchdir should not fail here */ > - assert(fchdir(lo->proc_self_fd) == 0); > + FCHDIR_NOFAIL(lo->proc_self_fd); > ret = getxattr(procname, name, value, size); > - assert(fchdir(lo->root.fd) == 0); > + FCHDIR_NOFAIL(lo->root.fd); > } > > if (ret == -1) { > @@ -2864,9 +2869,9 @@ static void lo_listxattr(fuse_req_t req, fuse_ino_t ino, size_t size) > ret = flistxattr(fd, value, size); > } else { > /* fchdir should not fail here */ > - assert(fchdir(lo->proc_self_fd) == 0); > + FCHDIR_NOFAIL(lo->proc_self_fd); > ret = listxattr(procname, value, size); > - assert(fchdir(lo->root.fd) == 0); > + FCHDIR_NOFAIL(lo->root.fd); > } > > if (ret == -1) { > @@ -3000,9 +3005,9 @@ static void lo_setxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name, > ret = fsetxattr(fd, name, value, size, flags); > } else { > /* fchdir should not fail here */ > - assert(fchdir(lo->proc_self_fd) == 0); > + FCHDIR_NOFAIL(lo->proc_self_fd); > ret = setxattr(procname, name, value, size, flags); > - assert(fchdir(lo->root.fd) == 0); > + FCHDIR_NOFAIL(lo->root.fd); > } > > saverr = ret == -1 ? errno : 0; > @@ -3066,9 +3071,9 @@ static void lo_removexattr(fuse_req_t req, fuse_ino_t ino, const char *in_name) > ret = fremovexattr(fd, name); > } else { > /* fchdir should not fail here */ > - assert(fchdir(lo->proc_self_fd) == 0); > + FCHDIR_NOFAIL(lo->proc_self_fd); > ret = removexattr(procname, name); > - assert(fchdir(lo->root.fd) == 0); > + FCHDIR_NOFAIL(lo->root.fd); > } > > saverr = ret == -1 ? errno : 0; > -- > 2.26.3 > > _______________________________________________ > Virtio-fs mailing list > Virtio-fs@redhat.com > https://listman.redhat.com/mailman/listinfo/virtio-fs
On 4/9/21 7:06 AM, Greg Kurz wrote: > It is bad practice to put an expression with a side-effect in > assert() because the side-effect won't happen if the code is > compiled with -DNDEBUG. > > Use an intermediate variable. Consolidate this in an macro to > have proper line numbers when the assertion is hit. > > virtiofsd: ../../tools/virtiofsd/passthrough_ll.c:2797: lo_getxattr: > Assertion `fchdir_res == 0' failed. > Aborted > > 2796 /* fchdir should not fail here */ > =>2797 FCHDIR_NOFAIL(lo->proc_self_fd); > 2798 ret = getxattr(procname, name, value, size); > 2799 FCHDIR_NOFAIL(lo->root.fd); > > Fixes: bdfd66788349 ("virtiofsd: Fix xattr operations") > Cc: misono.tomohiro@jp.fujitsu.com > Signed-off-by: Greg Kurz <groug@kaod.org> > --- > tools/virtiofsd/passthrough_ll.c | 21 +++++++++++++-------- > 1 file changed, 13 insertions(+), 8 deletions(-) Reviewed-by: Wainer dos Santos Moschetta <wainersm@redhat.com> > > diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c > index 1553d2ef454f..6592f96f685e 100644 > --- a/tools/virtiofsd/passthrough_ll.c > +++ b/tools/virtiofsd/passthrough_ll.c > @@ -2723,6 +2723,11 @@ static int xattr_map_server(const struct lo_data *lo, const char *server_name, > return -ENODATA; > } > > +#define FCHDIR_NOFAIL(fd) do { \ > + int fchdir_res = fchdir(fd); \ > + assert(fchdir_res == 0); \ > + } while (0) > + > static void lo_getxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name, > size_t size) > { > @@ -2789,9 +2794,9 @@ static void lo_getxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name, > ret = fgetxattr(fd, name, value, size); > } else { > /* fchdir should not fail here */ > - assert(fchdir(lo->proc_self_fd) == 0); > + FCHDIR_NOFAIL(lo->proc_self_fd); > ret = getxattr(procname, name, value, size); > - assert(fchdir(lo->root.fd) == 0); > + FCHDIR_NOFAIL(lo->root.fd); > } > > if (ret == -1) { > @@ -2864,9 +2869,9 @@ static void lo_listxattr(fuse_req_t req, fuse_ino_t ino, size_t size) > ret = flistxattr(fd, value, size); > } else { > /* fchdir should not fail here */ > - assert(fchdir(lo->proc_self_fd) == 0); > + FCHDIR_NOFAIL(lo->proc_self_fd); > ret = listxattr(procname, value, size); > - assert(fchdir(lo->root.fd) == 0); > + FCHDIR_NOFAIL(lo->root.fd); > } > > if (ret == -1) { > @@ -3000,9 +3005,9 @@ static void lo_setxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name, > ret = fsetxattr(fd, name, value, size, flags); > } else { > /* fchdir should not fail here */ > - assert(fchdir(lo->proc_self_fd) == 0); > + FCHDIR_NOFAIL(lo->proc_self_fd); > ret = setxattr(procname, name, value, size, flags); > - assert(fchdir(lo->root.fd) == 0); > + FCHDIR_NOFAIL(lo->root.fd); > } > > saverr = ret == -1 ? errno : 0; > @@ -3066,9 +3071,9 @@ static void lo_removexattr(fuse_req_t req, fuse_ino_t ino, const char *in_name) > ret = fremovexattr(fd, name); > } else { > /* fchdir should not fail here */ > - assert(fchdir(lo->proc_self_fd) == 0); > + FCHDIR_NOFAIL(lo->proc_self_fd); > ret = removexattr(procname, name); > - assert(fchdir(lo->root.fd) == 0); > + FCHDIR_NOFAIL(lo->root.fd); > } > > saverr = ret == -1 ? errno : 0;
Greg Kurz <groug@kaod.org> writes: > It is bad practice to put an expression with a side-effect in > assert() because the side-effect won't happen if the code is > compiled with -DNDEBUG. > > Use an intermediate variable. Consolidate this in an macro to > have proper line numbers when the assertion is hit. > > virtiofsd: ../../tools/virtiofsd/passthrough_ll.c:2797: lo_getxattr: > Assertion `fchdir_res == 0' failed. > Aborted > > 2796 /* fchdir should not fail here */ > =>2797 FCHDIR_NOFAIL(lo->proc_self_fd); > 2798 ret = getxattr(procname, name, value, size); > 2799 FCHDIR_NOFAIL(lo->root.fd); > > Fixes: bdfd66788349 ("virtiofsd: Fix xattr operations") > Cc: misono.tomohiro@jp.fujitsu.com > Signed-off-by: Greg Kurz <groug@kaod.org> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Am 09.04.21 um 12:06 schrieb Greg Kurz: > It is bad practice to put an expression with a side-effect in > assert() because the side-effect won't happen if the code is > compiled with -DNDEBUG. > > Use an intermediate variable. Consolidate this in an macro to > have proper line numbers when the assertion is hit. > > virtiofsd: ../../tools/virtiofsd/passthrough_ll.c:2797: lo_getxattr: > Assertion `fchdir_res == 0' failed. > Aborted > > 2796 /* fchdir should not fail here */ > =>2797 FCHDIR_NOFAIL(lo->proc_self_fd); > 2798 ret = getxattr(procname, name, value, size); > 2799 FCHDIR_NOFAIL(lo->root.fd); > > Fixes: bdfd66788349 ("virtiofsd: Fix xattr operations") > Cc: misono.tomohiro@jp.fujitsu.com > Signed-off-by: Greg Kurz <groug@kaod.org> > --- > tools/virtiofsd/passthrough_ll.c | 21 +++++++++++++-------- > 1 file changed, 13 insertions(+), 8 deletions(-) > > diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c > index 1553d2ef454f..6592f96f685e 100644 > --- a/tools/virtiofsd/passthrough_ll.c > +++ b/tools/virtiofsd/passthrough_ll.c > @@ -2723,6 +2723,11 @@ static int xattr_map_server(const struct lo_data *lo, const char *server_name, > return -ENODATA; > } > > +#define FCHDIR_NOFAIL(fd) do { \ > + int fchdir_res = fchdir(fd); \ > + assert(fchdir_res == 0); \ > + } while (0) > + > static void lo_getxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name, > I am afraid that this will raise a compiler warning (or error with -Werror) when NDEBUG is defined because the variable is unused in that case ([-Wunused-variable]). I suggest to use different implementations of the macro depending on NDEBUG. Stefan
Hi Stefan, On 4/9/21 5:11 PM, Stefan Weil wrote: > Am 09.04.21 um 12:06 schrieb Greg Kurz: > >> It is bad practice to put an expression with a side-effect in >> assert() because the side-effect won't happen if the code is >> compiled with -DNDEBUG. >> >> Use an intermediate variable. Consolidate this in an macro to >> have proper line numbers when the assertion is hit. >> >> virtiofsd: ../../tools/virtiofsd/passthrough_ll.c:2797: lo_getxattr: >> Assertion `fchdir_res == 0' failed. >> Aborted >> >> 2796 /* fchdir should not fail here */ >> =>2797 FCHDIR_NOFAIL(lo->proc_self_fd); >> 2798 ret = getxattr(procname, name, value, size); >> 2799 FCHDIR_NOFAIL(lo->root.fd); >> >> Fixes: bdfd66788349 ("virtiofsd: Fix xattr operations") >> Cc: misono.tomohiro@jp.fujitsu.com >> Signed-off-by: Greg Kurz <groug@kaod.org> >> --- >> tools/virtiofsd/passthrough_ll.c | 21 +++++++++++++-------- >> 1 file changed, 13 insertions(+), 8 deletions(-) >> >> diff --git a/tools/virtiofsd/passthrough_ll.c >> b/tools/virtiofsd/passthrough_ll.c >> index 1553d2ef454f..6592f96f685e 100644 >> --- a/tools/virtiofsd/passthrough_ll.c >> +++ b/tools/virtiofsd/passthrough_ll.c >> @@ -2723,6 +2723,11 @@ static int xattr_map_server(const struct lo_data > *lo, const char *server_name, >> return -ENODATA; >> } >> +#define FCHDIR_NOFAIL(fd) do { \ >> + int fchdir_res = fchdir(fd); \ >> + assert(fchdir_res == 0); \ >> + } while (0) >> + >> static void lo_getxattr(fuse_req_t req, fuse_ino_t ino, const char >> *in_name, >> > > > I am afraid that this will raise a compiler warning (or error with > -Werror) when NDEBUG is defined because the variable is unused in that > case ([-Wunused-variable]). > > I suggest to use different implementations of the macro depending on > NDEBUG. QEMU doesn't build with NDEBUG, see commit 262a69f4282 ("osdep.h: Prohibit disabling assert() in supported builds"). Regards, Phil.
* Greg Kurz (groug@kaod.org) wrote: > It is bad practice to put an expression with a side-effect in > assert() because the side-effect won't happen if the code is > compiled with -DNDEBUG. > > Use an intermediate variable. Consolidate this in an macro to > have proper line numbers when the assertion is hit. > > virtiofsd: ../../tools/virtiofsd/passthrough_ll.c:2797: lo_getxattr: > Assertion `fchdir_res == 0' failed. > Aborted > > 2796 /* fchdir should not fail here */ > =>2797 FCHDIR_NOFAIL(lo->proc_self_fd); > 2798 ret = getxattr(procname, name, value, size); > 2799 FCHDIR_NOFAIL(lo->root.fd); > > Fixes: bdfd66788349 ("virtiofsd: Fix xattr operations") > Cc: misono.tomohiro@jp.fujitsu.com > Signed-off-by: Greg Kurz <groug@kaod.org> Queued > --- > tools/virtiofsd/passthrough_ll.c | 21 +++++++++++++-------- > 1 file changed, 13 insertions(+), 8 deletions(-) > > diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c > index 1553d2ef454f..6592f96f685e 100644 > --- a/tools/virtiofsd/passthrough_ll.c > +++ b/tools/virtiofsd/passthrough_ll.c > @@ -2723,6 +2723,11 @@ static int xattr_map_server(const struct lo_data *lo, const char *server_name, > return -ENODATA; > } > > +#define FCHDIR_NOFAIL(fd) do { \ > + int fchdir_res = fchdir(fd); \ > + assert(fchdir_res == 0); \ > + } while (0) > + > static void lo_getxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name, > size_t size) > { > @@ -2789,9 +2794,9 @@ static void lo_getxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name, > ret = fgetxattr(fd, name, value, size); > } else { > /* fchdir should not fail here */ > - assert(fchdir(lo->proc_self_fd) == 0); > + FCHDIR_NOFAIL(lo->proc_self_fd); > ret = getxattr(procname, name, value, size); > - assert(fchdir(lo->root.fd) == 0); > + FCHDIR_NOFAIL(lo->root.fd); > } > > if (ret == -1) { > @@ -2864,9 +2869,9 @@ static void lo_listxattr(fuse_req_t req, fuse_ino_t ino, size_t size) > ret = flistxattr(fd, value, size); > } else { > /* fchdir should not fail here */ > - assert(fchdir(lo->proc_self_fd) == 0); > + FCHDIR_NOFAIL(lo->proc_self_fd); > ret = listxattr(procname, value, size); > - assert(fchdir(lo->root.fd) == 0); > + FCHDIR_NOFAIL(lo->root.fd); > } > > if (ret == -1) { > @@ -3000,9 +3005,9 @@ static void lo_setxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name, > ret = fsetxattr(fd, name, value, size, flags); > } else { > /* fchdir should not fail here */ > - assert(fchdir(lo->proc_self_fd) == 0); > + FCHDIR_NOFAIL(lo->proc_self_fd); > ret = setxattr(procname, name, value, size, flags); > - assert(fchdir(lo->root.fd) == 0); > + FCHDIR_NOFAIL(lo->root.fd); > } > > saverr = ret == -1 ? errno : 0; > @@ -3066,9 +3071,9 @@ static void lo_removexattr(fuse_req_t req, fuse_ino_t ino, const char *in_name) > ret = fremovexattr(fd, name); > } else { > /* fchdir should not fail here */ > - assert(fchdir(lo->proc_self_fd) == 0); > + FCHDIR_NOFAIL(lo->proc_self_fd); > ret = removexattr(procname, name); > - assert(fchdir(lo->root.fd) == 0); > + FCHDIR_NOFAIL(lo->root.fd); > } > > saverr = ret == -1 ? errno : 0; > -- > 2.26.3 > >
diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c index 1553d2ef454f..6592f96f685e 100644 --- a/tools/virtiofsd/passthrough_ll.c +++ b/tools/virtiofsd/passthrough_ll.c @@ -2723,6 +2723,11 @@ static int xattr_map_server(const struct lo_data *lo, const char *server_name, return -ENODATA; } +#define FCHDIR_NOFAIL(fd) do { \ + int fchdir_res = fchdir(fd); \ + assert(fchdir_res == 0); \ + } while (0) + static void lo_getxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name, size_t size) { @@ -2789,9 +2794,9 @@ static void lo_getxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name, ret = fgetxattr(fd, name, value, size); } else { /* fchdir should not fail here */ - assert(fchdir(lo->proc_self_fd) == 0); + FCHDIR_NOFAIL(lo->proc_self_fd); ret = getxattr(procname, name, value, size); - assert(fchdir(lo->root.fd) == 0); + FCHDIR_NOFAIL(lo->root.fd); } if (ret == -1) { @@ -2864,9 +2869,9 @@ static void lo_listxattr(fuse_req_t req, fuse_ino_t ino, size_t size) ret = flistxattr(fd, value, size); } else { /* fchdir should not fail here */ - assert(fchdir(lo->proc_self_fd) == 0); + FCHDIR_NOFAIL(lo->proc_self_fd); ret = listxattr(procname, value, size); - assert(fchdir(lo->root.fd) == 0); + FCHDIR_NOFAIL(lo->root.fd); } if (ret == -1) { @@ -3000,9 +3005,9 @@ static void lo_setxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name, ret = fsetxattr(fd, name, value, size, flags); } else { /* fchdir should not fail here */ - assert(fchdir(lo->proc_self_fd) == 0); + FCHDIR_NOFAIL(lo->proc_self_fd); ret = setxattr(procname, name, value, size, flags); - assert(fchdir(lo->root.fd) == 0); + FCHDIR_NOFAIL(lo->root.fd); } saverr = ret == -1 ? errno : 0; @@ -3066,9 +3071,9 @@ static void lo_removexattr(fuse_req_t req, fuse_ino_t ino, const char *in_name) ret = fremovexattr(fd, name); } else { /* fchdir should not fail here */ - assert(fchdir(lo->proc_self_fd) == 0); + FCHDIR_NOFAIL(lo->proc_self_fd); ret = removexattr(procname, name); - assert(fchdir(lo->root.fd) == 0); + FCHDIR_NOFAIL(lo->root.fd); } saverr = ret == -1 ? errno : 0;
It is bad practice to put an expression with a side-effect in assert() because the side-effect won't happen if the code is compiled with -DNDEBUG. Use an intermediate variable. Consolidate this in an macro to have proper line numbers when the assertion is hit. virtiofsd: ../../tools/virtiofsd/passthrough_ll.c:2797: lo_getxattr: Assertion `fchdir_res == 0' failed. Aborted 2796 /* fchdir should not fail here */ =>2797 FCHDIR_NOFAIL(lo->proc_self_fd); 2798 ret = getxattr(procname, name, value, size); 2799 FCHDIR_NOFAIL(lo->root.fd); Fixes: bdfd66788349 ("virtiofsd: Fix xattr operations") Cc: misono.tomohiro@jp.fujitsu.com Signed-off-by: Greg Kurz <groug@kaod.org> --- tools/virtiofsd/passthrough_ll.c | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-)