Re: [qemu-s390x] [RFC] error: auto propagated local_err

[Eric Blake]

On 9/18/19 12:46 PM, Vladimir Sementsov-Ogievskiy wrote:

>>> +/*
>>> + * Third variant:
>>> + *   Pros:
>>> + *     - simpler movement for functions which don't have local_err yet
>>> + *       the only thing to do is to call one macro at function start.
>>> + *       This extremely simplifies Greg's series
>>> + *   Cons:
>>> + *     - looks like errp shadowing.. Still seems safe.
>>> + *     - must be after all definitions of local variables and before any
>>> + *       code.
>>
>> Why?  I see no reason why it can't be hoisted earlier than other
>> declarations, and the only reason to not sink it after earlier code that
>> doesn't touch errp would be our coding standards that frowns on
>> declaration after code.
> 
> Hmm, I thought compiler would warn about mixing code and definitions.
> Seems that gcc don't care, so it's OK.

C89 required all definitions before code, but that's historical.
Meanwhile, we require a compiler that supports C99 as well as at least
the __attribute__((cleanup)) extension (gcc and clang qualify, nothing
else really does, but no one has been complaining).  And C99 requires
compiler support for intermixing definitions (in part because c++ did it
first, then gcc allowed that as an extension in C89); so it's been
permissible to intermix code and declarations for more than 20 years
now.  The only reason we don't do it more is because of habits and
aesthetics, rather than necessity.

>>
>> This is actually quite cool. 
> 
> I glad to see that you like my favorite variant)
> 
>> And if you get rid of your insistence that
>> it must occur after other variable declarations, you could instead
>> easily automate that any function that has a parameter 'Error **errp'
>> then has a MAKE_ERRP_SAFE(errp); as the first line of its function body
>> (that becomes something that you could grep for, rather than having to
>> use the smarts of coccinelle).
>>
>> Or if we want to enforce consistency on the parameter naming, even go with:
>>
>> #define MAKE_ERRP_SAFE() \
>> g_auto(ErrorPropagationStruct) (__auto_errp_prop) = {.errp = errp}; \
>> errp = &__auto_errp_prop.local_err
>>
> 
> I am for

So now to wait for other comments.


>>
>>> +     */
>>> +    MAKE_ERRP_SAFE(errp);
>>> +
>>>       if (filename) {
>>>           ret = qemu_gluster_parse_uri(gconf, filename);
>>>           if (ret < 0) {
>>>
>>
>> This is sweet - you can safely use '*errp' in the rest of the function,
>> without having to remember a second error name - while the caller can
>> still pass NULL or error_abort as desired.
>>
>> And I still think we can probably get Coccinelle to help make the
>> conversions, both of using this macro in any function that has an Error
>> **errp parameter, as well as getting rid of local_err declarations and
>> error_propagate() calls rendered redundant once this macro is used.
>>
> 
> Thanks! And sorry for dirty draft.

It was titled RFC, after all :)

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org