Re: [RFC v5 024/126] error: auto propagated local_err

[Vladimir Sementsov-Ogievskiy]

04.12.2019 17:59, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <address@hidden> writes:
> 
>> Here is introduced ERRP_AUTO_PROPAGATE macro, to be used at start of
>> functions with errp OUT parameter.
>>
>> It has three goals:
>>
>> 1. Fix issue with error_fatal & error_prepend/error_append_hint: user
>> can't see this additional information, because exit() happens in
>> error_setg earlier than information is added. [Reported by Greg Kurz]
>>
>> 2. Fix issue with error_abort & error_propagate: when we wrap
>> error_abort by local_err+error_propagate, resulting coredump will
>> refer to error_propagate and not to the place where error happened.
> 
> I get what you mean, but I have plenty of context.
> 
>> (the macro itself doesn't fix the issue, but it allows to [3.] drop all
>> local_err+error_propagate pattern, which will definitely fix the issue)
> 
> The parenthesis is not part of the goal.
> 
>> [Reported by Kevin Wolf]
>>
>> 3. Drop local_err+error_propagate pattern, which is used to workaround
>> void functions with errp parameter, when caller wants to know resulting
>> status. (Note: actually these functions could be merely updated to
>> return int error code).
>>
>> To achieve these goals, we need to add invocation of the macro at start
>> of functions, which needs error_prepend/error_append_hint (1.); add
>> invocation of the macro at start of functions which do
>> local_err+error_propagate scenario the check errors, drop local errors
>> from them and just use *errp instead (2., 3.).
> 
> The paragraph talks about two cases: 1. and 2.+3. 

Hmm, I don't think so.. 1. and 2. are issues. 3. is a refactoring.. We just
fix achieve 2 and 3 by one action.

> Makes me think we
> want two paragraphs, each illustrated with an example.
> 
> What about you provide the examples, and then I try to polish the prose?

1: error_fatal problem

Assume the following code flow:

int f1(errp) {
    ...
    ret = f2(errp);
    if (ret < 0) {
       error_append_hint(errp, "very useful hint");
       return ret;
    }
    ...
}

Now, if we call f1 with &error_fatal argument and f2 fails, the program
will exit immediately inside f2, when setting the errp. User will not
see the hint.

So, in this case we should use local_err.

2: error_abort problem

Now, consider functions without return value. We normally use local_err
variable to catch failures:

void f1(errp) {
    Error *local_err = NULL;
    ...
    f2(&local_err);
    if (local_err) {
        error_propagate(errp, local_err);
        return;
    }
    ...
}

Now, if we call f2 with &error_abort and f2 fails, the stack in resulting
crash dump will point to error_propagate, not to the failure point in f2,
which complicates debugging.

So, we should never wrap error_abort by local_err.

===

Our solution:

- Fixes [1.], adding invocation of new macro into functions with 
error_appen_hint/error_prepend,
   New macro will wrap error_fatal.
- Fixes [2.], by switching from hand-written local_err to smart macro, which 
never
   wraps error_abort.
- Handles [3.], by switching to macro, which is less code
- Additionally, macro doesn't wrap normal non-zero errp, to avoid extra 
propagations
   (in fact, error_propagate is called, but returns immediately on first if 
(!local_err))

