Re: [PATCH] virtiofsd: Fix side-effect in assert()
* Greg Kurz (gr...@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.tomoh...@jp.fujitsu.com > Signed-off-by: Greg Kurz 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 > > -- Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
Re: [PATCH] virtiofsd: Fix side-effect in assert()
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.tomoh...@jp.fujitsu.com >> Signed-off-by: Greg Kurz >> --- >> 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.
Re: [PATCH] virtiofsd: Fix side-effect in assert()
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.tomoh...@jp.fujitsu.com Signed-off-by: Greg Kurz --- 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
Re: [PATCH] virtiofsd: Fix side-effect in assert()
Greg Kurz 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.tomoh...@jp.fujitsu.com > Signed-off-by: Greg Kurz Reviewed-by: Alex Bennée -- Alex Bennée
Re: [PATCH] virtiofsd: Fix side-effect in assert()
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.tomoh...@jp.fujitsu.com Signed-off-by: Greg Kurz --- tools/virtiofsd/passthrough_ll.c | 21 + 1 file changed, 13 insertions(+), 8 deletions(-) Reviewed-by: Wainer dos Santos Moschetta 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;
Re: [Virtio-fs] [PATCH] virtiofsd: Fix side-effect in assert()
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.tomoh...@jp.fujitsu.com > Signed-off-by: Greg Kurz Looks good to me. Reviewed-by: Vivek Goyal 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...@redhat.com > https://listman.redhat.com/mailman/listinfo/virtio-fs
Re: [PATCH] virtiofsd: Fix side-effect in assert()
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.tomoh...@jp.fujitsu.com > Signed-off-by: Greg Kurz > --- > tools/virtiofsd/passthrough_ll.c | 21 + > 1 file changed, 13 insertions(+), 8 deletions(-) Reviewed-by: Philippe Mathieu-Daudé
[PATCH] virtiofsd: Fix side-effect in assert()
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.tomoh...@jp.fujitsu.com Signed-off-by: Greg Kurz --- 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