| Message ID | 1468383486-108169-1-git-send-email-guangrong.xiao@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@linux.intel.com> > --- > 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? Thanks, Paolo
Paolo Bonzini <pbonzini@redhat.com> 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@linux.intel.com> >> --- >> 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?
On 13/07/2016 13:29, Markus Armbruster wrote: >> > 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? Yes, definitely! I'm queueing Guangrong's patch because it fixes a crash and the leak existed before, but without next_list we can indeed visit a "virtual" list and fix the leak. It can be done during the -rc period. Paolo
On 07/13/2016 07:37 PM, Paolo Bonzini wrote: > > > On 13/07/2016 13:29, Markus Armbruster wrote: >>>> 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? > > Yes, definitely! I'm queueing Guangrong's patch because it fixes a > crash and the leak existed before, but without next_list we can indeed > visit a "virtual" list and fix the leak. It can be done during the -rc > period. So you want to build uint16List list and save it as a "virtual" list in host_memory_backend_get_host_nodes(), then its caller can directly fetch this 'virtual' list from the visit?
On 07/15/2016 12:56 AM, Xiao Guangrong wrote: >>> 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? >> >> Yes, definitely! I'm queueing Guangrong's patch because it fixes a >> crash and the leak existed before, but without next_list we can indeed >> visit a "virtual" list and fix the leak. It can be done during the -rc >> period. > > So you want to build uint16List list and save it as a "virtual" list in > host_memory_backend_get_host_nodes(), then its caller can directly fetch > this 'virtual' list from the visit? With a virtual list visit, you don't even need a uint16List object. Merely call visit_start_list(NULL) to start the list with no matching uint16List, then visit_type_int16() for each list element (note no visit_next_list() calls), then visit_end_list().
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); }
'info memdev' crashes QEMU: (qemu) info memdev Unexpected error in parse_str() at qapi/string-input-visitor.c:111: Parameter 'null' expects an int64 value or range It is caused by null uint16List is returned if 'host-nodes' is the default value Return MAX_NODES under this case to fix this bug Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com> --- backends/hostmem.c | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-)