Re: [PATCH v5 02/12] qapi: Make QAPI_LIST_ADD() public

[Eric Blake <eblake@redhat.com>]

On 10/26/20 9:25 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> We have a useful macro for inserting at the front of any
>> QAPI-generated list; move it from block.c to qapi/util.h so more
>> places can use it, including one earlier place in block.c.
>>
>> Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> ---
>>  include/qapi/util.h |  8 ++++++++
>>  block.c             | 15 +++------------
>>  2 files changed, 11 insertions(+), 12 deletions(-)
>>
>> diff --git a/include/qapi/util.h b/include/qapi/util.h
>> index 50201896c7a4..b6083055ce69 100644
>> --- a/include/qapi/util.h
>> +++ b/include/qapi/util.h
>> @@ -30,4 +30,12 @@ int qapi_enum_parse(const QEnumLookup *lookup, const char *buf,
>>
>>  int parse_qapi_name(const char *name, bool complete);
>>
>> +/* For any GenericList @list, insert @element at the front. */
>> +#define QAPI_LIST_ADD(list, element) do { \
>> +    typeof(list) _tmp = g_new(typeof(*(list)), 1); \
> 
> g_new(typeof(*p), 1) is an rather roundabout way to say
> g_malloc(sizeof(*p).  Yes, it returns typeof(p) instead of void *, but
> the chance of this catching mistakes here rounds to zero.
> 
> Not this patch's problem.  Ignore me :)

typeof is a gcc/clang extension; using sizeof makes the code slightly
more portable (but doesn't affect qemu's usage).  I'm happy to change
it, although it would require more commit message finesse since that
becomes more than just code motion.


>> @@ -5221,22 +5222,12 @@ BlockDeviceInfoList *bdrv_named_nodes_list(bool flat,
>>              qapi_free_BlockDeviceInfoList(list);
>>              return NULL;
>>          }
>> -        entry = g_malloc0(sizeof(*entry));
>> -        entry->value = info;
>> -        entry->next = list;
>> -        list = entry;
>> +        QAPI_LIST_ADD(list, info);
>>      }
>>
>>      return list;
>>  }
> 
> PATCH 12 has more.  I wonder whether PATCH 12 should be squashed into
> this one.  You decide.

Patch 12 has a LOT more.  And we're really close to soft freeze.  I kept
them separate to minimize the risk of landing my QAPI changes (4/12)
before 5.2 (that MUST happen, or we've locked in a problematic API with
block-export-add), vs. cleanup changes that may require review from a
lot more areas in the tree.  Given the timeline, I prefer to keep them
separate for v6.

I also still wonder if it is possible to make Coccinelle identify
candidates, rather than my manual audit that produced patch 12.

But for v6, I _will_ update the commit message to mention that more
conversions are possible, and saved for a later patch.

> 
> Reviewed-by: Markus Armbruster <armbru@redhat.com>

Thanks; I think I can keep this even for v6, since all I am changing is
an enhanced commit message.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org