diff mbox

[v4,4/6] qdev: add get_pointer_and_free() for temporary strings

Message ID 20140226105334.24f7d55f@nial.usersys.redhat.com
State New
Headers show

Commit Message

Igor Mammedov Feb. 26, 2014, 9:53 a.m. UTC 
On Fri, 21 Feb 2014 15:46:56 +0100
Stefan Hajnoczi <stefanha@redhat.com> wrote:

> get_pointer() assumes the string has unspecified lifetime (at least as
> long as the object is alive).  In some cases we can only produce a
> temporary string that should be freed when get_pointer() is done.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  hw/core/qdev-properties-system.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
> index 5f5957e..3bffc01 100644
> --- a/hw/core/qdev-properties-system.c
> +++ b/hw/core/qdev-properties-system.c
> @@ -31,6 +31,20 @@ static void get_pointer(Object *obj, Visitor *v, Property *prop,
>      visit_type_str(v, &p, name, errp);
>  }
>  
> +/* Same as get_pointer() but frees heap-allocated print() return value */
> +static void get_pointer_and_free(Object *obj, Visitor *v, Property *prop,
> +                                 char *(*print)(void *ptr),
> +                                 const char *name, Error **errp)
> +{
> +    DeviceState *dev = DEVICE(obj);
> +    void **ptr = qdev_get_prop_ptr(dev, prop);
> +    char *p;
> +
> +    p = *ptr ? print(*ptr) : g_strdup("");
> +    visit_type_str(v, &p, name, errp);
> +    g_free(p);
> +}
it could be better if get_pointer() would free pointer if 3 current users
are converted to return temporary string, something like this:

From 65dbdb27e6ccc555e84ddf898d015827fec04d9c Mon Sep 17 00:00:00 2001
From: Igor Mammedov <imammedo@redhat.com>
Date: Wed, 26 Feb 2014 10:37:36 +0100
Subject: [PATCH] qdev: make get_pointer() handle temporary strings

get_pointer()'s print() callback might return a heap allocated
string, to avoid adding dedicated get_pointer_foo for this case
convert current print() callbacks to return temporary heap
allocated string and make get_pointer() free it.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/core/qdev-properties-system.c |   11 +++++++----
 1 files changed, 7 insertions(+), 4 deletions(-)

Comments

Stefan Hajnoczi Feb. 28, 2014, 10:41 a.m. UTC | #1 
On Wed, Feb 26, 2014 at 10:53:34AM +0100, Igor Mammedov wrote:
> On Fri, 21 Feb 2014 15:46:56 +0100
> Stefan Hajnoczi <stefanha@redhat.com> wrote:
> 
> > get_pointer() assumes the string has unspecified lifetime (at least as
> > long as the object is alive).  In some cases we can only produce a
> > temporary string that should be freed when get_pointer() is done.
> > 
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> >  hw/core/qdev-properties-system.c | 14 ++++++++++++++
> >  1 file changed, 14 insertions(+)
> > 
> > diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
> > index 5f5957e..3bffc01 100644
> > --- a/hw/core/qdev-properties-system.c
> > +++ b/hw/core/qdev-properties-system.c
> > @@ -31,6 +31,20 @@ static void get_pointer(Object *obj, Visitor *v, Property *prop,
> >      visit_type_str(v, &p, name, errp);
> >  }
> >  
> > +/* Same as get_pointer() but frees heap-allocated print() return value */
> > +static void get_pointer_and_free(Object *obj, Visitor *v, Property *prop,
> > +                                 char *(*print)(void *ptr),
> > +                                 const char *name, Error **errp)
> > +{
> > +    DeviceState *dev = DEVICE(obj);
> > +    void **ptr = qdev_get_prop_ptr(dev, prop);
> > +    char *p;
> > +
> > +    p = *ptr ? print(*ptr) : g_strdup("");
> > +    visit_type_str(v, &p, name, errp);
> > +    g_free(p);
> > +}
> it could be better if get_pointer() would free pointer if 3 current users
> are converted to return temporary string, something like this:

Yes, that is nicer.  I'll switch to your patch in the next revision.

Stefan
diff mbox

Patch

diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
index 5f5957e..d865ea3 100644
--- a/hw/core/qdev-properties-system.c
+++ b/hw/core/qdev-properties-system.c
@@ -27,8 +27,9 @@  static void get_pointer(Object *obj, Visitor *v, Property *prop,
     void **ptr = qdev_get_prop_ptr(dev, prop);
     char *p;
 
-    p = (char *) (*ptr ? print(*ptr) : "");
+    p = (char *) (*ptr ? print(*ptr) : g_strdup(""));
     visit_type_str(v, &p, name, errp);
+    g_free(p);
 }
 
 static void set_pointer(Object *obj, Visitor *v, Property *prop,
@@ -93,7 +94,7 @@  static void release_drive(Object *obj, const char *name, void *opaque)
 
 static const char *print_drive(void *ptr)
 {
-    return bdrv_get_device_name(ptr);
+    return g_strdup(bdrv_get_device_name(ptr));
 }
 
 static void get_drive(Object *obj, Visitor *v, void *opaque,
@@ -148,8 +149,9 @@  static void release_chr(Object *obj, const char *name, void *opaque)
 static const char *print_chr(void *ptr)
 {
     CharDriverState *chr = ptr;
+    char *val = chr->label ? chr->label : "";
 
-    return chr->label ? chr->label : "";
+    return g_strdup(val);
 }
 
 static void get_chr(Object *obj, Visitor *v, void *opaque,
@@ -227,8 +229,9 @@  err:
 static const char *print_netdev(void *ptr)
 {
     NetClientState *netdev = ptr;
+    char *val = netdev->name ? netdev->name : "";
 
-    return netdev->name ? netdev->name : "";
+    return g_strdup(val);
 }
 
 static void get_netdev(Object *obj, Visitor *v, void *opaque,