[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v9 05/37] vl: Improve use of qapi visitor
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH v9 05/37] vl: Improve use of qapi visitor |
Date: |
Wed, 20 Jan 2016 14:43:20 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
Eric Blake <address@hidden> writes:
> Cache the visitor in a local variable instead of repeatedly
> calling the accessor. Pass NULL for the visit_start_struct()
> object (which matches the fact that we were already passing 0
> for the size argument, because we aren't using the visit to
> allocate a qapi struct). Guarantee that visit_end_struct()
> is called if visit_start_struct() succeeded.
Three separate things --- you're pushing it now :)
Impact of not calling visit_end_struct()?
> Signed-off-by: Eric Blake <address@hidden>
> Reviewed-by: Marc-André Lureau <address@hidden>
>
> ---
> v9: no change
> v8: no change
> v7: place earlier in series, drop attempts to provide a 'kind' string,
> drop bogus avoidance of qmp_object_del() on error
> v6: new patch, split from RFC on v5 7/46
> ---
> vl.c | 26 ++++++++++++++------------
> 1 file changed, 14 insertions(+), 12 deletions(-)
>
> diff --git a/vl.c b/vl.c
> index f043009..aaa5403 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2822,44 +2822,47 @@ static bool object_create_delayed(const char *type)
> static int object_create(void *opaque, QemuOpts *opts, Error **errp)
> {
> Error *err = NULL;
> + Error *err_end = NULL;
> char *type = NULL;
> char *id = NULL;
> - void *dummy = NULL;
> OptsVisitor *ov;
> QDict *pdict;
> bool (*type_predicate)(const char *) = opaque;
> + Visitor *v;
>
> ov = opts_visitor_new(opts);
> pdict = qemu_opts_to_qdict(opts, NULL);
> + v = opts_get_visitor(ov);
>
> - visit_start_struct(opts_get_visitor(ov), &dummy, NULL, NULL, 0, &err);
> + visit_start_struct(v, NULL, NULL, NULL, 0, &err);
Same argument as for the previous patch.
> if (err) {
> goto out;
> }
>
> qdict_del(pdict, "qom-type");
> - visit_type_str(opts_get_visitor(ov), &type, "qom-type", &err);
> + visit_type_str(v, &type, "qom-type", &err);
> if (err) {
> goto out;
> }
If we get here, we must call visit_end_struct().
> if (!type_predicate(type)) {
> + visit_end_struct(v, NULL);
Here, you add the previously missing visit_end_struct() to the error
path.
> goto out;
> }
>
> qdict_del(pdict, "id");
> - visit_type_str(opts_get_visitor(ov), &id, "id", &err);
> + visit_type_str(v, &id, "id", &err);
> if (err) {
> - goto out;
> + goto out_end;
Here, you skip to the function's cleanup, which now includes
visit_end_struct().
Such a mix is can be a sign the cleanup isn't quite in the right order.
When we take this error path to out_end, then:
out_end:
visit_end_struct(v, &err_end); // called, as we must
if (!err && err_end) { // !err is false
qmp_object_del(id, NULL); // not called
}
error_propagate(&err, err_end); // since err, this is
// error_free(err_end)
> }
>
> - object_add(type, id, pdict, opts_get_visitor(ov), &err);
> - if (err) {
> - goto out;
> - }
> - visit_end_struct(opts_get_visitor(ov), &err);
> - if (err) {
> + object_add(type, id, pdict, v, &err);
> +
> +out_end:
> + visit_end_struct(v, &err_end);
visit_end_struct() must be called when visit_start_struct() succeeded.
All error paths after that success pass through here, except for one,
and that one calls visit_end_struct(). Okay.
> + if (!err && err_end) {
> qmp_object_del(id, NULL);
> }
qmp_object_del() must be called when we fail after object_add()
succeeded. That's what the condition does.
> + error_propagate(&err, err_end);
>
> out:
Cleanup below here must be done always.
> opts_visitor_cleanup(ov);
The only reason I'm not asking you to rewrite this mess is that you're
already rewriting it later in this series.
> @@ -2867,7 +2870,6 @@ out:
> QDECREF(pdict);
> g_free(id);
> g_free(type);
> - g_free(dummy);
> if (err) {
> error_report_err(err);
> return -1;
I wonder whether splitting this and the previous patch along the change
rather than the file would come out nicer:
1. Cache the visitor
2. Suppress the pointless allocation
3. Fix to call visit_end_struct()
[Qemu-devel] [PATCH v9 03/37] qapi: Drop dead dealloc visitor variable, Eric Blake, 2016/01/19
[Qemu-devel] [PATCH v9 01/37] qobject: Document more shortcomings in our number handling, Eric Blake, 2016/01/19
[Qemu-devel] [PATCH v9 13/37] qom: Use typedef for Visitor, Eric Blake, 2016/01/19