qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v14 05/19] qmp-input: Clean up stack handling


From: Markus Armbruster
Subject: Re: [Qemu-devel] [PATCH v14 05/19] qmp-input: Clean up stack handling
Date: Fri, 15 Apr 2016 17:27:02 +0200
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux)

Eric Blake <address@hidden> writes:

> On 04/13/2016 09:53 AM, Markus Armbruster wrote:
>> Eric Blake <address@hidden> writes:
>> 
>>> Management of the top of stack was a bit verbose; creating a
>>> temporary variable and adding some comments makes the existing
>>> code more legible before the next few patches improve things.
>>> No semantic changes other than asserting that we are always
>>> visiting a QObject, and not a NULL value.
>>>
>>> Signed-off-by: Eric Blake <address@hidden>
>>>
>>> ---
>
>> 
>> The mixture of block comments and comments to the right is a bit
>> awkward.  What about:
>> 
>>    typedef struct StackObject {
>>        QObject *obj; /* Object being visited */
>> 
>>        GHashTable *h;              /* if obj is dict: unvisited keys */
>>        const QListEntry *entry;    /* if obj is list: unvisited tail */
>>    } StackObject;
>> 
>
> Works for me.
>
>>>
>>>  struct QmpInputVisitor
>>>  {
>>>      Visitor visitor;
>>> +
>>> +    /* Stack of objects being visited.  stack[0] is root of visit,
>>> +     * stack[1] and below correspond to visit_start_struct (nested
>>> +     * QDict) and visit_start_list (nested QList).  */
>> 
>> I guess what you want to say is stack[1..] record the nesting of
>> start_struct() ... end_struct() and start_list() ... end_list() pairs.
>> 
>> Comment gets rewritten in PATCH 17, no need to worry too much about it.
>> 
>>>      StackObject stack[QIV_STACK_SIZE];
>>>      int nb_stack;
>>> +
>>> +    /* True to track whether all keys in QDict have been parsed.  */
>>>      bool strict;
>> 
>> I think @strict switches on rejection of unexpected dictionary keys.
>> See qmp_input_pop() below.
>> 
>> I dislike the fact that we have two input visitors, and the one with the
>> obvious name ignores certain errors.  I don't doubt that it has its
>> uses, but reporting errors should be the default, and ignoring them
>> should be a conscious decision.  Anyway, not this patch's problem.
>
> Dan also has a pending patch that reworks it to add yet another
> parameter (the ability to take input in string format and auto-convert
> it to the correct type).  In that one, he exposes a third method for
> choosing which visitor you get, and which then under the hood call a
> helper with two boolean flags.  Maybe it's time to just convert all
> clients to always passing the parameters they want, along with auditing
> whether ignoring extra input is a sane option for that client - but as
> you say, it's fine for a separate patch.
>
>>> +
>>> +    /* If we have a name, and we're in a dictionary, then return that
>>> +     * value. */
>> 
>> Can we be in a dictionary and not have a name?
>
> The converse happens: we can certainly have a name and not be in a
> dictionary, for a top-level visit.  But it has weird semantics until I
> clean it up later in 17/19.  For this patch, it was just code motion and
> documentation (the 'if (name && qobject_type...)' condition here is the
> same pre- and post-patch), where I was just getting rid of a dead 'if
> (qobj)'.

So "in dictionary" implies "have a name".  Why do we bother to test
@name then?

>> 
>> 
>>    static void qmp_input_pop(QmpInputVisitor *qiv, Error **errp)
>>    {
>>        assert(qiv->nb_stack > 0);
>> 
>>        if (qiv->strict) {
>>            GHashTable * const top_ht = qiv->stack[qiv->nb_stack - 1].h;
>>            if (top_ht) {
>>                GHashTableIter iter;
>>                const char *key;
>> 
>>                g_hash_table_iter_init(&iter, top_ht);
>>                if (g_hash_table_iter_next(&iter, (void **)&key, NULL)) {
>>                    error_setg(errp, QERR_QMP_EXTRA_MEMBER, key);
>> 
>> This looks wrong.  If we have more than one extra members, the second
>> call error_setg() will fail an assertion in error_setv(), unless errp is
>> null.
>
> Whoops - looks like f96493b1 is broken for missing a 'break' statement.
>  I'll send that as a separate for-2.6 cleanup that we should pull sooner
> rather than later.

False alarm, as you noticed.



reply via email to

[Prev in Thread] Current Thread [Next in Thread]