Thanks for your reply.
As you say, for an input visitor we dont need to initialize the pointer.
"visit_type_str" in set_mac function and set_pci_devfn function is a input
visitor, it points to
"qmp_input_type_str", if qmp_input_type_str failed, it will alloc a Error
struct and return.
So, i think it doesnt matter whether initializing it or not in this
situation.
But the str must be freed to avoid memory leak
2012/5/15 Michael Roth <mdr...@linux.vnet.ibm.com>
> On Mon, May 14, 2012 at 09:36:36PM +0200, Stefan Weil wrote:
> > Hello,
> >
> > Am 03.05.2012 10:34, schrieb dunrong huang:
> > >The str allocated in visit_type_str was not freed
> > >
> > >Signed-off-by: dunrong huang <riegama...@gmail.com>
> > >---
> > >hw/qdev-properties.c | 8 ++++++--
> > >1 files changed, 6 insertions(+), 2 deletions(-)
> > >
> > >diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
> > >index 98dd06a..8088699 100644
> > >--- a/hw/qdev-properties.c
> > >+++ b/hw/qdev-properties.c
> > >@@ -726,7 +726,7 @@ static void set_mac(Object *obj, Visitor *v,
> > >void *opaque,
> > >MACAddr *mac = qdev_get_prop_ptr(dev, prop);
> > >Error *local_err = NULL;
> > >int i, pos;
> > >- char *str, *p;
> > >+ char *str = NULL, *p;
> >
> > Is this change really needed?
> >
> > >
> > >if (dev->state != DEV_STATE_CREATED) {
> > >error_set(errp, QERR_PERMISSION_DENIED);
> > >@@ -753,10 +753,12 @@ static void set_mac(Object *obj, Visitor *v,
> > >void *opaque,
> > >}
> > >mac->a[i] = strtol(str+pos, &p, 16);
> > >}
> > >+ g_free(str);
> > >return;
> > >
> > >inval:
> > >error_set_from_qdev_prop_error(errp, EINVAL, dev, prop, str);
> > >+ g_free(str);
> > >}
> > >
> > >PropertyInfo qdev_prop_macaddr = {
> > >@@ -825,7 +827,7 @@ static void set_pci_devfn(Object *obj, Visitor
> > >*v, void *opaque,
> > >uint32_t *ptr = qdev_get_prop_ptr(dev, prop);
> > >unsigned int slot, fn, n;
> > >Error *local_err = NULL;
> > >- char *str = (char *)"";
> > >+ char *str = NULL;
> >
> > As far as I could see, str does not need an initial value, so
> > neither the old nor the new code is optimal (both versions do
> > work).
> >
> > >
> > >if (dev->state != DEV_STATE_CREATED) {
> > >error_set(errp, QERR_PERMISSION_DENIED);
> > >@@ -847,10 +849,12 @@ static void set_pci_devfn(Object *obj,
> > >Visitor *v, void *opaque,
> > >goto invalid;
> > >}
> > >*ptr = slot << 3 | fn;
> > >+ g_free(str);
> > >return;
> > >
> > >invalid:
> > >error_set_from_qdev_prop_error(errp, EINVAL, dev, prop, str);
> > >+ g_free(str);
> > >}
> > >
> > >static int print_pci_devfn(DeviceState *dev, Property *prop, char
> > >*dest, size_t len)
> >
> > Maybe some expert (Michael Roth?) can comment on the correct
> > usage of visit_type_str.
> >
> > Is the initial value for the 2nd argument really needed?
> > The current QEMU code sometimes uses initialization,
> > but not always (see the code above, for example), so it is confusing.
>
> For an output visitor (native-to-<QMP/String/etc>), we assume we're
> getting a pointer to a valid string, and generally treat NULL as an
> indication
> to generate an empty string, so the initial value matters in that case.
> For that situation the changes would be warranted.
>
> But for an input visitor (<QMP/String/etc>-to-native) like we're using in
> the
> setters this patch modifies, the initial value gets clobbered with a
> pointer
> to whatever the visitor allocates, so initializing the pointers isn't
> neccessary.
>
> >
> > Otherwise the patch is ok. It fixes memory leaks,
> > therefore it should be added to QEMU 1.1.
> >
> > Tested-by: Stefan Weil <s...@weilnetz.de>
> > > Regards,
> >
> > Stefan Weil
> >
> >
>
--
linuxer and emacser and pythoner living in beijing
blog: http://mathslinux.org
twitter: https://twitter.com/mathslinux
google+: https://plus.google.com/118129852578326338750
