[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] error handling: Use TFR() macro where applicable
From: |
Markus Armbruster |
Subject: |
Re: [PATCH] error handling: Use TFR() macro where applicable |
Date: |
Mon, 08 Aug 2022 16:22:24 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/27.2 (gnu/linux) |
Christian Schoenebeck <qemu_oss@crudebyte.com> writes:
> On Montag, 8. August 2022 14:52:28 CEST Christian Schoenebeck wrote:
>> On Montag, 8. August 2022 10:05:56 CEST Markus Armbruster wrote:
>> > Nikita Ivanov <nivanov@cloudlinux.com> writes:
>> > > Summing up the discussion above, I suggest the following patch for TFR()
>> > > macro refactoring. (The patch is sequential to the first one I
>> > > introduced
>> > > in the start of the discussion).
>> > >
>> > >>From 6318bee052900aa93bba6620b53c7cb2290e5001 Mon Sep 17 00:00:00 2001
>> > >>
>> > > From: Nikita Ivanov <nivanov@cloudlinux.com>
>> > > Date: Mon, 8 Aug 2022 09:30:34 +0300
>> > > Subject: [PATCH] Refactoring: rename TFR() to TEMP_FAILURE_RETRY()
>> > >
>> > > glibc's unistd.h header provides the same macro with the
>> > > subtle difference in type casting. Adjust macro name to the
>> > > common standard and define conditionally.
>> > >
>> > > Signed-off-by: Nikita Ivanov <nivanov@cloudlinux.com>
[...]
>> > > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
>> > > index b1c161c035..55f2927d8b 100644
>> > > --- a/include/qemu/osdep.h
>> > > +++ b/include/qemu/osdep.h
>> > > @@ -242,8 +242,10 @@ void QEMU_ERROR("code path is reachable")
>> > >
>> > > #if !defined(ESHUTDOWN)
>> > > #define ESHUTDOWN 4099
>> > > #endif
>> > >
>> > > -
>> > > -#define TFR(expr) do { if ((expr) != -1) break; } while (errno ==
>> > > EINTR)
>> > > +#if !defined(TEMP_FAILURE_RETRY)
>> > > +#define TEMP_FAILURE_RETRY(expr) \
>> > > + do { if ((expr) != -1) break; } while (errno == EINTR)
To avoid / reduce confusion: this macro expands into a statement, and ...
>> > > +#endif
>> >
>> > GLibc's version is
>> >
>> > # define TEMP_FAILURE_RETRY(expression) \
>> > (__extension__
>> > \
>> > ({ long int __result;
>> > \
>> > do __result = (long int) (expression);
>> > \
>> > while (__result == -1L && errno == EINTR);
>> > \
>> > __result; }))
... this one expands into an expression. It uses GCC's "a compound
statement enclosed in parentheses may appear as an expression" extension.
>> >
>> > The difference isn't just "type casting", it's also statement
>> > vs. expression.
>> >
>> > Is it a good idea to have the macro expand into a statement on some
>> > hosts, and into an expression on others? Sure, CI should catch any uses
>> > as expression, but delaying compile errors to CI wastes developer time.
>>
>> For consistency and simplicity, I would define exactly one version (no
>> ifdefs) of the macro with a different macro name than glibc's
>> TEMP_FAILURE_RETRY(), and use that QEMU specific macro name in QEMU code
>> everywhere.
TFR()? Can't resist closing the circle...
>> As for statement vs. expression: The only advantage of the statement version
>> is if you'd need __result as an rvalue, which is not needed ATM, right? So
>> I would go for the expression version (with cast) for now.
The expression-like macro is nicer where the return value matters.
Example (stolen from "The GNU C Library Reference Manual"):
nbytes = TEMP_FAILURE_RETRY (write (desc, buffer, count));
With the statement-like macro, you have to write
TEMP_FAILURE_RETRY (nbytes = write (desc, buffer, count));
>> The glibc history does not reveal why they chose the statement version.
The expression version, actually.
>> Best regards,
>> Christian Schoenebeck
>
> Sorry: s/rvalue/lvalue/ i.e. if you need the memory address of result or if
> you need to take the result value of the last iteration in 'expression' into
> account.
- [PATCH] error handling: Use TFR() macro where applicable, Nikita Ivanov, 2022/08/04
- Re: [PATCH] error handling: Use TFR() macro where applicable, Christian Schoenebeck, 2022/08/05
- Re: [PATCH] error handling: Use TFR() macro where applicable, Marc-André Lureau, 2022/08/05
- Re: [PATCH] error handling: Use TFR() macro where applicable, Peter Maydell, 2022/08/05
- Re: [PATCH] error handling: Use TFR() macro where applicable, Nikita Ivanov, 2022/08/08
- Re: [PATCH] error handling: Use TFR() macro where applicable, Markus Armbruster, 2022/08/08
- Re: [PATCH] error handling: Use TFR() macro where applicable, Nikita Ivanov, 2022/08/08
- Re: [PATCH] error handling: Use TFR() macro where applicable, Christian Schoenebeck, 2022/08/08
- Re: [PATCH] error handling: Use TFR() macro where applicable, Christian Schoenebeck, 2022/08/08
- Re: [PATCH] error handling: Use TFR() macro where applicable,
Markus Armbruster <=
- Re: [PATCH] error handling: Use TFR() macro where applicable, Nikita Ivanov, 2022/08/08
- Re: [PATCH] error handling: Use TFR() macro where applicable, Nikita Ivanov, 2022/08/08
- Re: [PATCH] error handling: Use TFR() macro where applicable, Nikita Ivanov, 2022/08/17
- Re: [PATCH] error handling: Use TFR() macro where applicable, Peter Maydell, 2022/08/17
- Re: [PATCH] error handling: Use TFR() macro where applicable, Nikita Ivanov, 2022/08/17
- Re: [PATCH] error handling: Use TFR() macro where applicable, Peter Maydell, 2022/08/17
- Re: [PATCH] error handling: Use TFR() macro where applicable, Christian Schoenebeck, 2022/08/18
- Re: [PATCH] error handling: Use TFR() macro where applicable, Peter Maydell, 2022/08/18