On Fri, 2014-08-01 at 23:26 +1000, Peter Crosthwaite wrote:
> On Fri, Aug 1, 2014 at 8:22 PM, Chen Fan <chen.fan.f...@cn.fujitsu.com> wrote:
> > string_output_get_string() always return the data the sov->string
>
> "returns the data sov->string points to and never frees it"
>
> Although "sov" is a little out of context however of this change, and
> you need to go diving into qapi code to get context on what you mean
> by that. Perhaps just say it uses g_string_free which requires callers
> to free the returned data?
>
> > point. and never free.
> >
> > Signed-off-by: Chen Fan <chen.fan.f...@cn.fujitsu.com>
> > ---
> > hmp.c | 6 ++++--
> > qom/object.c | 11 ++++++++---
> > 2 files changed, 12 insertions(+), 5 deletions(-)
> >
> > diff --git a/hmp.c b/hmp.c
> > index 4d1838e..2414cc7 100644
> > --- a/hmp.c
> > +++ b/hmp.c
> > @@ -1687,6 +1687,7 @@ void hmp_info_memdev(Monitor *mon, const QDict *qdict)
> > MemdevList *memdev_list = qmp_query_memdev(&err);
> > MemdevList *m = memdev_list;
> > StringOutputVisitor *ov;
> > + char *str;
> > int i = 0;
> >
> >
> > @@ -1704,10 +1705,11 @@ void hmp_info_memdev(Monitor *mon, const QDict
> > *qdict)
> > m->value->prealloc ? "true" : "false");
> > monitor_printf(mon, " policy: %s\n",
> > HostMemPolicy_lookup[m->value->policy]);
> > - monitor_printf(mon, " host nodes: %s\n",
> > - string_output_get_string(ov));
> > + str = string_output_get_string(ov);
> > + monitor_printf(mon, " host nodes: %s\n", str);
> >
> > string_output_visitor_cleanup(ov);
> > + g_free(str);
> > m = m->next;
> > i++;
> > }
> > diff --git a/qom/object.c b/qom/object.c
> > index 0e8267b..7ae4f33 100644
> > --- a/qom/object.c
> > +++ b/qom/object.c
> > @@ -948,16 +948,18 @@ int object_property_get_enum(Object *obj, const char
> > *name,
> > {
> > StringOutputVisitor *sov;
> > StringInputVisitor *siv;
> > + char *str;
> > int ret;
> >
> > sov = string_output_visitor_new(false);
> > object_property_get(obj, string_output_get_visitor(sov), name, errp);
> > - siv = string_input_visitor_new(string_output_get_string(sov));
> > + str = string_output_get_string(sov);
> > + siv = string_input_visitor_new(str);
>
> Free here (ASAP) instead of below.
here, I think free here is incorrect, the below visit_type_enum() will
call visit_type_str which strdups the 'str', we should not free it
before that, so let it there.
Thanks,
Chen
>
> > string_output_visitor_cleanup(sov);
> > visit_type_enum(string_input_get_visitor(siv),
> > &ret, strings, NULL, name, errp);
> > string_input_visitor_cleanup(siv);
> > -
>
> Don't delete existing blank lines that separate sections like this.
>
> > + g_free(str);
> > return ret;
> > }
> >
> > @@ -966,15 +968,18 @@ void object_property_get_uint16List(Object *obj,
> > const char *name,
> > {
> > StringOutputVisitor *ov;
> > StringInputVisitor *iv;
> > + char *str;
> >
> > ov = string_output_visitor_new(false);
> > object_property_get(obj, string_output_get_visitor(ov),
> > name, errp);
> > - iv = string_input_visitor_new(string_output_get_string(ov));
> > + str = string_output_get_string(ov);
> > + iv = string_input_visitor_new(str);
>
> Ditto on the ASAP free
>
> Regards,
> Peter
>
> > visit_type_uint16List(string_input_get_visitor(iv),
> > list, NULL, errp);
> > string_output_visitor_cleanup(ov);
> > string_input_visitor_cleanup(iv);
> > + g_free(str);
> > }
> >
> > void object_property_parse(Object *obj, const char *string,
> > --
> > 1.9.3
> >
> >
