From mboxrd@z Thu Jan 1 00:00:00 1970 Date: Fri, 9 Apr 2021 09:30:06 -0400 From: Vivek Goyal Message-ID: <20210409133006.GC1111800@redhat.com> References: <20210409100627.451573-1-groug@kaod.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210409100627.451573-1-groug@kaod.org> Subject: Re: [Virtio-fs] [PATCH] virtiofsd: Fix side-effect in assert() List-Id: Development discussions about virtio-fs List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Greg Kurz Cc: virtio-fs@redhat.com, qemu-devel@nongnu.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 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-fs@redhat.com > https://listman.redhat.com/mailman/listinfo/virtio-fs