Re: [Qemu-devel] [PATCH] qapi: change QmpInputVisitor to QSLIST

[Paolo Bonzini <pbonzini@redhat.com>]

On 06/07/2016 17:39, Eric Blake wrote:
> On 07/06/2016 06:43 AM, Paolo Bonzini wrote:
>> This saves a lot of memory compared to a statically-sized array.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>  qapi/qmp-input-visitor.c | 53 ++++++++++++++++++++++++------------------------
>>  1 file changed, 26 insertions(+), 27 deletions(-)
>>
> 
>> @@ -99,17 +100,10 @@ static const QListEntry *qmp_input_push(QmpInputVisitor *qiv, QObject *obj,
>>                                          Error **errp)
>>  {
>>      GHashTable *h;
>> -    StackObject *tos = &qiv->stack[qiv->nb_stack];
>> +    StackObject *tos = g_new0(StackObject, 1);
>>  
>>      assert(obj);
>> -    if (qiv->nb_stack >= QIV_STACK_SIZE) {
> 
> You should also delete QIV_STACK_SIZE as it is now unused.
> 
>> @@ -127,9 +121,7 @@ static const QListEntry *qmp_input_push(QmpInputVisitor *qiv, QObject *obj,
>>  static void qmp_input_check_struct(Visitor *v, Error **errp)
>>  {
>>      QmpInputVisitor *qiv = to_qiv(v);
>> -    StackObject *tos = &qiv->stack[qiv->nb_stack - 1];
>> -
>> -    assert(qiv->nb_stack > 0);
>> +    StackObject *tos = QSLIST_FIRST(&qiv->stack);
> 
> Does QSLIST_FIRST() properly crash if the list is empty, or do we need
> to add an assert(tos) to replace the assertion on nb_stack being non-zero?

    StackObject *tos = QSLIST_FIRST(&qiv->stack);

    if (qiv->strict) {
        GHashTable *const top_ht = tos->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);
            }
        }
    }

If qiv->strict, the tos->h dereference will cause a crash so the
assertion is not necessary.  If !qiv->strict, check_struct should be
immediately followed by end_struct which will also crash.

However, I agree that it's nicer to keep the assertion.

Paolo

> Otherwise looking reasonable; looking forward to v2.
>