Eric Blake <ebl...@redhat.com> writes: > On 01/20/2016 06:43 AM, Markus Armbruster wrote: >> Eric Blake <ebl...@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 :)