Re: [Qemu-devel] [PATCH v2 01/17] qapi: fix error propagation

[Laszlo Ersek]

On 07/13/12 18:38, Luiz Capitulino wrote:
> On Wed, 13 Jun 2012 10:22:32 +0200
> Laszlo Ersek <address@hidden> wrote:
> 
>> From: Paolo Bonzini <address@hidden>
>>
>> Don't overwrite / leak previously set errors.
> 
> Can you elaborate a bit more? It's not clear to me where the bug is.

Suppose you encounter the first error on the normal path, while a bunch
of objects is being constructed / composed. You set the error
accordingly and start to unwind the stack, tearing down objects
previously composed fully or partially. While doing this, you encounter
another error. If you call error_set() or error_propagate() now, the
first error is leaked. To avoid this, you have to check.

This change saves you the checks during stack unwinding, keeps the first
error stored (which is more important than any destructor errors, since
the latter could be the direct consequence of the first error and
aborting further processing). Second and later errors attempted to be
set via error_set() are simply not formatted, while second and later
errors attempted to be propagated with error_propagate() are released
(as there are two errors and we keep only one).

See "qapi: introduce OptsVisitor", function opts_end_struct(), comment
"we should have processed all (distinct) QemuOpt instances". If we abort
processing due to some error, there may be leftover options. We
shouldn't overwrite the first (real) error with this bogus one.

The stack is unwound in this case by the generated code -- if some
deeper part of OptsVisitor sets an error, the generated code will make
sure opts_end_struct() is called the right number of times.


> 
> More comments below.
> 
>> Don't try to end a container that could not be started.
>>
>> Signed-off-by: Paolo Bonzini <address@hidden>
>> Signed-off-by: Laszlo Ersek <address@hidden>
>> ---
>>  error.h                        |    4 +-
>>  error.c                        |    4 +-
>>  qapi/qapi-visit-core.c         |   10 +--
>>  tests/test-qmp-input-visitor.c |   24 +++++---
>>  docs/qapi-code-gen.txt         |    2 +
>>  scripts/qapi-visit.py          |  129 
>> +++++++++++++++++++++++----------------
>>  6 files changed, 102 insertions(+), 71 deletions(-)
>>
>> diff --git a/error.h b/error.h
>> index 45ff6c1..6898f84 100644
>> --- a/error.h
>> +++ b/error.h
>> @@ -24,7 +24,7 @@ typedef struct Error Error;
>>  /**
>>   * Set an indirect pointer to an error given a printf-style format 
>> parameter.
>>   * Currently, qerror.h defines these error formats.  This function is not
>> - * meant to be used outside of QEMU.
>> + * meant to be used outside of QEMU.  Errors after the first are discarded.
>>   */
>>  void error_set(Error **err, const char *fmt, ...) GCC_FMT_ATTR(2, 3);
>>  
>> @@ -57,7 +57,7 @@ void error_set_field(Error *err, const char *field, const 
>> char *value);
>>  /**
>>   * Propagate an error to an indirect pointer to an error.  This function 
>> will
>>   * always transfer ownership of the error reference and handles the case 
>> where
>> - * dst_err is NULL correctly.
>> + * dst_err is NULL correctly.  Errors after the first are discarded.
>>   */
>>  void error_propagate(Error **dst_err, Error *local_err);
>>  
>> diff --git a/error.c b/error.c
>> index a52b771..0177972 100644
>> --- a/error.c
>> +++ b/error.c
>> @@ -29,7 +29,7 @@ void error_set(Error **errp, const char *fmt, ...)
>>      Error *err;
>>      va_list ap;
>>  
>> -    if (errp == NULL) {
>> +    if (errp == NULL || *errp != NULL) {
> 
> I think we should use assert() here.
> 
> If the error is already set, that most probably indicates a bug in the 
> caller, as
> it's the caller's responsibility to decide which error to return.

I believe we had a good argument against this, but I can't precisely
recall (or find) it now. Paolo, do you remember? Can you please both
search your respective mailboxen for Message-ID
<address@hidden>? That's where we started to discuss this.

I believe I saw some paths in the code that tripped on this leak, and
generally keeping the first error seemed like a good idea.
opts_end_struct() originally checked for any pre-existent error
explicitly, but then the check was moved to the common code.


> 
>>          return;
>>      }
>>  
>> @@ -132,7 +132,7 @@ bool error_is_type(Error *err, const char *fmt)
>>  
>>  void error_propagate(Error **dst_err, Error *local_err)
>>  {
>> -    if (dst_err) {
>> +    if (dst_err && !*dst_err) {
>>          *dst_err = local_err;
>>      } else if (local_err) {
>>          error_free(local_err);
>> diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
>> index ffffbf7..0a513d2 100644
>> --- a/qapi/qapi-visit-core.c
>> +++ b/qapi/qapi-visit-core.c
>> @@ -39,9 +39,8 @@ void visit_start_struct(Visitor *v, void **obj, const char 
>> *kind,
>>  
>>  void visit_end_struct(Visitor *v, Error **errp)
>>  {
>> -    if (!error_is_set(errp)) {
>> -        v->end_struct(v, errp);
>> -    }
> 
> Is this the ending of a container that could not be started? But if it 
> couldn't
> be started, then errp be will be set and we won't try to end it, no?
> 
>> +    assert(!error_is_set(errp));
>> +    v->end_struct(v, errp);
>>  }

(Perhaps I'm misunderstanding.)

Exactly as you say. That's why we replace the check with an assert.

(This seems to be your last question for this patch, so I'm cutting the
rest.)

Thanks,
Laszlo