| Message ID | 1339575768-2557-2-git-send-email-lersek@redhat.com |
|---|---|
| State | New |
| Headers | show |
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); > } > ''') >
On 07/13/12 18:38, Luiz Capitulino wrote: > 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. 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 <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. 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 <4FB21B71.7030804@redhat.com>? 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
Il 13/07/2012 19:30, Laszlo Ersek ha scritto: >>> >> - 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 > <4FB21B71.7030804@redhat.com>? 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. The reason to do this for error_propagate was to allow this idiom: /* Always call end_struct if start_struct succeeded. */ error_propagate(errp, err); err = NULL; visit_end_struct(v, &err); error_propagate(errp, err); I think doing it for error_set was just for symmetry and to avoid introducing excessive complexity. Paolo
On 07/13/12 21:11, Paolo Bonzini wrote: > Il 13/07/2012 19:30, Laszlo Ersek ha scritto: >>>>>> - 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 >> <4FB21B71.7030804@redhat.com>? 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. > > The reason to do this for error_propagate was to allow this idiom: > > /* Always call end_struct if start_struct succeeded. */ > error_propagate(errp, err); > err = NULL; > visit_end_struct(v, &err); > error_propagate(errp, err); Right! > I think doing it for error_set was just for symmetry and to avoid > introducing excessive complexity. Correct again. IIRC it was even yours truly who humbly suggested that. Thanks! Laszlo
On Fri, 13 Jul 2012 21:11:15 +0200 Paolo Bonzini <pbonzini@redhat.com> wrote: > Il 13/07/2012 19:30, Laszlo Ersek ha scritto: > >>> >> - 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 > > <4FB21B71.7030804@redhat.com>? 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. > > The reason to do this for error_propagate was to allow this idiom: > > /* Always call end_struct if start_struct succeeded. */ > error_propagate(errp, err); > err = NULL; > visit_end_struct(v, &err); > error_propagate(errp, err); I agree with this change for error_propagate() because it encapsulates our rules for error propagation. > I think doing it for error_set was just for symmetry and to avoid > introducing excessive complexity. We already check if the error is set in several places, and I don't think it will add much complexity. I still think that an assert() is better.
On 07/16/12 19:12, Luiz Capitulino wrote: > On Fri, 13 Jul 2012 21:11:15 +0200 > Paolo Bonzini <pbonzini@redhat.com> wrote: > >> I think doing it for error_set was just for symmetry and to avoid >> introducing excessive complexity. > > We already check if the error is set in several places, and I don't think > it will add much complexity. I still think that an assert() is better. If that means that the generated traversal code takes responsibility to call any visitor callback with a fresh error receptacle, IOW I can go ahead and just use error_set() in OptsVisitor and any firing assert will be blamed on the generator: fine :) Thanks, Laszlo
On Mon, 16 Jul 2012 22:31:26 +0200 Laszlo Ersek <lersek@redhat.com> wrote: > On 07/16/12 19:12, Luiz Capitulino wrote: > > On Fri, 13 Jul 2012 21:11:15 +0200 > > Paolo Bonzini <pbonzini@redhat.com> wrote: > > > > >> I think doing it for error_set was just for symmetry and to avoid > >> introducing excessive complexity. > > > > We already check if the error is set in several places, and I don't think > > it will add much complexity. I still think that an assert() is better. > > If that means that the generated traversal code takes responsibility to > call any visitor callback with a fresh error receptacle, IOW I can go > ahead and just use error_set() in OptsVisitor and any firing assert will > be blamed on the generator: fine :) If that means it's finding bugs then that's great. On the other hand, if it shows only false positives and we end up having to re-work the code just to avoid that, then I'd agree on not having an assert().
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) { 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); - } + 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); } ''')