From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.71) id 1SU8KH-000309-UM for mharc-qemu-trivial@gnu.org; Mon, 14 May 2012 23:21:29 -0400 Received: from eggs.gnu.org ([208.118.235.92]:37131) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SU8KD-0002lg-6h for qemu-trivial@nongnu.org; Mon, 14 May 2012 23:21:27 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SU8KB-0004WU-0S for qemu-trivial@nongnu.org; Mon, 14 May 2012 23:21:24 -0400 Received: from mail-lb0-f173.google.com ([209.85.217.173]:64995) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SU8K5-0004UN-O9; Mon, 14 May 2012 23:21:18 -0400 Received: by lbok6 with SMTP id k6so4479061lbo.4 for ; Mon, 14 May 2012 20:21:14 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type; bh=1GMgNxzV7ivSD1bxtVautzGDQQDU9Eu8mMjV68MWjbE=; b=YWYtDACbGbeQnNP03b5FXH1M9IkyJbHNqvOB6ho4olv9Kq2sGRfEGyEI5o6BAaKQCl HvojwpysJzs0f8Kr5p6B59a8oiuXr1k1UXc3mWYAT4eqNUWGLOBM6BjVrrfhshgwpv5i XH64mNQuKtTviFDAcnfnLhV7rc2TC+j6PTY/oS123osWSVVt1AGCjSt2K6Dx+fn5Ht3b 6LAj2QeeG8WltllUoQqP1rVFCOJIY0IGtVwsDEOnjTy6WL6yHW2EVqhx2WMepTXJsm2f OdY27aCM40ZrrmWJv4Ff0bF8IRRAObqLjtobseuaMzsCxgL2CHWVCT6i1g+8BXzDAwMc jmWA== MIME-Version: 1.0 Received: by 10.112.49.230 with SMTP id x6mr4261179lbn.86.1337052074019; Mon, 14 May 2012 20:21:14 -0700 (PDT) Received: by 10.112.100.102 with HTTP; Mon, 14 May 2012 20:21:13 -0700 (PDT) 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 To: Michael Roth Content-Type: multipart/alternative; boundary=bcaec554deb2eb0d3e04c00ab127 X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 209.85.217.173 Cc: qemu-trivial@nongnu.org, Stefan Weil , Anthony Liguori , qemu-devel@nongnu.org Subject: Re: [Qemu-trivial] [Qemu-devel] [PATCH] qdev: Fix memory leak X-BeenThere: qemu-trivial@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 15 May 2012 03:21:27 -0000 --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--