[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RFC PATCH] qapi: split visit_end_struct() into pieces
|
From: |
Markus Armbruster |
|
Subject: |
Re: [Qemu-devel] [RFC PATCH] qapi: split visit_end_struct() into pieces |
|
Date: |
Wed, 07 Oct 2015 14:00:37 +0200 |
|
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
Eric Blake <address@hidden> writes:
> We want to call the various visit_end_*() functions unconditionally,
> so that visitors can release resources tied up since the matching
> visit_start_*(). But we also have a requirement for detecting when
> an input visitor did not consume everything, so the code allowed
> visit_end_*() to set an error. This led to some awkward coding,
> since safely setting the error, even if an earlier one had
> occurred, while still favoring the earliest error, requires either
> multiple error_propagate() calls (and an understanding that unlike
> error_setg(), it is safe to propagate onto an already-set error),
> or else ternaries such as foo(..., err ? NULL : &err) to ignore
> secondary errors if an earlier error is already known.
>
> Splitting the functionality allows slightly cleaner idioms. Now,
> clients must call visit_check_struct() if all earlier actions
> have been successful for one last chance at flagging excess
> input, while visit_end_struct() is unconditional and does not
> set any further errors.
>
> As it is an API change, everything must be changed in one commit
> (there is no way to break this into smaller pieces): both the
> generated code and other existing clients, as well as the various
> visitor backends.
>
> Signed-off-by: Eric Blake <address@hidden>
>
> ---
> TODO: There is some redundancy in the testsuite, with multiple
> files hand-writing their own visit_type_TestStruct() instead
> of letting the qapi generators create it. Should this be
> consolidated as a separate patch?
If generating makes sense, it should be a separate patch. Whether it
makes sense I can't say until I see the patch.
> Probably won't apply directly to any published commit, so much
> as me posting a part of my worktree early to get feedback on
> whether the idea even makes sense, given how large the results
> end up being.
>
> v6: new patch, based on discussion of v5 7/46:
>
> On 09/28/2015 03:14 AM, Markus Armbruster wrote:
>
>>> One other potential alternative: What if we split visit_end_struct()
>>> into two visitor functions, one that checks for success, and the other
>>> that is called unconditionally to clean up resources.
>
>> I think this split could help with writing safe code: in
>> visit_check_struct() you can rely on "no error so far", as usual. In
>> visit_end_struct(), you can't, but it should be a pure cleanup function,
>> where that's quite normal.
>>
>> Looks like we're getting drawn into visitor contract territory again.
>>
>
> ---
> hmp.c | 3 ++-
> hw/virtio/virtio-balloon.c | 14 ++++++++------
> include/qapi/visitor-impl.h | 8 +++++---
> include/qapi/visitor.h | 24 ++++++++++++++++++------
> qapi/opts-visitor.c | 17 +++++++++++++++--
> qapi/qapi-dealloc-visitor.c | 6 +++---
> qapi/qapi-visit-core.c | 19 +++++++++++++------
> qapi/qmp-input-visitor.c | 36 +++++++++++++++++++++++++-----------
> qapi/qmp-output-visitor.c | 4 ++--
> qapi/string-input-visitor.c | 2 +-
> qapi/string-output-visitor.c | 2 +-
> qom/object.c | 5 ++---
> scripts/qapi-event.py | 5 +++--
> scripts/qapi-visit.py | 24 ++++++++++++------------
> tests/test-qmp-input-strict.c | 9 +++++----
> tests/test-qmp-input-visitor.c | 9 +++++----
> tests/test-qmp-output-visitor.c | 23 ++++++++++++++++-------
> tests/test-visitor-serialization.c | 9 +++++----
> vl.c | 12 +++++++-----
> 19 files changed, 148 insertions(+), 83 deletions(-)
>
> diff --git a/hmp.c b/hmp.c
> index 5048eee..b7a42dc 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -1658,8 +1658,9 @@ void hmp_object_add(Monitor *mon, const QDict *qdict)
>
> object_add(type, id, pdict, opts_get_visitor(ov), &err);
>
> + visit_check_struct(opts_get_visitor(ov), &err_end);
> out_end:
> - visit_end_struct(opts_get_visitor(ov), &err_end);
> + visit_end_struct(opts_get_visitor(ov));
> if (!err && err_end) {
> qmp_object_del(id, NULL);
> }
Preexisting: calling object_add() before visit_end_struct() is awkward.
Can we simplify things now we have separate visit_check_struct() and
visit_end_struct()? Call visit_check_struct() before object_add(),
bypass object_add() on error, avoiding the need to undo it with
qmp_object_del().
> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> index c419b17..48867c4 100644
> --- a/hw/virtio/virtio-balloon.c
> +++ b/hw/virtio/virtio-balloon.c
> @@ -132,14 +132,16 @@ static void balloon_stats_get_all(Object *obj, struct
> Visitor *v,
visit_start_struct(v, NULL, NULL, "stats", 0, &err);
if (err) {
goto out_end;
}
for (i = 0; !err && i < VIRTIO_BALLOON_S_NR; i++) {
> visit_type_int64(v, (int64_t *) &s->stats[i], balloon_stat_names[i],
> &err);
Preexisting: if visit_type_int64() fails, we need to break the loop.
Separate fix.
I wonder whether this is a common mistake. Perhaps I can search for it
with Coccinelle.
> }
> - error_propagate(errp, err);
> - err = NULL;
> - visit_end_struct(v, &err);
> + if (!err) {
> + visit_check_struct(v, &err);
> + }
> + visit_end_struct(v);
>
> out_end:
> - error_propagate(errp, err);
> - err = NULL;
> - visit_end_struct(v, &err);
> + if (!err) {
> + visit_check_struct(v, &err);
> + }
> + visit_end_struct(v);
> out:
> error_propagate(errp, err);
> }
This is the obvious mechanical change.
Loop fix and visit_end_struct() split squashed together could look like
this (not even compile-tested):
for (i = 0; !err && i < VIRTIO_BALLOON_S_NR; i++) {
visit_type_int64(v, (int64_t *) &s->stats[i], balloon_stat_names[i],
&err);
if (err) {
goto out_inner_end;
}
}
visit_check_struct(v, &err);
out_inner_end:
visit_end_struct(v);
if (!err) {
visit_check_struct(v, &err);
}
out_end:
visit_end_struct(v);
out:
error_propagate(errp, err);
> diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
> index 090b19b..c53d4d3 100644
> --- a/include/qapi/visitor-impl.h
> +++ b/include/qapi/visitor-impl.h
> @@ -28,21 +28,23 @@ struct Visitor
> * currently visit structs). */
> void (*start_struct)(Visitor *v, void **obj, const char *kind,
> const char *name, size_t size, Error **errp);
> + /* May be NULL; most useful for input visitors. */
> + void (*check_struct)(Visitor *v, Error **errp);
> /* Must be provided if start_struct is present. */
> - void (*end_struct)(Visitor *v, Error **errp);
> + void (*end_struct)(Visitor *v);
>
> /* May be NULL; most useful for input visitors. */
> void (*start_implicit_struct)(Visitor *v, void **obj, size_t size,
> Error **errp);
> /* May be NULL */
> - void (*end_implicit_struct)(Visitor *v, Error **errp);
> + void (*end_implicit_struct)(Visitor *v);
>
> /* Must be set */
> void (*start_list)(Visitor *v, const char *name, Error **errp);
> /* Must be set */
> GenericList *(*next_list)(Visitor *v, GenericList **list, Error **errp);
> /* Must be set */
> - void (*end_list)(Visitor *v, Error **errp);
> + void (*end_list)(Visitor *v);
>
> /* Must be set, although the helpers input_type_enum() and
> * output_type_enum() can be used. */
> diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
> index 6534cca..708f55c 100644
> --- a/include/qapi/visitor.h
> +++ b/include/qapi/visitor.h
> @@ -56,11 +56,19 @@ typedef struct GenericList
> void visit_start_struct(Visitor *v, void **obj, const char *kind,
> const char *name, size_t size, Error **errp);
> /**
> + * Check whether completing a struct is safe.
"Safe"? We need to complete the struct visit with visit_end_struct()
regardless of what this function returns...
> + * Should be called prior to visit_end_struct() if all other intermediate
> + * visit steps were successful, to allow the caller one last chance to
> + * report errors such as remaining data that was not consumed by the visit.
> + */
> +void visit_check_struct(Visitor *v, Error **errp);
> +/**
> * Complete a struct visit started earlier.
> * Must be called after any successful use of visit_start_struct(),
> - * even if intermediate processing was skipped due to errors.
> + * even if intermediate processing was skipped due to errors, to allow
> + * the backend to release any resources.
> */
> -void visit_end_struct(Visitor *v, Error **errp);
> +void visit_end_struct(Visitor *v);
>
> /**
> * Prepare to visit an implicit struct.
> @@ -76,9 +84,12 @@ void visit_start_implicit_struct(Visitor *v, void **obj,
> size_t size,
> /**
> * Complete an implicit struct visit started earlier.
> * Must be called after any successful use of visit_start_implicit_struct(),
> - * even if intermediate processing was skipped due to errors.
> + * even if intermediate processing was skipped due to errors. Unlike
> + * visit_end_struct(), there is no need for a prior check call (since
> + * an implicit object is a subset of larger object, checking before ending
> + * the overall struct is sufficient).
> */
> -void visit_end_implicit_struct(Visitor *v, Error **errp);
> +void visit_end_implicit_struct(Visitor *v);
>
> /**
> * Prepare to visit an array tied to an object key @name.
> @@ -101,9 +112,10 @@ GenericList *visit_next_list(Visitor *v, GenericList
> **list, Error **errp);
> /**
> * Complete the list started earlier.
> * Must be called after any successful use of visit_start_list(),
> - * even if intermediate processing was skipped due to errors.
> + * even if intermediate processing was skipped due to errors, to allow
> + * the backend to release any resources.
> */
> -void visit_end_list(Visitor *v, Error **errp);
> +void visit_end_list(Visitor *v);
>
> /**
> * Check if an optional member @name of an object needs visiting.
> diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
> index cd10392..39dc431 100644
> --- a/qapi/opts-visitor.c
> +++ b/qapi/opts-visitor.c
> @@ -158,7 +158,7 @@ ghr_true(gpointer ign_key, gpointer ign_value, gpointer
> ign_user_data)
>
>
> static void
> -opts_end_struct(Visitor *v, Error **errp)
> +opts_check_struct(Visitor *v, Error **errp)
> {
> OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
> GQueue *any;
if (--ov->depth > 0) {
Do we want to decrement ov->depth here? We'll decrement it again in
opts_end_struct()...
return;
}
/* we should have processed all (distinct) QemuOpt instances */
any = g_hash_table_find(ov->unprocessed_opts, &ghr_true, NULL);
if (any) {
const QemuOpt *first;
> @@ -175,6 +175,18 @@ opts_end_struct(Visitor *v, Error **errp)
> first = g_queue_peek_head(any);
> error_setg(errp, QERR_INVALID_PARAMETER, first->name);
> }
> +}
> +
> +
> +static void
> +opts_end_struct(Visitor *v)
> +{
> + OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
> +
> + if (--ov->depth > 0) {
> + return;
> + }
> +
> g_hash_table_destroy(ov->unprocessed_opts);
> ov->unprocessed_opts = NULL;
> if (ov->fake_id_opt) {
> @@ -263,7 +275,7 @@ opts_next_list(Visitor *v, GenericList **list, Error
> **errp)
>
>
> static void
> -opts_end_list(Visitor *v, Error **errp)
> +opts_end_list(Visitor *v)
> {
> OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
>
> @@ -506,6 +518,7 @@ opts_visitor_new(const QemuOpts *opts)
> ov = g_malloc0(sizeof *ov);
>
> ov->visitor.start_struct = &opts_start_struct;
> + ov->visitor.check_struct = &opts_check_struct;
> ov->visitor.end_struct = &opts_end_struct;
>
> ov->visitor.start_list = &opts_start_list;
> diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c
> index 737deab..a2139dd 100644
> --- a/qapi/qapi-dealloc-visitor.c
> +++ b/qapi/qapi-dealloc-visitor.c
> @@ -67,7 +67,7 @@ static void qapi_dealloc_start_struct(Visitor *v, void
> **obj, const char *kind,
> qapi_dealloc_push(qov, obj);
> }
>
> -static void qapi_dealloc_end_struct(Visitor *v, Error **errp)
> +static void qapi_dealloc_end_struct(Visitor *v)
> {
> QapiDeallocVisitor *qov = to_qov(v);
> void **obj = qapi_dealloc_pop(qov);
> @@ -85,7 +85,7 @@ static void qapi_dealloc_start_implicit_struct(Visitor *v,
> qapi_dealloc_push(qov, obj);
> }
>
> -static void qapi_dealloc_end_implicit_struct(Visitor *v, Error **errp)
> +static void qapi_dealloc_end_implicit_struct(Visitor *v)
> {
> QapiDeallocVisitor *qov = to_qov(v);
> void **obj = qapi_dealloc_pop(qov);
> @@ -121,7 +121,7 @@ static GenericList *qapi_dealloc_next_list(Visitor *v,
> GenericList **listp,
> return NULL;
> }
>
> -static void qapi_dealloc_end_list(Visitor *v, Error **errp)
> +static void qapi_dealloc_end_list(Visitor *v)
> {
> QapiDeallocVisitor *qov = to_qov(v);
> void *obj = qapi_dealloc_pop(qov);
> diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
> index cbf7780..82d54af 100644
> --- a/qapi/qapi-visit-core.c
> +++ b/qapi/qapi-visit-core.c
> @@ -23,9 +23,16 @@ void visit_start_struct(Visitor *v, void **obj, const char
> *kind,
> v->start_struct(v, obj, kind, name, size, errp);
> }
>
> -void visit_end_struct(Visitor *v, Error **errp)
> +void visit_check_struct(Visitor *v, Error **errp)
> {
> - v->end_struct(v, errp);
> + if (v->check_struct) {
> + v->check_struct(v, errp);
> + }
> +}
> +
> +void visit_end_struct(Visitor *v)
> +{
> + v->end_struct(v);
> }
>
> void visit_start_implicit_struct(Visitor *v, void **obj, size_t size,
> @@ -36,10 +43,10 @@ void visit_start_implicit_struct(Visitor *v, void **obj,
> size_t size,
> }
> }
>
> -void visit_end_implicit_struct(Visitor *v, Error **errp)
> +void visit_end_implicit_struct(Visitor *v)
> {
> if (v->end_implicit_struct) {
> - v->end_implicit_struct(v, errp);
> + v->end_implicit_struct(v);
> }
> }
>
> @@ -53,9 +60,9 @@ GenericList *visit_next_list(Visitor *v, GenericList
> **list, Error **errp)
> return v->next_list(v, list, errp);
> }
>
> -void visit_end_list(Visitor *v, Error **errp)
> +void visit_end_list(Visitor *v)
> {
> - v->end_list(v, errp);
> + v->end_list(v);
> }
>
> bool visit_start_union(Visitor *v, bool data_present, Error **errp)
> diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
> index 5310db5..f10f74f 100644
> --- a/qapi/qmp-input-visitor.c
> +++ b/qapi/qmp-input-visitor.c
> @@ -88,14 +88,14 @@ static void qmp_input_push(QmpInputVisitor *qiv, QObject
> *obj, Error **errp)
> qiv->nb_stack++;
> }
>
> -/** Only for qmp_input_pop. */
> +/** Only for qmp_input_check. */
Drop the comment instead?
Aside: a loop would be more idiomatic C. Leave higher order functions
to languages that are actually equipped for the job.
> static gboolean always_true(gpointer key, gpointer val, gpointer user_pkey)
> {
> *(const char **)user_pkey = (const char *)key;
> return TRUE;
> }
>
> -static void qmp_input_pop(QmpInputVisitor *qiv, Error **errp)
> +static void qmp_input_check(QmpInputVisitor *qiv, Error **errp)
> {
> assert(qiv->nb_stack > 0);
>
> @@ -107,6 +107,17 @@ static void qmp_input_pop(QmpInputVisitor *qiv, Error
> **errp)
> g_hash_table_find(top_ht, always_true, &key);
> error_setg(errp, QERR_QMP_EXTRA_MEMBER, key);
> }
> + }
> + }
> +}
> +
> +static void qmp_input_pop(QmpInputVisitor *qiv)
> +{
> + assert(qiv->nb_stack > 0);
> +
> + if (qiv->strict) {
> + GHashTable * const top_ht = qiv->stack[qiv->nb_stack - 1].h;
> + if (top_ht) {
> g_hash_table_unref(top_ht);
> }
> }
> @@ -138,11 +149,18 @@ static void qmp_input_start_struct(Visitor *v, void
> **obj, const char *kind,
> }
> }
>
> -static void qmp_input_end_struct(Visitor *v, Error **errp)
> +static void qmp_input_check_struct(Visitor *v, Error **errp)
> {
> QmpInputVisitor *qiv = to_qiv(v);
>
> - qmp_input_pop(qiv, errp);
> + qmp_input_check(qiv, errp);
qmp_input_check(to_qiv(v));
Alternatively, inline, as this is the only caller.
> +}
> +
> +static void qmp_input_end_struct(Visitor *v)
> +{
> + QmpInputVisitor *qiv = to_qiv(v);
> +
> + qmp_input_pop(qiv);
qmp_input_pop(to_qiv(v));
Since all callers pass to_qiv(v), we could move that into the callee.
Same for qmp_input_check() then.
> }
>
> static void qmp_input_start_implicit_struct(Visitor *v, void **obj,
> @@ -153,10 +171,6 @@ static void qmp_input_start_implicit_struct(Visitor *v,
> void **obj,
> }
> }
>
> -static void qmp_input_end_implicit_struct(Visitor *v, Error **errp)
> -{
> -}
> -
Further evidence of confusion about the visitor contract.
> static void qmp_input_start_list(Visitor *v, const char *name, Error **errp)
> {
> QmpInputVisitor *qiv = to_qiv(v);
> @@ -201,11 +215,11 @@ static GenericList *qmp_input_next_list(Visitor *v,
> GenericList **list,
> return entry;
> }
>
> -static void qmp_input_end_list(Visitor *v, Error **errp)
> +static void qmp_input_end_list(Visitor *v)
> {
> QmpInputVisitor *qiv = to_qiv(v);
>
> - qmp_input_pop(qiv, errp);
> + qmp_input_pop(qiv);
> }
Identical to qmp_input_end_struct() now. If we move to_qiv() into
qmp_input_pop(), we can use it as callback directly, and drop the two
wrappers.
>
> static void qmp_input_get_next_type(Visitor *v, qtype_code *type,
> @@ -332,9 +346,9 @@ QmpInputVisitor *qmp_input_visitor_new(QObject *obj)
> v = g_malloc0(sizeof(*v));
>
> v->visitor.start_struct = qmp_input_start_struct;
> + v->visitor.check_struct = qmp_input_check_struct;
> v->visitor.end_struct = qmp_input_end_struct;
> v->visitor.start_implicit_struct = qmp_input_start_implicit_struct;
> - v->visitor.end_implicit_struct = qmp_input_end_implicit_struct;
> v->visitor.start_list = qmp_input_start_list;
> v->visitor.next_list = qmp_input_next_list;
> v->visitor.end_list = qmp_input_end_list;
> diff --git a/qapi/qmp-output-visitor.c b/qapi/qmp-output-visitor.c
> index f891e72..b9fba7b 100644
> --- a/qapi/qmp-output-visitor.c
> +++ b/qapi/qmp-output-visitor.c
> @@ -122,7 +122,7 @@ static void qmp_output_start_struct(Visitor *v, void
> **obj, const char *kind,
> qmp_output_push(qov, dict);
> }
>
> -static void qmp_output_end_struct(Visitor *v, Error **errp)
> +static void qmp_output_end_struct(Visitor *v)
> {
> QmpOutputVisitor *qov = to_qov(v);
> qmp_output_pop(qov);
> @@ -153,7 +153,7 @@ static GenericList *qmp_output_next_list(Visitor *v,
> GenericList **listp,
> return list ? list->next : NULL;
> }
>
> -static void qmp_output_end_list(Visitor *v, Error **errp)
> +static void qmp_output_end_list(Visitor *v)
> {
> QmpOutputVisitor *qov = to_qov(v);
> qmp_output_pop(qov);
> diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
> index bbd6a54..97e00c8 100644
> --- a/qapi/string-input-visitor.c
> +++ b/qapi/string-input-visitor.c
> @@ -173,7 +173,7 @@ next_list(Visitor *v, GenericList **list, Error **errp)
> }
>
> static void
> -end_list(Visitor *v, Error **errp)
> +end_list(Visitor *v)
> {
> StringInputVisitor *siv = DO_UPCAST(StringInputVisitor, visitor, v);
> siv->head = true;
> diff --git a/qapi/string-output-visitor.c b/qapi/string-output-visitor.c
> index b86ce2c..b269f39 100644
> --- a/qapi/string-output-visitor.c
> +++ b/qapi/string-output-visitor.c
> @@ -290,7 +290,7 @@ next_list(Visitor *v, GenericList **list, Error **errp)
> }
>
> static void
> -end_list(Visitor *v, Error **errp)
> +end_list(Visitor *v)
> {
> StringOutputVisitor *sov = DO_UPCAST(StringOutputVisitor, visitor, v);
>
> diff --git a/qom/object.c b/qom/object.c
> index 11cd86b..6d92e01 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -1844,10 +1844,9 @@ static void property_get_tm(Object *obj, Visitor *v,
> void *opaque,
> if (err) {
> goto out_end;
> }
> + visit_check_struct(v, &err);
> out_end:
> - error_propagate(errp, err);
> - err = NULL;
> - visit_end_struct(v, errp);
> + visit_end_struct(v);
> out:
> error_propagate(errp, err);
>
By now the improvement is obvious: the users of this interface get nicer
pretty consistently.
> diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py
> index 720486f..250e753 100644
> --- a/scripts/qapi-event.py
> +++ b/scripts/qapi-event.py
> @@ -73,13 +73,14 @@ def gen_event_send(name, arg_type):
> ret += gen_err_check()
> ret += gen_visit_fields(arg_type.members, need_cast=True)
> ret += mcgen('''
> - visit_end_struct(v, &err);
> + visit_check_struct(v, &err);
> if (err) {
> goto out;
> }
> + visit_end_struct(v);
>
> obj = qmp_output_get_qobject(qov);
> - g_assert(obj != NULL);
> + g_assert(obj);
I prefer the more laconic form myself, but it's an unrelated change.
>
> qdict_put_obj(qmp, "data", obj);
> ''')
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index 1ac5350..c5a32cf 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -52,7 +52,7 @@ static void visit_type_implicit_%(c_type)s(Visitor *v,
> %(c_type)s **obj, Error *
> visit_start_implicit_struct(v, (void **)obj, sizeof(%(c_type)s), &err);
> if (!err) {
> visit_type_%(c_type)s_fields(v, obj, errp);
> - visit_end_implicit_struct(v, &err);
> + visit_end_implicit_struct(v);
> }
> error_propagate(errp, err);
> }
> @@ -115,9 +115,12 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj,
> const char *name, Error
> visit_start_struct(v, (void **)obj, "%(name)s", name,
> sizeof(%(c_name)s), &err);
> if (!err) {
> if (*obj) {
> - visit_type_%(c_name)s_fields(v, obj, errp);
> + visit_type_%(c_name)s_fields(v, obj, &err);
> }
> - visit_end_struct(v, &err);
> + if (!err) {
> + visit_check_struct(v, &err);
> + }
> + visit_end_struct(v);
> }
> error_propagate(errp, err);
> }
> @@ -147,9 +150,7 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj,
> const char *name, Error
> visit_type_%(c_elt_type)s(v, &native_i->value, NULL, &err);
> }
>
> - error_propagate(errp, err);
> - err = NULL;
> - visit_end_list(v, &err);
> + visit_end_list(v);
> out:
> error_propagate(errp, err);
> }
> @@ -208,9 +209,7 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj,
> const char *name, Error
> "%(name)s");
> }
> out_obj:
> - error_propagate(errp, err);
> - err = NULL;
> - visit_end_implicit_struct(v, &err);
> + visit_end_implicit_struct(v);
> out:
> error_propagate(errp, err);
> }
> @@ -297,13 +296,14 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s
> **obj, const char *name, Error
> default:
> abort();
> }
> + if (!err) {
> + visit_check_struct(v, &err);
> + }
> out_obj:
> error_propagate(errp, err);
> err = NULL;
> visit_end_union(v, !!(*obj)->data, &err);
Should visit_end_union() be similarly split? Or should its Error **
parameter be dropped? As far as I can tell, no visitor implements this
method...
> - error_propagate(errp, err);
> - err = NULL;
> - visit_end_struct(v, &err);
> + visit_end_struct(v);
> out:
> error_propagate(errp, err);
> }
> diff --git a/tests/test-qmp-input-strict.c b/tests/test-qmp-input-strict.c
> index 1c13c8f..da9dafd 100644
> --- a/tests/test-qmp-input-strict.c
> +++ b/tests/test-qmp-input-strict.c
> @@ -118,11 +118,12 @@ static void visit_type_TestStruct(Visitor *v,
> TestStruct **obj,
> goto out_end;
> }
> visit_type_str(v, &(*obj)->string, "string", &err);
> -
> + if (err) {
> + goto out_end;
> + }
> + visit_check_struct(v, &err);
> out_end:
> - error_propagate(errp, err);
> - err = NULL;
> - visit_end_struct(v, &err);
> + visit_end_struct(v);
> out:
> error_propagate(errp, err);
> }
> diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c
> index ef5871b..38b1436 100644
> --- a/tests/test-qmp-input-visitor.c
> +++ b/tests/test-qmp-input-visitor.c
> @@ -200,11 +200,12 @@ static void visit_type_TestStruct(Visitor *v,
> TestStruct **obj,
> goto out_end;
> }
> visit_type_str(v, &(*obj)->string, "string", &err);
> -
> + if (err) {
> + goto out_end;
> + }
> + visit_check_struct(v, &err);
Here's you goto over visit_check_struct()...
> out_end:
> - error_propagate(errp, err);
> - err = NULL;
> - visit_end_struct(v, &err);
> + visit_end_struct(v);
> out:
> error_propagate(errp, err);
> }
> diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c
> index 11996d0..f836588 100644
> --- a/tests/test-qmp-output-visitor.c
> +++ b/tests/test-qmp-output-visitor.c
> @@ -194,10 +194,11 @@ static void visit_type_TestStruct(Visitor *v,
> TestStruct **obj,
> }
> visit_type_str(v, &(*obj)->string, "string", &err);
>
> + if (!err) {
> + visit_check_struct(v, &err);
> + }
... but here, you guard it with an if. Either way works, but I'd like
us to pick just one for the generators.
> out_end:
> - error_propagate(errp, err);
> - err = NULL;
> - visit_end_struct(v, &err);
> + visit_end_struct(v);
> out:
> error_propagate(errp, err);
> }
> @@ -329,15 +330,23 @@ static void visit_type_TestStructList(Visitor *v,
> TestStructList **obj,
> const char *name, Error **errp)
> {
> GenericList *i, **head = (GenericList **)obj;
> + Error *err = NULL;
>
> - visit_start_list(v, name, errp);
> + visit_start_list(v, name, &err);
> + if (err) {
> + goto out;
> + }
>
> - for (*head = i = visit_next_list(v, head, errp); i; i =
> visit_next_list(v, &i, errp)) {
> + for (*head = i = visit_next_list(v, head, &err);
> + !err && i;
> + i = visit_next_list(v, &i, &err)) {
> TestStructList *native_i = (TestStructList *)i;
> - visit_type_TestStruct(v, &native_i->value, NULL, errp);
> + visit_type_TestStruct(v, &native_i->value, NULL, &err);
> }
Is this a silent bug fix? Before your patch, the loop doesn't break on
error.
I'm not afraid of complex for expressions, but I think this one is
unnecessarily complex.
Why does visit_type_TestStruct() store to *head? All that buys us is
less obviously correct list cleanup in test_visitor_out_list().
Testing err in the for expression works, but the admittedly more verbose
visit_type_TestStruct(v, &native_i->value, NULL, &err);
if (err) {
break; // or goto wherever
}
would be closer to common Error usage. Matter of taste.
Aside: the caller creates identical list elements, which weakens the
test needlessly.
>
> - visit_end_list(v, errp);
> + visit_end_list(v);
> +out:
> + error_propagate(errp, err);
> }
>
> static void test_visitor_out_list(TestOutputVisitorData *data,
> diff --git a/tests/test-visitor-serialization.c
> b/tests/test-visitor-serialization.c
> index fa86cae..dd50d69 100644
> --- a/tests/test-visitor-serialization.c
> +++ b/tests/test-visitor-serialization.c
> @@ -212,11 +212,12 @@ static void visit_type_TestStruct(Visitor *v,
> TestStruct **obj,
> goto out_end;
> }
> visit_type_str(v, &(*obj)->string, "string", &err);
> -
> + if (err) {
> + goto out_end;
> + }
> + visit_check_struct(v, &err);
> out_end:
> - error_propagate(errp, err);
> - err = NULL;
> - visit_end_struct(v, &err);
> + visit_end_struct(v);
> out:
> error_propagate(errp, err);
> }
> diff --git a/vl.c b/vl.c
> index 8d1846c..8b944aa 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2796,23 +2796,25 @@ static int object_create(void *opaque, QemuOpts
> *opts, Error **errp)
> qdict_del(pdict, "qom-type");
> visit_type_str(opts_get_visitor(ov), &type, "qom-type", &err);
> if (err) {
> - goto out;
> + goto obj_out;
> }
> if (!type_predicate(type)) {
> - goto out;
> + goto obj_out;
> }
>
> qdict_del(pdict, "id");
> visit_type_str(opts_get_visitor(ov), &id, "id", &err);
> if (err) {
> - goto out;
> + goto obj_out;
> }
>
> object_add(type, id, pdict, opts_get_visitor(ov), &err);
> if (err) {
> - goto out;
> + goto obj_out;
> }
> - visit_end_struct(opts_get_visitor(ov), &err);
> + visit_check_struct(opts_get_visitor(ov), &err);
> +obj_out:
> + visit_end_struct(opts_get_visitor(ov));
> if (err) {
> qmp_object_del(id, NULL);
> }
Silent bug fix: we now call visit_end_struct() even on error. Impact?
Separate patch?