Re: [PATCH v2 7/7] qapi: More complex uses of QAPI_LIST_APPEND

[Eric Blake]

On 11/19/20 2:50 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> These cases require a bit more thought to review; in each case, the
>> code was appending to a list, but not with a FOOList **tail variable.

>> +++ b/hw/core/machine-qmp-cmds.c
> [...]
>> @@ -294,41 +281,31 @@ void qmp_set_numa_node(NumaOptions *cmd, Error **errp)
>>  static int query_memdev(Object *obj, void *opaque)

>>          v = qobject_input_visitor_new(host_nodes);
>> -        visit_type_uint16List(v, NULL, &m->value->host_nodes, &error_abort);
>> +        visit_type_uint16List(v, NULL, &m->host_nodes, &error_abort);
>>          visit_free(v);
>>          qobject_unref(host_nodes);
>>
>> -        m->next = *list;
>> -        *list = m;
>> +        QAPI_LIST_APPEND(list, m);
> 
> The old code prepends, doesn't it?

Good catch, will correct and hoist this into 4/7 for v3.

> 
>>      }
>>
>>      return 0;
>> diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
>> index cf0627fd01c1..1afcc29a0649 100644
>> --- a/hw/mem/memory-device.c
>> +++ b/hw/mem/memory-device.c
>> @@ -199,7 +199,7 @@ out:
>>  MemoryDeviceInfoList *qmp_memory_device_list(void)
>>  {
>>      GSList *devices = NULL, *item;
>> -    MemoryDeviceInfoList *list = NULL, *prev = NULL;
>> +    MemoryDeviceInfoList *list = NULL, **prev = &list;
> 
> Here, you reuse the old name for the new variable.

>> +++ b/hw/pci/pci.c
>> @@ -1681,41 +1681,34 @@ static PciDeviceInfoList 
>> *qmp_query_pci_devices(PCIBus *bus, int bus_num);
>>
>>  static PciMemoryRegionList *qmp_query_pci_regions(const PCIDevice *dev)
>>  {
>> -    PciMemoryRegionList *head = NULL, *cur_item = NULL;
>> +    PciMemoryRegionList *head = NULL, **tail = &head;
> 
> Here, you use a new and better name.
> 
> I'd like to encourage you to name tail pointer variables @tail
> elsewhere, too.

In v3, I will consistently rename the FOOList ** variable 'tail'.

>> @@ -2863,7 +2846,6 @@ qmp_guest_set_memory_blocks(GuestMemoryBlockList 
>> *mem_blks, Error **errp)
>>
>>      while (mem_blks != NULL) {
>>          GuestMemoryBlockResponse *result;
>> -        GuestMemoryBlockResponseList *entry;
>>          GuestMemoryBlock *current_mem_blk = mem_blks->value;
>>
>>          result = g_malloc0(sizeof(*result));
>> @@ -2872,11 +2854,7 @@ qmp_guest_set_memory_blocks(GuestMemoryBlockList 
>> *mem_blks, Error **errp)
>>          if (local_err) { /* should never happen */
>>              goto err;
>>          }
>> -        entry = g_malloc0(sizeof *entry);
>> -        entry->value = result;
>> -
>> -        *link = entry;
>> -        link = &entry->next;
>> +        QAPI_LIST_APPEND(link, result);
>>          mem_blks = mem_blks->next;
>>      }
>>
> 
> This one looks like a candidate for PATCH 6.

Yes.  Will hoist.

v3 will be posted soon.

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