On 01/21/2016 06:58 AM, Markus Armbruster wrote: > Eric Blake <[email protected]> writes: > >> The previous commit documented an inconsistency in how we are >> using the stack of qmp-output-visitor. Normally, pushing a >> single top-level object puts the object on the stack twice: >> once as the root, and once as the current container being >> appended to; but popping that struct only pops once. However, >> qmp_ouput_add() was trying to either set up the added object >> as the new root (works if you parse two top-level scalars in a >> row: the second replaces the first as the root) or as a member >> of the current container (works as long as you have an open >> container on the stack; but if you have popped the first >> top-level container, it then resolves to the root and still >> tries to add into that existing container). >> >> Fix the stupidity by not tracking two separate things in the >> stack. Drop the now-useless qmp_output_first() while at it. >> >> Saved for a later patch: we still are rather sloppy in that >> qmp_output_get_object() can be called in the middle of a parse, >> rather than requiring that a visit is complete. >>
>> + switch (qobject_type(cur)) {
>> + case QTYPE_QDICT:
>> + assert(name);
>> + qdict_put_obj(qobject_to_qdict(cur), name, value);
>> + break;
>> + case QTYPE_QLIST:
>> + qlist_append_obj(qobject_to_qlist(cur), value);
>> + break;
>> + default:
>> + g_assert_not_reached();
>
> We usually just abort().
But there are definitely existing examples, and it is a bit more
self-documenting.
>>
>> @@ -230,7 +205,9 @@ static void qmp_output_type_any(Visitor *v, const char
>> *name, QObject **obj,
>> /* Finish building, and return the root object. Will not be NULL. */
>> QObject *qmp_output_get_qobject(QmpOutputVisitor *qov)
>> {
>> - QObject *obj = qmp_output_first(qov);
>> + /* FIXME: we should require that a visit occurred, and that it is
>> + * complete (no starts without a matching end) */
>
> I agree the visit must complete before you can retrieve the value.
>
> I think there are two sane ways to recover from errors:
>
> 1. Require the client to empty the stack by calling the necessary end
> methods.
>
> 2. Allow the client to reset or destroy the visitor without calling end
> methods.
>
> *This* visitor would be fine with either. I guess the others would be
> fine, too. So it's a question of interface design.
>
> I'm currently leaning towards 2, because "you must do A, B and C before
> you can destroy this object" would be weird. What do you think?
Patches later in the series revisit the question, and adding a reset
also makes it more interesting to be able to reset at any point in a
partial visit. For _this_ patch, I think we're okay, but this is
probably the cutoff for what I'm sending in the first round of v10,
saving all the trickier stuff for later.
>
>> + QObject *obj = qov->root;
>> if (obj) {
>> qobject_incref(obj);
>> } else {
>> @@ -248,16 +225,12 @@ void qmp_output_visitor_cleanup(QmpOutputVisitor *v)
>> {
>> QStackEntry *e, *tmp;
>>
>> - /* The bottom QStackEntry, if any, owns the root QObject. See the
>> - * qmp_output_push_obj() invocations in qmp_output_add_obj(). */
>> - QObject *root = QTAILQ_EMPTY(&v->stack) ? NULL : qmp_output_first(v);
>> -
>
> If we require end methods to be called, the stack must be empty here,
> rendering the following loop useless.
Yeah, an interesting observation that will affect what I do in 24/37.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
