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

[Luiz Capitulino <lcapitulino@redhat.com>]

On Wed, 13 Jun 2012 10:22:32 +0200
Laszlo Ersek <lersek@redhat.com> wrote:

> From: Paolo Bonzini <pbonzini@redhat.com>
> 
> Don't overwrite / leak previously set errors.

Can you elaborate a bit more? It's not clear to me where the bug is.

More comments below.

> Don't try to end a container that could not be started.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  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.

>          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);
>  }
>  
>  void visit_start_list(Visitor *v, const char *name, Error **errp)
> @@ -62,9 +61,8 @@ GenericList *visit_next_list(Visitor *v, GenericList **list, Error **errp)
>  
>  void visit_end_list(Visitor *v, Error **errp)
>  {
> -    if (!error_is_set(errp)) {
> -        v->end_list(v, errp);
> -    }
> +    assert(!error_is_set(errp));
> +    v->end_list(v, errp);
>  }
>  
>  void visit_start_optional(Visitor *v, bool *present, const char *name,
> diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c
> index c30fdc4..8f5a509 100644
> --- a/tests/test-qmp-input-visitor.c
> +++ b/tests/test-qmp-input-visitor.c
> @@ -151,14 +151,22 @@ typedef struct TestStruct
>  static void visit_type_TestStruct(Visitor *v, TestStruct **obj,
>                                    const char *name, Error **errp)
>  {
> -    visit_start_struct(v, (void **)obj, "TestStruct", name, sizeof(TestStruct),
> -                       errp);
> -
> -    visit_type_int(v, &(*obj)->integer, "integer", errp);
> -    visit_type_bool(v, &(*obj)->boolean, "boolean", errp);
> -    visit_type_str(v, &(*obj)->string, "string", errp);
> -
> -    visit_end_struct(v, errp);
> +    Error *err = NULL;
> +    if (!error_is_set(errp)) {
> +        visit_start_struct(v, (void **)obj, "TestStruct", name, sizeof(TestStruct),
> +                           &err);
> +        if (!err) {
> +            visit_type_int(v, &(*obj)->integer, "integer", &err);
> +            visit_type_bool(v, &(*obj)->boolean, "boolean", &err);
> +            visit_type_str(v, &(*obj)->string, "string", &err);
> +
> +            /* Always call end_struct if start_struct succeeded.  */
> +            error_propagate(errp, err);
> +            err = NULL;
> +            visit_end_struct(v, &err);
> +        }
> +        error_propagate(errp, err);
> +    }
>  }
>  
>  static void test_visitor_in_struct(TestInputVisitorData *data,
> diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
> index ad11767..cccb11e 100644
> --- a/docs/qapi-code-gen.txt
> +++ b/docs/qapi-code-gen.txt
> @@ -220,6 +220,8 @@ Example:
>      #endif
>      mdroth@illuin:~/w/qemu2.git$
>  
> +(The actual structure of the visit_type_* functions is a bit more complex
> +in order to propagate errors correctly and avoid leaking memory).
>  
>  === scripts/qapi-commands.py ===
>  
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index 8d4e94a..61cf586 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -17,14 +17,37 @@ import os
>  import getopt
>  import errno
>  
> -def generate_visit_struct_body(field_prefix, members):
> -    ret = ""
> +def generate_visit_struct_body(field_prefix, name, members):
> +    ret = mcgen('''
> +if (!error_is_set(errp)) {
> +''')
> +    push_indent()
> +
>      if len(field_prefix):
>          field_prefix = field_prefix + "."
> +        ret += mcgen('''
> +Error **errp = &err; /* from outer scope */
> +Error *err = NULL;
> +visit_start_struct(m, NULL, "", "%(name)s", 0, &err);
> +''',
> +                name=name)
> +    else:
> +        ret += mcgen('''
> +Error *err = NULL;
> +visit_start_struct(m, (void **)obj, "%(name)s", name, sizeof(%(name)s), &err);
> +''',
> +                name=name)
> +
> +    ret += mcgen('''
> +if (!err) {
> +    assert(!obj || *obj);
> +''')
> +
> +    push_indent()
>      for argname, argentry, optional, structured in parse_args(members):
>          if optional:
>              ret += mcgen('''
> -visit_start_optional(m, (obj && *obj) ? &(*obj)->%(c_prefix)shas_%(c_name)s : NULL, "%(name)s", errp);
> +visit_start_optional(m, obj ? &(*obj)->%(c_prefix)shas_%(c_name)s : NULL, "%(name)s", &err);
>  if ((*obj)->%(prefix)shas_%(c_name)s) {
>  ''',
>                           c_prefix=c_var(field_prefix), prefix=field_prefix,
> @@ -32,17 +55,10 @@ if ((*obj)->%(prefix)shas_%(c_name)s) {
>              push_indent()
>  
>          if structured:
> -            ret += mcgen('''
> -visit_start_struct(m, NULL, "", "%(name)s", 0, errp);
> -''',
> -                         name=argname)
> -            ret += generate_visit_struct_body(field_prefix + argname, argentry)
> -            ret += mcgen('''
> -visit_end_struct(m, errp);
> -''')
> +            ret += generate_visit_struct_body(field_prefix + argname, argname, argentry)
>          else:
>              ret += mcgen('''
> -visit_type_%(type)s(m, (obj && *obj) ? &(*obj)->%(c_prefix)s%(c_name)s : NULL, "%(name)s", errp);
> +visit_type_%(type)s(m, obj ? &(*obj)->%(c_prefix)s%(c_name)s : NULL, "%(name)s", &err);
>  ''',
>                           c_prefix=c_var(field_prefix), prefix=field_prefix,
>                           type=type_name(argentry), c_name=c_var(argname),
> @@ -52,7 +68,19 @@ visit_type_%(type)s(m, (obj && *obj) ? &(*obj)->%(c_prefix)s%(c_name)s : NULL, "
>              pop_indent()
>              ret += mcgen('''
>  }
> -visit_end_optional(m, errp);
> +visit_end_optional(m, &err);
> +''')
> +
> +    pop_indent()
> +    pop_indent()
> +    ret += mcgen('''
> +        /* Always call end_struct if start_struct succeeded.  */
> +        error_propagate(errp, err);
> +        err = NULL;
> +        visit_end_struct(m, &err);
> +    }
> +    error_propagate(errp, err);
> +}
>  ''')
>      return ret
>  
> @@ -61,22 +89,14 @@ def generate_visit_struct(name, members):
>  
>  void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **errp)
>  {
> -    if (error_is_set(errp)) {
> -        return;
> -    }
> -    visit_start_struct(m, (void **)obj, "%(name)s", name, sizeof(%(name)s), errp);
> -    if (obj && !*obj) {
> -        goto end;
> -    }
>  ''',
>                  name=name)
> +
>      push_indent()
> -    ret += generate_visit_struct_body("", members)
> +    ret += generate_visit_struct_body("", name, members)
>      pop_indent()
>  
>      ret += mcgen('''
> -end:
> -    visit_end_struct(m, errp);
>  }
>  ''')
>      return ret
> @@ -87,18 +107,22 @@ def generate_visit_list(name, members):
>  void visit_type_%(name)sList(Visitor *m, %(name)sList ** obj, const char *name, Error **errp)
>  {
>      GenericList *i, **prev = (GenericList **)obj;
> +    Error *err = NULL;
>  
> -    if (error_is_set(errp)) {
> -        return;
> -    }
> -    visit_start_list(m, name, errp);
> -
> -    for (; (i = visit_next_list(m, prev, errp)) != NULL; prev = &i) {
> -        %(name)sList *native_i = (%(name)sList *)i;
> -        visit_type_%(name)s(m, &native_i->value, NULL, errp);
> +    if (!error_is_set(errp)) {
> +        visit_start_list(m, name, &err);
> +        if (!err) {
> +            for (; (i = visit_next_list(m, prev, &err)) != NULL; prev = &i) {
> +                %(name)sList *native_i = (%(name)sList *)i;
> +                visit_type_%(name)s(m, &native_i->value, NULL, &err);
> +            }
> +            /* Always call end_list if start_list succeeded.  */
> +            error_propagate(errp, err);
> +            err = NULL;
> +            visit_end_list(m, &err);
> +        }
> +        error_propagate(errp, err);
>      }
> -
> -    visit_end_list(m, errp);
>  }
>  ''',
>                  name=name)
> @@ -122,27 +146,21 @@ void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **
>  {
>      Error *err = NULL;
>  
> -    if (error_is_set(errp)) {
> -        return;
> -    }
> -    visit_start_struct(m, (void **)obj, "%(name)s", name, sizeof(%(name)s), &err);
> -    if (obj && !*obj) {
> -        goto end;
> -    }
> -    visit_type_%(name)sKind(m, &(*obj)->kind, "type", &err);
> -    if (err) {
> -        error_propagate(errp, err);
> -        goto end;
> -    }
> -    switch ((*obj)->kind) {
> +    if (!error_is_set(errp)) {
> +        visit_start_struct(m, (void **)obj, "%(name)s", name, sizeof(%(name)s), &err);
> +        if (!err) {
> +            visit_type_%(name)sKind(m, &(*obj)->kind, "type", &err);
> +        }
> +        if (!err) {
> +            switch ((*obj)->kind) {
>  ''',
>                   name=name)
>  
>      for key in members:
>          ret += mcgen('''
> -    case %(abbrev)s_KIND_%(enum)s:
> -        visit_type_%(c_type)s(m, &(*obj)->%(c_name)s, "data", errp);
> -        break;
> +            case %(abbrev)s_KIND_%(enum)s:
> +                visit_type_%(c_type)s(m, &(*obj)->%(c_name)s, "data", &err);
> +                break;
>  ''',
>                  abbrev = de_camel_case(name).upper(),
>                  enum = c_fun(de_camel_case(key)).upper(),
> @@ -150,11 +168,16 @@ void visit_type_%(name)s(Visitor *m, %(name)s ** obj, const char *name, Error **
>                  c_name=c_fun(key))
>  
>      ret += mcgen('''
> -    default:
> -        abort();
> +            default:
> +                abort();
> +            }
> +        }
> +        /* Always call end_struct if start_struct succeeded.  */
> +        error_propagate(errp, err);
> +        err = NULL;
> +        visit_end_struct(m, &err);
>      }
> -end:
> -    visit_end_struct(m, errp);
> +    error_propagate(errp, err);
>  }
>  ''')
>