Re: [PATCH] error handling: Use TFR() macro where applicable

[Markus Armbruster <armbru@redhat.com>]

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.