On Wed, 13 Jun 2012 10:22:32 +0200
Laszlo Ersek <ler...@redhat.com> wrote:
> From: Paolo Bonzini <pbonz...@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 <pbonz...@redhat.com>
> Signed-off-by: Laszlo Ersek <ler...@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);
> }
> ''')
>
