Re: [Qemu-devel] [PATCH v9 20/37] qmp: Don't abuse stack to track qmp-output root

[Eric Blake]

On 01/21/2016 06:58 AM, Markus Armbruster wrote:
> Eric Blake <address@hidden> 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