[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] virtiofsd: Fix side-effect in assert()
From: |
Philippe Mathieu-Daudé |
Subject: |
Re: [PATCH] virtiofsd: Fix side-effect in assert() |
Date: |
Fri, 9 Apr 2021 17:24:09 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.8.1 |
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.
- [PATCH] virtiofsd: Fix side-effect in assert(), Greg Kurz, 2021/04/09
- Re: [PATCH] virtiofsd: Fix side-effect in assert(), Philippe Mathieu-Daudé, 2021/04/09
- Re: [Virtio-fs] [PATCH] virtiofsd: Fix side-effect in assert(), Vivek Goyal, 2021/04/09
- Re: [PATCH] virtiofsd: Fix side-effect in assert(), Wainer dos Santos Moschetta, 2021/04/09
- Re: [PATCH] virtiofsd: Fix side-effect in assert(), Alex Bennée, 2021/04/09
- Re: [PATCH] virtiofsd: Fix side-effect in assert(), Stefan Weil, 2021/04/09
- Re: [PATCH] virtiofsd: Fix side-effect in assert(),
Philippe Mathieu-Daudé <=