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

[Eric Blake]

On 11/13/20 1:39 PM, Dr. David Alan Gilbert wrote:
> * Eric Blake (eblake@redhat.com) wrote:
>> 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.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> ---

> 
> <snip>
> 

>> +++ b/monitor/hmp-cmds.c
>> @@ -1699,7 +1699,8 @@ void hmp_closefd(Monitor *mon, const QDict *qdict)
>>  void hmp_sendkey(Monitor *mon, const QDict *qdict)
>>  {
>>      const char *keys = qdict_get_str(qdict, "keys");
>> -    KeyValueList *keylist, *head = NULL, *tmp = NULL;
>> +    KeyValue *v;
>> +    KeyValueList *head = NULL, **tail = &head;
>>      int has_hold_time = qdict_haskey(qdict, "hold-time");
>>      int hold_time = qdict_get_try_int(qdict, "hold-time", -1);
>>      Error *err = NULL;
>> @@ -1716,16 +1717,7 @@ void hmp_sendkey(Monitor *mon, const QDict *qdict)
>>              keyname_len = 4;
>>          }
>>
>> -        keylist = g_malloc0(sizeof(*keylist));
>> -        keylist->value = g_malloc0(sizeof(*keylist->value));
>> -
>> -        if (!head) {
>> -            head = keylist;
>> -        }
>> -        if (tmp) {
>> -            tmp->next = keylist;
>> -        }
>> -        tmp = keylist;
>> +        v = g_malloc0(sizeof(*v));
>>
>>          if (strstart(keys, "0x", NULL)) {
>>              char *endp;
>> @@ -1734,16 +1726,17 @@ void hmp_sendkey(Monitor *mon, const QDict *qdict)
>>              if (endp != keys + keyname_len) {
>>                  goto err_out;
>>              }
>> -            keylist->value->type = KEY_VALUE_KIND_NUMBER;
>> -            keylist->value->u.number.data = value;
>> +            v->type = KEY_VALUE_KIND_NUMBER;
>> +            v->u.number.data = value;
>>          } else {
>>              int idx = index_from_key(keys, keyname_len);
>>              if (idx == Q_KEY_CODE__MAX) {
>>                  goto err_out;
>>              }
>> -            keylist->value->type = KEY_VALUE_KIND_QCODE;
>> -            keylist->value->u.qcode.data = idx;
>> +            v->type = KEY_VALUE_KIND_QCODE;
>> +            v->u.qcode.data = idx;
>>          }
>> +        QAPI_LIST_APPEND(tail, v);
>>
>>          if (!*separator) {
>>              break;
> 
> Don't you need to arrange for 'v' to be free'd in the err_out case?

Good catch.  Pre-patch, the allocation was appended to the list before
it was possible to reach 'goto err_out', but post-patch, the use of a
separate variable and delayed addition to the list matters.  Will fix.

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