| Message ID | 1453219845-30939-6-git-send-email-eblake@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Eric Blake <eblake@redhat.com> 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 <eblake@redhat.com> > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > --- > 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()
On 01/20/2016 06:43 AM, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> 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 :) Two of them the same as in 4/37. So, among the five items, I did a split in two based on file. I could split in the other direction: fix hmp and vl to cache their visitor fix hmp and vl to pass NULL to avoid pointless allocation fix vl to guarantee visit_end_struct > > Impact of not calling visit_end_struct()? Assertion failures after patch 24. :) Basically, I wanted to see whether the code base could get simpler if we always enforced visit start/end grouping, and there were only a couple of outliers that needed fixing (here, and in patch 7). >> >> 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) Correct. And it gets simpler later on, when visit_end_struct() loses the errp argument (patch 33). > >> } >> >> - 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. So I think you found patch 33. > >> @@ -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() > What do you know - we're thinking on the same lines :) [And now you know I just replied to your email in the order that I read it, rather than reading the whole thing first]
Eric Blake <eblake@redhat.com> writes: > On 01/20/2016 06:43 AM, Markus Armbruster wrote: >> Eric Blake <eblake@redhat.com> 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 :) > > Two of them the same as in 4/37. > > So, among the five items, I did a split in two based on file. I could > split in the other direction: > > fix hmp and vl to cache their visitor > fix hmp and vl to pass NULL to avoid pointless allocation > fix vl to guarantee visit_end_struct > >> >> Impact of not calling visit_end_struct()? > > Assertion failures after patch 24. :) > Basically, I wanted to see whether the code base could get simpler if we > always enforced visit start/end grouping, and there were only a couple > of outliers that needed fixing (here, and in patch 7). Makes sense, although I wonder why things work without visit_end_struct(). Looking... I guess we skip visit_end_struct() only on error, and then we go straight to opts_visitor_cleanup(). Okay, that's sane enough to work. But now I wonder whether requiring visit_end_struct() before visit_cleanup() is a good idea. >>> >>> 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) > > Correct. And it gets simpler later on, when visit_end_struct() loses the > errp argument (patch 33). > >> >>> } >>> >>> - 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. > > So I think you found patch 33. > >> >>> @@ -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() >> > > What do you know - we're thinking on the same lines :) [And now you > know I just replied to your email in the order that I read it, rather > than reading the whole thing first] I do that all the time for long messages :)
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); 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 (!type_predicate(type)) { + visit_end_struct(v, NULL); 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; } - 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); + if (!err && err_end) { qmp_object_del(id, NULL); } + error_propagate(&err, err_end); out: opts_visitor_cleanup(ov); @@ -2867,7 +2870,6 @@ out: QDECREF(pdict); g_free(id); g_free(type); - g_free(dummy); if (err) { error_report_err(err); return -1;