Re: [Xen-devel] [PATCH,v2]: xl: don't free string literals

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Wed, 2010-09-08 at 17:31 +0100, Gianni Tedesco wrote:
> The function init_dm_info() is initialising some strings from literals.
> This is bad juju because when the destructor is called we cannot know if
> the string literal was overridden with a strdup()'d value. Therefore
> strdup values in the initialiser then introduce and use the function
> libxlu_cfg_replace_string() which free's whatever is set before
> strdupping the new value on top of it. The rule for the new call should
> be clear due to const vs. non-const arguments - changing the behaviour
> of libxlu_cfg_get_string() would cause more complexity than it saves.
> 
> Signed-off-by: Gianni Tedesco <gianni.tedesco@xxxxxxxxxx>

About to bail for the day, but I guess this (untested) patch makes sense
on top of this?

Avoids c_info->name being a string literal as well. Although I wonder if
maybe domain configurations with no name could just be rejected as
invalid? Having a whole bunch of domains called "test" doesn't seem
likely to be what anyone wants...

Ian.

Subject: xl: use xlu_cfg_replace_string in a few more places.

Signed-off-by: Ian Campbell <ian.campbell@xxxxxxxxxx>

diff -r d6026f6dcebf tools/libxl/xl_cmdimpl.c
--- a/tools/libxl/xl_cmdimpl.c  Wed Sep 08 17:37:47 2010 +0100
+++ b/tools/libxl/xl_cmdimpl.c  Wed Sep 08 17:41:56 2010 +0100
@@ -606,10 +606,8 @@ static void parse_config_data(const char
     if (!xlu_cfg_get_long (config, "hap", &l))
         c_info->hap = l;
 
-    if (!xlu_cfg_get_string (config, "name", &buf))
-        c_info->name = strdup(buf);
-    else
-        c_info->name = "test";
+    if (xlu_cfg_replace_string (config, "name", &c_info->name))
+        c_info->name = strdup("test");
 
     if (!xlu_cfg_get_string (config, "uuid", &buf) ) {
         if ( libxl_uuid_from_string(&c_info->uuid, buf) ) {
@@ -695,8 +693,7 @@ static void parse_config_data(const char
     if (!xlu_cfg_get_long (config, "videoram", &l))
         b_info->video_memkb = l * 1024;
 
-    if (!xlu_cfg_get_string (config, "kernel", &buf))
-        b_info->kernel.path = strdup(buf);
+    xlu_cfg_replace_string (config, "kernel", &b_info->kernel.path);
 
     if (c_info->hvm == 1) {
         if (!xlu_cfg_get_long (config, "pae", &l))
@@ -734,10 +731,8 @@ static void parse_config_data(const char
             exit(1);
         }
 
-        if (!xlu_cfg_get_string (config, "bootloader", &buf))
-            b_info->u.pv.bootloader = strdup(buf);
-        if (!xlu_cfg_get_string (config, "bootloader_args", &buf))
-            b_info->u.pv.bootloader_args = strdup(buf);
+        xlu_cfg_replace_string (config, "bootloader", 
&b_info->u.pv.bootloader);
+        xlu_cfg_replace_string (config, "bootloader_args", 
&b_info->u.pv.bootloader_args);
 
         if (!b_info->u.pv.bootloader && !b_info->kernel.path) {
             fprintf(stderr, "Neither kernel nor bootloader specified\n");
@@ -745,8 +740,7 @@ static void parse_config_data(const char
         }
 
         b_info->u.pv.cmdline = cmdline;
-        if (!xlu_cfg_get_string (config, "ramdisk", &buf))
-            b_info->u.pv.ramdisk.path = strdup(buf);
+        xlu_cfg_replace_string (config, "ramdisk", &b_info->u.pv.ramdisk.path);
     }
 
     if (!xlu_cfg_get_list (config, "disk", &vbds, 0)) {



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel