| Message ID | 1461879932-9020-3-git-send-email-eblake@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Eric Blake <eblake@redhat.com> writes: > Our existing input visitors were not very consistent on errors > in a function taking 'TYPE **obj' (that is, start_struct(), > start_alternate(), type_str(), and type_any(). next_list() is > similar, except that since commit 08f9541, it can't fail). Multiple sentences in a parenthesis, ugh :) Our existing input visitors were not very consistent on errors in a function taking 'TYPE **obj'. These are start_struct(), start_alternate(), type_str(), and type_any(). next_list() is similar, but can't fail (see commit since 08f9541). Can touch up on commit. > While all of them set '*obj' to allocated storage on success, > it was not obvious whether '*obj' was guaranteed safe on failure, > or whether it was left uninitialized. But a future patch wants > to guarantee that visit_type_FOO() does not leak a partially- > constructed obj back to the caller; it is easier to implement > this if we can reliably state that input visitors assign '*obj' > regardless of success or failure, and that on failure *obj is > NULL. Add assertions to enforce consistency in the final > setting of err vs. *obj. > > The opts-visitor start_struct() doesn't set an error, but it > also was doing a weird check for 0 size; all callers pass in > non-zero size if obj is non-NULL. > > The testsuite has at least one spot where we no longer need > to pre-initialize a variable prior to a visit; valgrind confirms > that the test is still fine with the cleanup. > > A later patch will document the design constraint implemented > here. > > Signed-off-by: Eric Blake <eblake@redhat.com> > > --- > v16: tighten assertions, another commit message tweak > v15: enhance commit message, hoist assertions from later in series > v14: no change > v13: no change > v12: new patch > --- > qapi/qapi-visit-core.c | 34 ++++++++++++++++++++++++++++++---- > qapi/opts-visitor.c | 3 ++- > qapi/qmp-input-visitor.c | 4 ++++ > qapi/string-input-visitor.c | 1 + > tests/test-qmp-input-strict.c | 2 +- > 5 files changed, 38 insertions(+), 6 deletions(-) > > diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c > index 3cd7edc..7ad5ff4 100644 > --- a/qapi/qapi-visit-core.c > +++ b/qapi/qapi-visit-core.c > @@ -23,7 +23,13 @@ > void visit_start_struct(Visitor *v, const char *name, void **obj, > size_t size, Error **errp) > { > - v->start_struct(v, name, obj, size, errp); > + Error *err = NULL; > + > + v->start_struct(v, name, obj, size, &err); > + if (obj && v->type == VISITOR_INPUT) { > + assert(!err != !*obj); > + } > + error_propagate(errp, err); > } > > void visit_end_struct(Visitor *v, Error **errp) > @@ -51,10 +57,16 @@ void visit_start_alternate(Visitor *v, const char *name, > GenericAlternate **obj, size_t size, > bool promote_int, Error **errp) > { > + Error *err = NULL; > + > assert(obj && size >= sizeof(GenericAlternate)); > if (v->start_alternate) { > - v->start_alternate(v, name, obj, size, promote_int, errp); > + v->start_alternate(v, name, obj, size, promote_int, &err); > } > + if (v->type == VISITOR_INPUT) { > + assert(!err != !*obj); Could assert(v->start_alternate && !err != !*obj), to preempt "what if !v->start_alternate" worries. If you like that, I can do it on commit. > + } > + error_propagate(errp, err); > } > > void visit_end_alternate(Visitor *v) > @@ -188,7 +200,14 @@ void visit_type_bool(Visitor *v, const char *name, bool *obj, Error **errp) > > void visit_type_str(Visitor *v, const char *name, char **obj, Error **errp) > { > - v->type_str(v, name, obj, errp); > + Error *err = NULL; > + > + assert(obj); > + v->type_str(v, name, obj, &err); > + if (v->type == VISITOR_INPUT) { > + assert(!err != !*obj); > + } > + error_propagate(errp, err); > } > > void visit_type_number(Visitor *v, const char *name, double *obj, > @@ -199,7 +218,14 @@ void visit_type_number(Visitor *v, const char *name, double *obj, > > void visit_type_any(Visitor *v, const char *name, QObject **obj, Error **errp) > { > - v->type_any(v, name, obj, errp); > + Error *err = NULL; > + > + assert(obj); > + v->type_any(v, name, obj, &err); > + if (v->type == VISITOR_INPUT) { > + assert(!err != !*obj); > + } > + error_propagate(errp, err); > } > > static void output_type_enum(Visitor *v, const char *name, int *obj, > diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c > index 66aeaed..4cb6436 100644 > --- a/qapi/opts-visitor.c > +++ b/qapi/opts-visitor.c > @@ -133,7 +133,7 @@ opts_start_struct(Visitor *v, const char *name, void **obj, > const QemuOpt *opt; > > if (obj) { > - *obj = g_malloc0(size > 0 ? size : 1); > + *obj = g_malloc0(size); > } > if (ov->depth++ > 0) { > return; > @@ -314,6 +314,7 @@ opts_type_str(Visitor *v, const char *name, char **obj, Error **errp) > > opt = lookup_scalar(ov, name, errp); > if (!opt) { > + *obj = NULL; > return; > } > *obj = g_strdup(opt->str ? opt->str : ""); > diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c > index 02d4233..77cce8b 100644 > --- a/qapi/qmp-input-visitor.c > +++ b/qapi/qmp-input-visitor.c > @@ -120,6 +120,9 @@ static void qmp_input_start_struct(Visitor *v, const char *name, void **obj, > QObject *qobj = qmp_input_get_object(qiv, name, true); > Error *err = NULL; > > + if (obj) { > + *obj = NULL; > + } > if (!qobj || qobject_type(qobj) != QTYPE_QDICT) { > error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null", > "QDict"); > @@ -267,6 +270,7 @@ static void qmp_input_type_str(Visitor *v, const char *name, char **obj, > QString *qstr = qobject_to_qstring(qmp_input_get_object(qiv, name, true)); > > if (!qstr) { > + *obj = NULL; > error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null", > "string"); > return; > diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c > index d604575..797973a 100644 > --- a/qapi/string-input-visitor.c > +++ b/qapi/string-input-visitor.c > @@ -293,6 +293,7 @@ static void parse_type_str(Visitor *v, const char *name, char **obj, > if (siv->string) { > *obj = g_strdup(siv->string); > } else { > + *obj = NULL; > error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null", > "string"); > } > diff --git a/tests/test-qmp-input-strict.c b/tests/test-qmp-input-strict.c > index d71727e..d5f80ec 100644 > --- a/tests/test-qmp-input-strict.c > +++ b/tests/test-qmp-input-strict.c > @@ -263,7 +263,7 @@ static void test_validate_fail_union_flat_no_discrim(TestInputVisitorData *data, > static void test_validate_fail_alternate(TestInputVisitorData *data, > const void *unused) > { > - UserDefAlternate *tmp = NULL; > + UserDefAlternate *tmp; > Visitor *v; > Error *err = NULL;
On 04/29/2016 02:28 AM, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> writes: > >> Our existing input visitors were not very consistent on errors >> in a function taking 'TYPE **obj' (that is, start_struct(), >> start_alternate(), type_str(), and type_any(). next_list() is >> similar, except that since commit 08f9541, it can't fail). > > Multiple sentences in a parenthesis, ugh :) > > Our existing input visitors were not very consistent on errors in a > function taking 'TYPE **obj'. These are start_struct(), > start_alternate(), type_str(), and type_any(). next_list() is > similar, but can't fail (see commit since 08f9541). > > Can touch up on commit. > Yes, sounds better. >> @@ -51,10 +57,16 @@ void visit_start_alternate(Visitor *v, const char *name, >> GenericAlternate **obj, size_t size, >> bool promote_int, Error **errp) >> { >> + Error *err = NULL; >> + >> assert(obj && size >= sizeof(GenericAlternate)); >> if (v->start_alternate) { >> - v->start_alternate(v, name, obj, size, promote_int, errp); >> + v->start_alternate(v, name, obj, size, promote_int, &err); >> } >> + if (v->type == VISITOR_INPUT) { >> + assert(!err != !*obj); > > Could assert(v->start_alternate && !err != !*obj), to preempt "what if > !v->start_alternate" worries. If you like that, I can do it on commit. Can't hurt :)
On 04/29/2016 06:10 AM, Eric Blake wrote: > On 04/29/2016 02:28 AM, Markus Armbruster wrote: >> Eric Blake <eblake@redhat.com> writes: >> >>> Our existing input visitors were not very consistent on errors >>> in a function taking 'TYPE **obj' (that is, start_struct(), >>> start_alternate(), type_str(), and type_any(). next_list() is >>> similar, except that since commit 08f9541, it can't fail). >> >> Multiple sentences in a parenthesis, ugh :) >> >> Our existing input visitors were not very consistent on errors in a >> function taking 'TYPE **obj'. These are start_struct(), >> start_alternate(), type_str(), and type_any(). next_list() is >> similar, but can't fail (see commit since 08f9541). >> >> Can touch up on commit. >> > > Yes, sounds better. Spoke too soon: "see commit since 08f9541" is too wordy, should be either "see commit 08f9541" or "since commit 08f9541" > > >>> @@ -51,10 +57,16 @@ void visit_start_alternate(Visitor *v, const char *name, >>> GenericAlternate **obj, size_t size, >>> bool promote_int, Error **errp) >>> { >>> + Error *err = NULL; >>> + >>> assert(obj && size >= sizeof(GenericAlternate)); >>> if (v->start_alternate) { >>> - v->start_alternate(v, name, obj, size, promote_int, errp); >>> + v->start_alternate(v, name, obj, size, promote_int, &err); >>> } >>> + if (v->type == VISITOR_INPUT) { >>> + assert(!err != !*obj); >> >> Could assert(v->start_alternate && !err != !*obj), to preempt "what if >> !v->start_alternate" worries. If you like that, I can do it on commit. > > Can't hurt :) >
Eric Blake <eblake@redhat.com> writes: > On 04/29/2016 06:10 AM, Eric Blake wrote: >> On 04/29/2016 02:28 AM, Markus Armbruster wrote: >>> Eric Blake <eblake@redhat.com> writes: >>> >>>> Our existing input visitors were not very consistent on errors >>>> in a function taking 'TYPE **obj' (that is, start_struct(), >>>> start_alternate(), type_str(), and type_any(). next_list() is >>>> similar, except that since commit 08f9541, it can't fail). >>> >>> Multiple sentences in a parenthesis, ugh :) >>> >>> Our existing input visitors were not very consistent on errors in a >>> function taking 'TYPE **obj'. These are start_struct(), >>> start_alternate(), type_str(), and type_any(). next_list() is >>> similar, but can't fail (see commit since 08f9541). >>> >>> Can touch up on commit. >>> >> >> Yes, sounds better. > > Spoke too soon: "see commit since 08f9541" is too wordy, should be > either "see commit 08f9541" or "since commit 08f9541" I'm terribly prone to leaving unwanted words behind when tinkering with a sentence... Thanks! >>>> @@ -51,10 +57,16 @@ void visit_start_alternate(Visitor *v, const char *name, >>>> GenericAlternate **obj, size_t size, >>>> bool promote_int, Error **errp) >>>> { >>>> + Error *err = NULL; >>>> + >>>> assert(obj && size >= sizeof(GenericAlternate)); >>>> if (v->start_alternate) { >>>> - v->start_alternate(v, name, obj, size, promote_int, errp); >>>> + v->start_alternate(v, name, obj, size, promote_int, &err); >>>> } >>>> + if (v->type == VISITOR_INPUT) { >>>> + assert(!err != !*obj); >>> >>> Could assert(v->start_alternate && !err != !*obj), to preempt "what if >>> !v->start_alternate" worries. If you like that, I can do it on commit. >> >> Can't hurt :) Done.
diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c index 3cd7edc..7ad5ff4 100644 --- a/qapi/qapi-visit-core.c +++ b/qapi/qapi-visit-core.c @@ -23,7 +23,13 @@ void visit_start_struct(Visitor *v, const char *name, void **obj, size_t size, Error **errp) { - v->start_struct(v, name, obj, size, errp); + Error *err = NULL; + + v->start_struct(v, name, obj, size, &err); + if (obj && v->type == VISITOR_INPUT) { + assert(!err != !*obj); + } + error_propagate(errp, err); } void visit_end_struct(Visitor *v, Error **errp) @@ -51,10 +57,16 @@ void visit_start_alternate(Visitor *v, const char *name, GenericAlternate **obj, size_t size, bool promote_int, Error **errp) { + Error *err = NULL; + assert(obj && size >= sizeof(GenericAlternate)); if (v->start_alternate) { - v->start_alternate(v, name, obj, size, promote_int, errp); + v->start_alternate(v, name, obj, size, promote_int, &err); } + if (v->type == VISITOR_INPUT) { + assert(!err != !*obj); + } + error_propagate(errp, err); } void visit_end_alternate(Visitor *v) @@ -188,7 +200,14 @@ void visit_type_bool(Visitor *v, const char *name, bool *obj, Error **errp) void visit_type_str(Visitor *v, const char *name, char **obj, Error **errp) { - v->type_str(v, name, obj, errp); + Error *err = NULL; + + assert(obj); + v->type_str(v, name, obj, &err); + if (v->type == VISITOR_INPUT) { + assert(!err != !*obj); + } + error_propagate(errp, err); } void visit_type_number(Visitor *v, const char *name, double *obj, @@ -199,7 +218,14 @@ void visit_type_number(Visitor *v, const char *name, double *obj, void visit_type_any(Visitor *v, const char *name, QObject **obj, Error **errp) { - v->type_any(v, name, obj, errp); + Error *err = NULL; + + assert(obj); + v->type_any(v, name, obj, &err); + if (v->type == VISITOR_INPUT) { + assert(!err != !*obj); + } + error_propagate(errp, err); } static void output_type_enum(Visitor *v, const char *name, int *obj, diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c index 66aeaed..4cb6436 100644 --- a/qapi/opts-visitor.c +++ b/qapi/opts-visitor.c @@ -133,7 +133,7 @@ opts_start_struct(Visitor *v, const char *name, void **obj, const QemuOpt *opt; if (obj) { - *obj = g_malloc0(size > 0 ? size : 1); + *obj = g_malloc0(size); } if (ov->depth++ > 0) { return; @@ -314,6 +314,7 @@ opts_type_str(Visitor *v, const char *name, char **obj, Error **errp) opt = lookup_scalar(ov, name, errp); if (!opt) { + *obj = NULL; return; } *obj = g_strdup(opt->str ? opt->str : ""); diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c index 02d4233..77cce8b 100644 --- a/qapi/qmp-input-visitor.c +++ b/qapi/qmp-input-visitor.c @@ -120,6 +120,9 @@ static void qmp_input_start_struct(Visitor *v, const char *name, void **obj, QObject *qobj = qmp_input_get_object(qiv, name, true); Error *err = NULL; + if (obj) { + *obj = NULL; + } if (!qobj || qobject_type(qobj) != QTYPE_QDICT) { error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null", "QDict"); @@ -267,6 +270,7 @@ static void qmp_input_type_str(Visitor *v, const char *name, char **obj, QString *qstr = qobject_to_qstring(qmp_input_get_object(qiv, name, true)); if (!qstr) { + *obj = NULL; error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null", "string"); return; diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c index d604575..797973a 100644 --- a/qapi/string-input-visitor.c +++ b/qapi/string-input-visitor.c @@ -293,6 +293,7 @@ static void parse_type_str(Visitor *v, const char *name, char **obj, if (siv->string) { *obj = g_strdup(siv->string); } else { + *obj = NULL; error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null", "string"); } diff --git a/tests/test-qmp-input-strict.c b/tests/test-qmp-input-strict.c index d71727e..d5f80ec 100644 --- a/tests/test-qmp-input-strict.c +++ b/tests/test-qmp-input-strict.c @@ -263,7 +263,7 @@ static void test_validate_fail_union_flat_no_discrim(TestInputVisitorData *data, static void test_validate_fail_alternate(TestInputVisitorData *data, const void *unused) { - UserDefAlternate *tmp = NULL; + UserDefAlternate *tmp; Visitor *v; Error *err = NULL;
Our existing input visitors were not very consistent on errors in a function taking 'TYPE **obj' (that is, start_struct(), start_alternate(), type_str(), and type_any(). next_list() is similar, except that since commit 08f9541, it can't fail). While all of them set '*obj' to allocated storage on success, it was not obvious whether '*obj' was guaranteed safe on failure, or whether it was left uninitialized. But a future patch wants to guarantee that visit_type_FOO() does not leak a partially- constructed obj back to the caller; it is easier to implement this if we can reliably state that input visitors assign '*obj' regardless of success or failure, and that on failure *obj is NULL. Add assertions to enforce consistency in the final setting of err vs. *obj. The opts-visitor start_struct() doesn't set an error, but it also was doing a weird check for 0 size; all callers pass in non-zero size if obj is non-NULL. The testsuite has at least one spot where we no longer need to pre-initialize a variable prior to a visit; valgrind confirms that the test is still fine with the cleanup. A later patch will document the design constraint implemented here. Signed-off-by: Eric Blake <eblake@redhat.com> --- v16: tighten assertions, another commit message tweak v15: enhance commit message, hoist assertions from later in series v14: no change v13: no change v12: new patch --- qapi/qapi-visit-core.c | 34 ++++++++++++++++++++++++++++++---- qapi/opts-visitor.c | 3 ++- qapi/qmp-input-visitor.c | 4 ++++ qapi/string-input-visitor.c | 1 + tests/test-qmp-input-strict.c | 2 +- 5 files changed, 38 insertions(+), 6 deletions(-)