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.f...@cn.fujitsu.com> wrote:
> > Signed-off-by: Chen Fan <chen.fan.f...@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
> >
> >
