Re: [Qemu-devel] [PATCH v4 4/6] qdev: add get_pointer_and_free() for temporary strings

[Igor Mammedov <imammedo@redhat.com>]

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(-)

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,
-- 
1.7.1



>  static void set_pointer(Object *obj, Visitor *v, Property *prop,
>                          int (*parse)(DeviceState *dev, const char *str,
>                                       void **ptr),