Re: [Virtio-fs] [PATCH] virtiofsd: Fix side-effect in assert()

From: Vivek Goyal <vgoyal@redhat.com>
To: Greg Kurz <groug@kaod.org>
Cc: virtio-fs@redhat.com, qemu-devel@nongnu.org
Subject: Re: [Virtio-fs] [PATCH] virtiofsd: Fix side-effect in assert()
Date: Fri, 9 Apr 2021 09:30:06 -0400	[thread overview]
Message-ID: <20210409133006.GC1111800@redhat.com> (raw)
In-Reply-To: <20210409100627.451573-1-groug@kaod.org>

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