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

2021-05-06 Thread Dr. David Alan Gilbert
* Greg Kurz (gr...@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.tomoh...@jp.fujitsu.com
> Signed-off-by: Greg Kurz 

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 / dgilb...@redhat.com / Manchester, UK




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

2021-04-09 Thread Philippe Mathieu-Daudé
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.tomoh...@jp.fujitsu.com
>> Signed-off-by: Greg Kurz 
>> ---
>>   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.




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

2021-04-09 Thread Stefan Weil

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.tomoh...@jp.fujitsu.com
Signed-off-by: Greg Kurz 
---
  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






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

2021-04-09 Thread Alex Bennée


Greg Kurz  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.tomoh...@jp.fujitsu.com
> Signed-off-by: Greg Kurz 

Reviewed-by: Alex Bennée 

-- 
Alex Bennée



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

2021-04-09 Thread Wainer dos Santos Moschetta



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.tomoh...@jp.fujitsu.com
Signed-off-by: Greg Kurz 
---
  tools/virtiofsd/passthrough_ll.c | 21 +
  1 file changed, 13 insertions(+), 8 deletions(-)



Reviewed-by: Wainer dos Santos Moschetta 




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;





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

2021-04-09 Thread Vivek Goyal
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.tomoh...@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...@redhat.com
> https://listman.redhat.com/mailman/listinfo/virtio-fs




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

2021-04-09 Thread Philippe Mathieu-Daudé
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.tomoh...@jp.fujitsu.com
> Signed-off-by: Greg Kurz 
> ---
>  tools/virtiofsd/passthrough_ll.c | 21 +
>  1 file changed, 13 insertions(+), 8 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé 




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

2021-04-09 Thread 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.tomoh...@jp.fujitsu.com
Signed-off-by: Greg Kurz 
---
 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