Re: [Virtio-fs] [PULL 01/12] virtiofsd: Fix side-effect in assert()

From: Edward McClanahan <emcclanahan@nvidia.com>
To: "qemu-devel@nongnu.org" <qemu-devel@nongnu.org>,
	"groug@kaod.org" <groug@kaod.org>,
	"jose.carlos.venegas.munoz@intel.com"
	<jose.carlos.venegas.munoz@intel.com>,
	"ma.mandourr@gmail.com" <ma.mandourr@gmail.com>,
	"Dr. David Alan Gilbert (git)" <dgilbert@redhat.com>
Cc: "virtio-fs@redhat.com" <virtio-fs@redhat.com>,
	"vgoyal@redhat.com" <vgoyal@redhat.com>
Subject: Re: [Virtio-fs] [PULL 01/12] virtiofsd: Fix side-effect in assert()
Date: Thu, 6 May 2021 19:11:14 +0000	[thread overview]
Message-ID: <BY5PR12MB3809AA7700A873AEF4CCEB9AC5589@BY5PR12MB3809.namprd12.prod.outlook.com> (raw)
In-Reply-To: <20210506185641.284821-2-dgilbert@redhat.com>

[-- Attachment #1: Type: text/plain, Size: 4533 bytes --]

Very nice catch David... Countless times I've gotten bit by this one!

Get Outlook for Android<https://aka.ms/AAb9ysg>

________________________________
From: virtio-fs-bounces@redhat.com <virtio-fs-bounces@redhat.com> on behalf of Dr. David Alan Gilbert (git) <dgilbert@redhat.com>
Sent: Thursday, May 6, 2021 1:56:30 PM
To: qemu-devel@nongnu.org <qemu-devel@nongnu.org>; groug@kaod.org <groug@kaod.org>; jose.carlos.venegas.munoz@intel.com <jose.carlos.venegas.munoz@intel.com>; ma.mandourr@gmail.com <ma.mandourr@gmail.com>
Cc: virtio-fs@redhat.com <virtio-fs@redhat.com>; vgoyal@redhat.com <vgoyal@redhat.com>
Subject: [Virtio-fs] [PULL 01/12] virtiofsd: Fix side-effect in assert()

External email: Use caution opening links or attachments


From: Greg Kurz <groug@kaod.org>

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>
Message-Id: <20210409100627.451573-1-groug@kaod.org>
Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 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 1553d2ef45..6592f96f68 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.31.1

_______________________________________________
Virtio-fs mailing list
Virtio-fs@redhat.com
https://listman.redhat.com/mailman/listinfo/virtio-fs

[-- Attachment #2: Type: text/html, Size: 8163 bytes --]
