This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
Home Products Support Community News


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

To: Gianni Tedesco <gianni.tedesco@xxxxxxxxxx>
Subject: Re: [Xen-devel] [PATCH,v2]: xl: don't free string literals
From: Ian Campbell <Ian.Campbell@xxxxxxxxxx>
Date: Wed, 8 Sep 2010 17:46:49 +0100
Cc: Xen Devel <xen-devel@xxxxxxxxxxxxxxxxxxx>, Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>
Delivery-date: Wed, 08 Sep 2010 09:48:38 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <1283963489.20276.180.camel@xxxxxxxxxxxxxxxxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
Organization: Citrix Systems, Inc.
References: <1283963489.20276.180.camel@xxxxxxxxxxxxxxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
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...


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
-        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", 
+        xlu_cfg_replace_string (config, "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