Re: [Qemu-devel] [PATCH] fixup? qapi: Simplify QObject

[Markus Armbruster <armbru@redhat.com>]

Eric Blake <eblake@redhat.com> writes:

> On 11/19/2015 01:58 AM, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> writes:
>> 
>>> [If we squash, replace the old commit message with this; you'll
>>> also have some merge conflicts in 29/36 and 30/36.  Note that
>>> while this mail is net lines added, squashing would still result
>>> in a net reduction:
>>>  16 files changed, 67 insertions(+), 83 deletions(-)
>>> ]
>
> I'm leaning towards squashing, so I'll post a v12.5 of 28-30.

I'll push my current branch shortly.  I'm happy with it up to PATCH 26.
I'm still pondering PATCH 27.

>>> +++ b/include/qapi/qmp/qobject.h
>>> @@ -52,15 +52,17 @@ typedef struct QObject QObject;
>>>  struct QObject {
>>>      qtype_code type;
>>>      size_t refcnt;
>>> -    void (*destroy)(QObject *);
>>>  };
>>>
>>> +typedef void (*QDestroy)(QObject *);
>>> +extern QDestroy qdestroy[QTYPE_MAX];
>>> +
>
> So if I'm understanding you, instead of declaring the table,...
>
>>> @@ -97,8 +97,8 @@ static inline void qobject_decref(QObject *obj)
>>>  {
>>>      assert(!obj || obj->refcnt);
>>>      if (obj && --obj->refcnt == 0) {
>>> -        assert(obj->destroy);
>>> -        obj->destroy(obj);
>>> +        assert(qdestroy[obj->type]);
>>> +        qdestroy[obj->type](obj);
>>>      }
>>>  }
>>>
>> 
>> Preexisting: the assertion is of little value, because all it does is
>> convert one kind of crash into another.
>> 
>> Asserting obj->type is in range might be more useful, i.e.
>> 
>>   -        assert(obj->destroy);
>>   -        obj->destroy(obj);
>>   +        assert(QTYPE_QNULL < obj->type && obj->type < QTYPE_MAX);
>>   +        qdestroy[obj->type](obj);
>> 
>> We could outline the conditional part, and keep qdestroy[] static.
>> Could save a bit of code (unless NDEBUG, but we shouldn't optimize for
>> that).
>
> ...I could do:
>
> /* High-level interface for qobject_decref() */
> #define QDECREF(obj)              \
>     do { \
>         if (obj) { \
>             assert(QOBJECT(obj)->refcnt)); \
>             if (!--QOBJECT(obj)->refcnt) { \
>                 qobject_decref(QOBJECT(obj)); \
>             } \
>         } \
>     } while (0)
>
> and move qobject_decref() into qobject.c where the destroy table is then
> static to that file.
>
> Correct?

Almost.

> Oh wait, it won't work quite like that. We have direct callers of
> qobject_decref() (QDECREF is only usable on subclasses, but if you have
> a QObject* you can't use QDECREF), so I can't change semantics by
> hoisting things into the macro.

In qobject.h:

/**
 * qobject_decref(): Decrement QObject's reference count, deallocate
 * when it reaches zero
 */
static inline void qobject_decref(QObject *obj)
{
    assert(!obj || obj->refcnt);
    if (obj && --obj->refcnt == 0) {
        qobject_destroy(obj);
    }
}

In qobject.c:

void qobject_destroy(QObject *obj)
{
    assert(!obj->refcnt);
    assert(QTYPE_QNULL < obj->type && obj->type < QTYPE_MAX);
    qdestroy[obj->type](obj);
}

>>> +QDestroy qdestroy[QTYPE_MAX] = {
>>> +    [QTYPE_QBOOL] = qbool_destroy_obj,
>>> +    [QTYPE_QDICT] = qdict_destroy_obj,
>>> +    [QTYPE_QFLOAT] = qfloat_destroy_obj,
>>> +    [QTYPE_QINT] = qint_destroy_obj,
>>> +    [QTYPE_QLIST] = qlist_destroy_obj,
>>> +    [QTYPE_QSTRING] = qstring_destroy_obj,
>>> +    /* [QTYPE_QNULL] = NULL, */
>>> +};
>> 
>> Suggest
>> 
>>    QDestroy qdestroy[QTYPE_MAX] = {
>>        [QTYPE_QNULL] = NULL,       /* no such object exists */
>>        [QTYPE_QNULL] = NULL,       /* qnull_ is indestructible */
>
> Makes sense (with the obvious fix for QTYPE_NONE).
>
>
>> If you'd like to respin, I'd prefer a non-incremental [PATCH v12.5
>> 28/36] replacing v12.
>
> Plus 29 and 30 for conflict resolutions.
>
>> 
>> If not, I'm happy to squash in trivial changes like the one I suggested
>> for qdestroy[]'s initializer.