Paolo Bonzini <pbonzini@xxxxxxxxxx> writes:
> On 13/07/2016 06:18, Xiao Guangrong wrote:
>>
>> Return MAX_NODES under this case to fix this bug
>>
>> Signed-off-by: Xiao Guangrong <guangrong.xiao@xxxxxxxxxxxxxxx>
>> ---
>> backends/hostmem.c | 22 ++++++++++++++--------
>> 1 file changed, 14 insertions(+), 8 deletions(-)
>>
>> diff --git a/backends/hostmem.c b/backends/hostmem.c
>> index 6e28be1..8dede4d 100644
>> --- a/backends/hostmem.c
>> +++ b/backends/hostmem.c
>> @@ -64,6 +64,14 @@ out:
>> error_propagate(errp, local_err);
>> }
>>
>> +static uint16List **host_memory_append_node(uint16List **node,
>> + unsigned long value)
>> +{
>> + *node = g_malloc0(sizeof(**node));
>> + (*node)->value = value;
>> + return &(*node)->next;
>> +}
>> +
>> static void
>> host_memory_backend_get_host_nodes(Object *obj, Visitor *v, const char *name,
>> void *opaque, Error **errp)
>> @@ -74,25 +82,23 @@ host_memory_backend_get_host_nodes(Object *obj, Visitor *v, const char *name,
>> unsigned long value;
>>
>> value = find_first_bit(backend->host_nodes, MAX_NODES);
>> +
>> + node = host_memory_append_node(node, value);
>> +
>> if (value == MAX_NODES) {
>> - return;
>> + goto out;
>> }
>>
>> - *node = g_malloc0(sizeof(**node));
>> - (*node)->value = value;
>> - node = &(*node)->next;
>> -
>> do {
>> value = find_next_bit(backend->host_nodes, MAX_NODES, value + 1);
>> if (value == MAX_NODES) {
>> break;
>> }
>>
>> - *node = g_malloc0(sizeof(**node));
>> - (*node)->value = value;
>> - node = &(*node)->next;
>> + node = host_memory_append_node(node, value);
>> } while (true);
>>
>> +out:
>> visit_type_uint16List(v, name, &host_nodes, errp);
>
> This function is leaking host_nodes, so you need a
>
> qapi_free_uint16List(head);
>
> here (and saving the head pointer on the first call to
> host_memory_append_node). The bug is preexisting.
>
> I'm curious about one thing. Eric/Markus, it would be nice to open code
> the visit of the list with
>
> visit_start_list(v, name, NULL, 0, &err);
> if (err) {
> goto out;
> }
> ...
> visit_type_uint16(v, name, &value, &err);
> visit_next_list(v, NULL, 0);
> ...
> visit_end_list(v, NULL);
>
> We know here that on the other side there is an output visitor.
> However, it doesn't work because visit_next_list asserts that tail ==
> NULL. Would it be easy to support this idiom, and would it make sense
> to extend it to other kinds of visitor?
visit_next_list() asserts tail != NULL because to protect the
next_list() method. qmp_output_next_list() dereferences tail.
Note that you don't have to call visit_next_list() in a virtual visit.
For an example, see prop_get_fdt(). Good enough already?
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
