[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: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH v14 05/19] qmp-input: Clean up stack handling |
Date: |
Wed, 13 Apr 2016 10:36:11 -0600 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.7.1 |
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)'.
>
>
> 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.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
[Qemu-devel] [PATCH v14 06/19] qmp-input: Don't consume input when checking has_member, Eric Blake, 2016/04/08