From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:37118) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SU8K8-0002l9-VU for qemu-devel@nongnu.org; Mon, 14 May 2012 23:21:22 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SU8K6-0004VC-71 for qemu-devel@nongnu.org; Mon, 14 May 2012 23:21:20 -0400 MIME-Version: 1.0 In-Reply-To: <20120514204011.GF28865@illuin> References: <1336034082-5288-1-git-send-email-riegamaths@gmail.com> <4FB15EC4.3050206@weilnetz.de> <20120514204011.GF28865@illuin> Date: Tue, 15 May 2012 11:21:13 +0800 Message-ID: From: dunrong huang Content-Type: multipart/alternative; boundary=bcaec554deb2eb0d3e04c00ab127 Subject: Re: [Qemu-devel] [PATCH] qdev: Fix memory leak List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Michael Roth Cc: qemu-trivial@nongnu.org, Stefan Weil , Anthony Liguori , qemu-devel@nongnu.org --bcaec554deb2eb0d3e04c00ab127 Content-Type: text/plain; charset=ISO-8859-1 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 > 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 > > >--- > > >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-), 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 (-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 > > > 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 --bcaec554deb2eb0d3e04c00ab127 Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Thanks for your reply.

As you say, for an input visitor we dont nee= d to initialize the pointer.
"visit_type_str" in set_mac funct= ion and set_pci_devfn function is a input visitor, it points to
"qm= p_input_type_str", if qmp_input_type_str failed, it will alloc a Erro= r struct and return.

So, i think it doesnt matter whether initializing it or not in this sit= uation.
But the str must be freed to avoid memory leak


2012/5/15 Michael Roth <mdroth@linux.vnet.i= bm.com>
On M= on, 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 <riegamaths@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 =3D qdev_get_prop_ptr(dev, prop);
> >Error *local_err =3D NULL;
> >int i, pos;
> >- char *str, *p;
> >+ char *str =3D NULL, *p;
>
> Is this change really needed?
>
> >
> >if (dev->state !=3D 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] =3D 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 =3D {
> >@@ -825,7 +827,7 @@ static void set_pci_devfn(Object *obj, Visitor=
> >*v, void *opaque,
> >uint32_t *ptr =3D qdev_get_prop_ptr(dev, prop);
> >unsigned int slot, fn, n;
> >Error *local_err =3D NULL;
> >- char *str =3D (char *)"";
> >+ char *str =3D 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 !=3D 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 =3D 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<= br> > >*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.<= br>
For an output visitor (native-to-<QMP/String/etc>), we as= sume we're
getting a pointer to a valid string, and generally treat NULL as an indicat= ion
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 pointe= r
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 <sw@weilne= tz.de>
> > Regards,
>
> Stefan Weil
>
>



--
linuxer and= emacser and pythoner living in beijing
blog: http://mathslinux.org
twitter: https://twitter.com/maths= linux
google+: https://plus.google.com/118129852578326338750

--bcaec554deb2eb0d3e04c00ab127--