Re: [Qemu-devel] [PATCH 3/3] hmp: fix MemdevList memory leak

[address@hidden]

On Fri, 2014-08-01 at 23:38 +1000, Peter Crosthwaite wrote: 
> On Fri, Aug 1, 2014 at 8:22 PM, Chen Fan <address@hidden> wrote:
> > Signed-off-by: Chen Fan <address@hidden>
> > ---
> >  hmp.c | 9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/hmp.c b/hmp.c
> > index 2414cc7..0b1c830 100644
> > --- a/hmp.c
> > +++ b/hmp.c
> > @@ -1685,13 +1685,14 @@ void hmp_info_memdev(Monitor *mon, const QDict 
> > *qdict)
> >  {
> >      Error *err = NULL;
> >      MemdevList *memdev_list = qmp_query_memdev(&err);
> > -    MemdevList *m = memdev_list;
> > +    MemdevList *m;
> >      StringOutputVisitor *ov;
> >      char *str;
> >      int i = 0;
> >
> >
> > -    while (m) {
> > +    while (memdev_list) {
> > +        m = memdev_list;
> >          ov = string_output_visitor_new(false);
> >          visit_type_uint16List(string_output_get_visitor(ov),
> >                                &m->value->host_nodes, NULL, NULL);
> > @@ -1710,7 +1711,9 @@ void hmp_info_memdev(Monitor *mon, const QDict *qdict)
> >
> >          string_output_visitor_cleanup(ov);
> >          g_free(str);
> > -        m = m->next;
> > +        memdev_list = memdev_list->next;
> > +        g_free(m->value);
> > +        g_free(m);
> 
> This doesnt feel quite right. You are piecewise freeing an entire data
> structure as allocated by a single function (qmp_query_memdev). I
> think you should create a function that cleans up MemdevList (just
> loops and frees) so that any and all callers of qmp_query_memdev can
> cleanup in one single action.
> 
> Note that qmp_query_memdev already has the code you need in it's error path:
> 
>     while (list) {
>         m = list;
>         list = list->next;
>         g_free(m->value);
>         g_free(m);
>     }
> 
> So you can split this out into it's own fn and call it both here and
> in that error path.
You're right. I will add a common fn to free them.

Thanks,
Chen

> 
> Regards,
> Peter
> 
> >          i++;
> >      }
> >
> > --
> > 1.9.3
> >
> >