Eric Blake <ebl...@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(), next_list(), type_str(), and type_any()).
> 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 '*obj' is assigned on exit,
> even on failures. Add assertions to enforce it.
I had to read this several times, because by now I've forgotten that
we're talking about input visitors only. Easy enough to avoid: ... that
input visitors assign to *obj regardless of success or failure.
Begs the question what is assigned to it on failure, though.
>
> 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 <ebl...@redhat.com>
>
> ---
> 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..3a131ce 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);
> }
The commit message claims you're adding assertions to enforce input
visitors assign *obj even on failure. This assertion doesn't do that.
It enforces "on success, *obj is non-null". Is that what you want? Or
do you actually want something like "either err or *obj are non-null"?
I.e.
assert(!err != !*obj);
>
> void visit_end_struct(Visitor *v, Error **errp)
> @@ -51,9 +57,15 @@ 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);
> }
> }
>
Hmm, you check the postcondition only when v implements
start_alternate(). Shouldn't it hold regardless of v? If yes, then
let's check it regardless of v:
if (v->start_alternate) {
v->start_alternate(v, name, obj, size, promote_int, &err);
}
if (v->type == VISITOR_INPUT) {
assert(err || *obj);
}
error_propagate(errp, err);
But that makes it pretty obvious that the postcondition won't hold when
!v->start_alternate. May v->start_alternate() be null for an input
visitor? According to visitor-impl.h, it may not. Okay.
> @@ -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);
> }
>
The commit message lists start_struct(), start_alternate(), next_list(),
type_str(), and type_any(). You cover them except for next_list(). Why
is that missing?
> 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;
