tools/virtiofsd/passthrough_ll.c | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-)
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,
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
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
* 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 > > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
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> 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> -- Alex Bennée
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;
© 2016 - 2024 Red Hat, Inc.