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

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3336 bytes --]

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.


>> +++ 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?

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.

>> +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.
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]