[3/3] hmp: fix MemdevList memory leak

Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
---
 hmp.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

On Fri, Aug 1, 2014 at 8:22 PM, Chen Fan <chen.fan.fnst@cn.fujitsu.com> wrote:
> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
> ---
>  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.

Regards,
Peter

>          i++;
>      }
>
> --
> 1.9.3
>
>

On Fri, 2014-08-01 at 23:38 +1000, Peter Crosthwaite wrote: 
> On Fri, Aug 1, 2014 at 8:22 PM, Chen Fan <chen.fan.fnst@cn.fujitsu.com> wrote:
> > Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
> > ---
> >  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
> >
> >