* Re: [PATCH] virtiofsd: Fix side-effect in assert()
2021-04-09 10:06 [PATCH] virtiofsd: Fix side-effect in assert() Greg Kurz
@ 2021-04-09 10:23 ` Philippe Mathieu-Daudé
2021-04-09 13:30 ` [Virtio-fs] " Vivek Goyal
` (4 subsequent siblings)
5 siblings, 0 replies; 8+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-04-09 10:23 UTC (permalink / raw)
To: Greg Kurz, qemu-devel
Cc: virtio-fs, misono.tomohiro, Stefan Hajnoczi,
Dr. David Alan Gilbert
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>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Virtio-fs] [PATCH] virtiofsd: Fix side-effect in assert()
2021-04-09 10:06 [PATCH] virtiofsd: Fix side-effect in assert() Greg Kurz
2021-04-09 10:23 ` Philippe Mathieu-Daudé
@ 2021-04-09 13:30 ` Vivek Goyal
2021-04-09 14:40 ` Wainer dos Santos Moschetta
` (3 subsequent siblings)
5 siblings, 0 replies; 8+ messages in thread
From: Vivek Goyal @ 2021-04-09 13:30 UTC (permalink / raw)
To: Greg Kurz; +Cc: virtio-fs, qemu-devel
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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] virtiofsd: Fix side-effect in assert()
2021-04-09 10:06 [PATCH] virtiofsd: Fix side-effect in assert() Greg Kurz
2021-04-09 10:23 ` Philippe Mathieu-Daudé
2021-04-09 13:30 ` [Virtio-fs] " Vivek Goyal
@ 2021-04-09 14:40 ` Wainer dos Santos Moschetta
2021-04-09 14:56 ` Alex Bennée
` (2 subsequent siblings)
5 siblings, 0 replies; 8+ messages in thread
From: Wainer dos Santos Moschetta @ 2021-04-09 14:40 UTC (permalink / raw)
To: Greg Kurz, qemu-devel
Cc: virtio-fs, misono.tomohiro, Stefan Hajnoczi,
Dr. David Alan Gilbert
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;
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] virtiofsd: Fix side-effect in assert()
2021-04-09 10:06 [PATCH] virtiofsd: Fix side-effect in assert() Greg Kurz
` (2 preceding siblings ...)
2021-04-09 14:40 ` Wainer dos Santos Moschetta
@ 2021-04-09 14:56 ` Alex Bennée
2021-04-09 15:11 ` Stefan Weil
2021-05-06 15:46 ` Dr. David Alan Gilbert
5 siblings, 0 replies; 8+ messages in thread
From: Alex Bennée @ 2021-04-09 14:56 UTC (permalink / raw)
To: Greg Kurz
Cc: virtio-fs, qemu-devel, misono.tomohiro, Stefan Hajnoczi,
Dr. David Alan Gilbert
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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] virtiofsd: Fix side-effect in assert()
2021-04-09 10:06 [PATCH] virtiofsd: Fix side-effect in assert() Greg Kurz
` (3 preceding siblings ...)
2021-04-09 14:56 ` Alex Bennée
@ 2021-04-09 15:11 ` Stefan Weil
2021-04-09 15:24 ` Philippe Mathieu-Daudé
2021-05-06 15:46 ` Dr. David Alan Gilbert
5 siblings, 1 reply; 8+ messages in thread
From: Stefan Weil @ 2021-04-09 15:11 UTC (permalink / raw)
To: Greg Kurz, qemu-devel
Cc: virtio-fs, misono.tomohiro, Stefan Hajnoczi,
Dr. David Alan Gilbert
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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] virtiofsd: Fix side-effect in assert()
2021-04-09 15:11 ` Stefan Weil
@ 2021-04-09 15:24 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 8+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-04-09 15:24 UTC (permalink / raw)
To: Stefan Weil, Greg Kurz, qemu-devel
Cc: virtio-fs, misono.tomohiro, Stefan Hajnoczi,
Dr. David Alan Gilbert
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] virtiofsd: Fix side-effect in assert()
2021-04-09 10:06 [PATCH] virtiofsd: Fix side-effect in assert() Greg Kurz
` (4 preceding siblings ...)
2021-04-09 15:11 ` Stefan Weil
@ 2021-05-06 15:46 ` Dr. David Alan Gilbert
5 siblings, 0 replies; 8+ messages in thread
From: Dr. David Alan Gilbert @ 2021-05-06 15:46 UTC (permalink / raw)
To: Greg Kurz; +Cc: virtio-fs, qemu-devel, Stefan Hajnoczi, misono.tomohiro
* 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
^ permalink raw reply [flat|nested] 8+ messages in thread