* [Qemu-devel] [PATCH] qdev: Fix memory leak
@ 2012-05-03 8:34 dunrong huang
2012-05-14 19:36 ` Stefan Weil
0 siblings, 1 reply; 7+ messages in thread
From: dunrong huang @ 2012-05-03 8:34 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-trivial, Anthony Liguori, 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 = qdev_get_prop_ptr(dev, prop);
Error *local_err = NULL;
int i, pos;
- char *str, *p;
+ char *str = NULL, *p;
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;
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)
--
1.7.8.4
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [Qemu-devel] [PATCH] qdev: Fix memory leak
2012-05-03 8:34 [Qemu-devel] [PATCH] qdev: Fix memory leak dunrong huang
@ 2012-05-14 19:36 ` Stefan Weil
2012-05-14 20:40 ` Michael Roth
0 siblings, 1 reply; 7+ messages in thread
From: Stefan Weil @ 2012-05-14 19:36 UTC (permalink / raw)
To: dunrong huang; +Cc: qemu-trivial, Anthony Liguori, qemu-devel, Michael Roth
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 = 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.
Otherwise the patch is ok. It fixes memory leaks,
therefore it should be added to QEMU 1.1.
Tested-by: Stefan Weil <sw@weilnetz.de>
Regards,
Stefan Weil
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [Qemu-devel] [PATCH] qdev: Fix memory leak
2012-05-14 19:36 ` Stefan Weil
@ 2012-05-14 20:40 ` Michael Roth
2012-05-15 3:21 ` dunrong huang
0 siblings, 1 reply; 7+ messages in thread
From: Michael Roth @ 2012-05-14 20:40 UTC (permalink / raw)
To: Stefan Weil; +Cc: qemu-trivial, Anthony Liguori, qemu-devel, dunrong huang
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 <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 = 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 <sw@weilnetz.de>
> > Regards,
>
> Stefan Weil
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [Qemu-devel] [PATCH] qdev: Fix memory leak
2012-05-14 20:40 ` Michael Roth
@ 2012-05-15 3:21 ` dunrong huang
2012-05-18 9:10 ` [Qemu-devel] [Qemu-trivial] " Stefan Hajnoczi
0 siblings, 1 reply; 7+ messages in thread
From: dunrong huang @ 2012-05-15 3:21 UTC (permalink / raw)
To: Michael Roth; +Cc: qemu-trivial, Stefan Weil, Anthony Liguori, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 3810 bytes --]
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 <mdroth@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 <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 = 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 <sw@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
[-- Attachment #2: Type: text/html, Size: 5321 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [Qemu-devel] [Qemu-trivial] [PATCH] qdev: Fix memory leak
2012-05-15 3:21 ` dunrong huang
@ 2012-05-18 9:10 ` Stefan Hajnoczi
0 siblings, 0 replies; 7+ messages in thread
From: Stefan Hajnoczi @ 2012-05-18 9:10 UTC (permalink / raw)
To: dunrong huang
Cc: qemu-trivial, Stefan Weil, Anthony Liguori, Michael Roth,
qemu-devel
On Tue, May 15, 2012 at 11:21:13AM +0800, dunrong huang wrote:
> 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
Want to send a fixed "[PATCH 1.1 v2]" that can be merged for QEMU 1.1?
Please go directly through Anthony Liguori or another qemu.git committer
instead of qemu-trivial for QEMU 1.1 fixes.
Stefan
^ permalink raw reply [flat|nested] 7+ messages in thread
* [Qemu-devel] [PATCH] qdev: Fix memory leak
@ 2012-05-03 7:02 dunrong huang
2012-05-03 8:33 ` dunrong huang
0 siblings, 1 reply; 7+ messages in thread
From: dunrong huang @ 2012-05-03 7:02 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-trivial, 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 = qdev_get_prop_ptr(dev, prop);
Error *local_err = NULL;
int i, pos;
- char *str, *p;
+ char *str = NULL, *p;
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;
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)
--
1.7.8.4
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [Qemu-devel] [PATCH] qdev: Fix memory leak
2012-05-03 7:02 [Qemu-devel] " dunrong huang
@ 2012-05-03 8:33 ` dunrong huang
0 siblings, 0 replies; 7+ messages in thread
From: dunrong huang @ 2012-05-03 8:33 UTC (permalink / raw)
To: qemu-devel
[-- Attachment #1: Type: text/plain, Size: 2079 bytes --]
Sorry, i miss the "[PATCH]" in mail's title, i will resend it again.
2012/5/3 dunrong huang <riegamaths@gmail.com>
> 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 = qdev_get_prop_ptr(dev, prop);
> Error *local_err = NULL;
> int i, pos;
> - char *str, *p;
> + char *str = NULL, *p;
>
> 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;
>
> 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)
> --
> 1.7.8.4
>
>
--
linuxer and emacser and pythoner living in beijing
blog: http://mathslinux.org
twitter: https://twitter.com/mathslinux
google+: https://plus.google.com/118129852578326338750
[-- Attachment #2: Type: text/html, Size: 2895 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2012-05-18 9:11 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-05-03 8:34 [Qemu-devel] [PATCH] qdev: Fix memory leak dunrong huang
2012-05-14 19:36 ` Stefan Weil
2012-05-14 20:40 ` Michael Roth
2012-05-15 3:21 ` dunrong huang
2012-05-18 9:10 ` [Qemu-devel] [Qemu-trivial] " Stefan Hajnoczi
-- strict thread matches above, loose matches on Subject: below --
2012-05-03 7:02 [Qemu-devel] " dunrong huang
2012-05-03 8:33 ` dunrong huang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).