> 
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
>> Reviewed-by: Eric Blake <address@hidden>
>> ---
>>
>> CC: Gerd Hoffmann <address@hidden>
>> CC: "Gonglei (Arei)" <address@hidden>
>> CC: Eduardo Habkost <address@hidden>
>> CC: Igor Mammedov <address@hidden>
>> CC: Laurent Vivier <address@hidden>
>> CC: Amit Shah <address@hidden>
>> CC: Kevin Wolf <address@hidden>
>> CC: Max Reitz <address@hidden>
>> CC: John Snow <address@hidden>
>> CC: Ari Sundholm <address@hidden>
>> CC: Pavel Dovgalyuk <address@hidden>
>> CC: Paolo Bonzini <address@hidden>
>> CC: Stefan Hajnoczi <address@hidden>
>> CC: Fam Zheng <address@hidden>
>> CC: Stefan Weil <address@hidden>
>> CC: Ronnie Sahlberg <address@hidden>
>> CC: Peter Lieven <address@hidden>
>> CC: Eric Blake <address@hidden>
>> CC: "Denis V. Lunev" <address@hidden>
>> CC: Markus Armbruster <address@hidden>
>> CC: Alberto Garcia <address@hidden>
>> CC: Jason Dillaman <address@hidden>
>> CC: Wen Congyang <address@hidden>
>> CC: Xie Changlong <address@hidden>
>> CC: Liu Yuan <address@hidden>
>> CC: "Richard W.M. Jones" <address@hidden>
>> CC: Jeff Cody <address@hidden>
>> CC: "Marc-André Lureau" <address@hidden>
>> CC: "Daniel P. Berrangé" <address@hidden>
>> CC: Richard Henderson <address@hidden>
>> CC: Greg Kurz <address@hidden>
>> CC: "Michael S. Tsirkin" <address@hidden>
>> CC: Marcel Apfelbaum <address@hidden>
>> CC: Beniamino Galvani <address@hidden>
>> CC: Peter Maydell <address@hidden>
>> CC: "Cédric Le Goater" <address@hidden>
>> CC: Andrew Jeffery <address@hidden>
>> CC: Joel Stanley <address@hidden>
>> CC: Andrew Baumann <address@hidden>
>> CC: "Philippe Mathieu-Daudé" <address@hidden>
>> CC: Antony Pavlov <address@hidden>
>> CC: Jean-Christophe Dubois <address@hidden>
>> CC: Peter Chubb <address@hidden>
>> CC: Subbaraya Sundeep <address@hidden>
>> CC: Eric Auger <address@hidden>
>> CC: Alistair Francis <address@hidden>
>> CC: "Edgar E. Iglesias" <address@hidden>
>> CC: Stefano Stabellini <address@hidden>
>> CC: Anthony Perard <address@hidden>
>> CC: Paul Durrant <address@hidden>
>> CC: Paul Burton <address@hidden>
>> CC: Aleksandar Rikalo <address@hidden>
>> CC: Chris Wulff <address@hidden>
>> CC: Marek Vasut <address@hidden>
>> CC: David Gibson <address@hidden>
>> CC: Cornelia Huck <address@hidden>
>> CC: Halil Pasic <address@hidden>
>> CC: Christian Borntraeger <address@hidden>
>> CC: "Hervé Poussineau" <address@hidden>
>> CC: Xiao Guangrong <address@hidden>
>> CC: Aurelien Jarno <address@hidden>
>> CC: Aleksandar Markovic <address@hidden>
>> CC: Mark Cave-Ayland <address@hidden>
>> CC: Jason Wang <address@hidden>
>> CC: Laszlo Ersek <address@hidden>
>> CC: Yuval Shaia <address@hidden>
>> CC: Palmer Dabbelt <address@hidden>
>> CC: Sagar Karandikar <address@hidden>
>> CC: Bastian Koppelmann <address@hidden>
>> CC: David Hildenbrand <address@hidden>
>> CC: Thomas Huth <address@hidden>
>> CC: Eric Farman <address@hidden>
>> CC: Matthew Rosato <address@hidden>
>> CC: Hannes Reinecke <address@hidden>
>> CC: Michael Walle <address@hidden>
>> CC: Artyom Tarasenko <address@hidden>
>> CC: Stefan Berger <address@hidden>
>> CC: Samuel Thibault <address@hidden>
>> CC: Alex Williamson <address@hidden>
>> CC: Tony Krowiak <address@hidden>
>> CC: Pierre Morel <address@hidden>
>> CC: Michael Roth <address@hidden>
>> CC: Hailiang Zhang <address@hidden>
>> CC: Juan Quintela <address@hidden>
>> CC: "Dr. David Alan Gilbert" <address@hidden>
>> CC: Luigi Rizzo <address@hidden>
>> CC: Giuseppe Lettieri <address@hidden>
>> CC: Vincenzo Maffione <address@hidden>
>> CC: Jan Kiszka <address@hidden>
>> CC: Anthony Green <address@hidden>
>> CC: Stafford Horne <address@hidden>
>> CC: Guan Xuetao <address@hidden>
>> CC: Max Filippov <address@hidden>
>> CC: address@hidden
>> CC: address@hidden
>> CC: address@hidden
>> CC: address@hidden
>> CC: address@hidden
>> CC: address@hidden
>> CC: address@hidden
>> CC: address@hidden
>>
>>   include/qapi/error.h | 38 ++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 38 insertions(+)
>>
>> diff --git a/include/qapi/error.h b/include/qapi/error.h
>> index d6898d833b..47238d9065 100644
>> --- a/include/qapi/error.h
>> +++ b/include/qapi/error.h
>> @@ -345,6 +345,44 @@ void error_set_internal(Error **errp,
>>                           ErrorClass err_class, const char *fmt, ...)
>>       GCC_FMT_ATTR(6, 7);
>>   
>> +typedef struct ErrorPropagator {
>> +    Error *local_err;
>> +    Error **errp;
>> +} ErrorPropagator;
>> +
>> +static inline void error_propagator_cleanup(ErrorPropagator *prop)
>> +{
>> +    error_propagate(prop->errp, prop->local_err);
>> +}
>> +
>> +G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(ErrorPropagator, error_propagator_cleanup);
>> +
>> +/*
>> + * ERRP_AUTO_PROPAGATE
>> + *
>> + * This macro is created to be the first line of a function with Error 
>> **errp
>> + * OUT parameter. It's needed only in cases where we want to use 
>> error_prepend,
>> + * error_append_hint or dereference *errp. It's still safe (but useless) in
>> + * other cases.
>> + *
>> + * If errp is NULL or points to error_fatal, it is rewritten to point to a
>> + * local Error object, which will be automatically propagated to the 
>> original
>> + * errp on function exit (see error_propagator_cleanup).
>> + *
>> + * After invocation of this macro it is always safe to dereference errp
>> + * (as it's not NULL anymore) and to add information (by error_prepend or
>> + * error_append_hint)
>> + * (as, if it was error_fatal, we swapped it with a local_error to be
>> + * propagated on cleanup).
>> + *
>> + * Note: we don't wrap the error_abort case, as we want resulting coredump
>> + * to point to the place where the error happened, not to error_propagate.
>> + */
>> +#define ERRP_AUTO_PROPAGATE()                                  \
>> +    g_auto(ErrorPropagator) _auto_errp_prop = {.errp = errp};  \
>> +    errp = ((errp == NULL || *errp == error_fatal)             \
>> +            ? &_auto_errp_prop.local_err : errp)
>> +
>>   /*
>>    * Special error destination to abort on error.
>>    * See error_setg() and error_propagate() for details.
> 
> Missing: update of the big comment starting with "Error reporting system
> loosely patterned after Glib's GError."
> 


-- 
Best regards,
Vladimir