* [PATCH,v2]: xl: don't free string literals
@ 2010-09-08 16:31 Gianni Tedesco
2010-09-08 16:46 ` Ian Campbell
2010-09-08 16:46 ` Ian Campbell
0 siblings, 2 replies; 6+ messages in thread
From: Gianni Tedesco @ 2010-09-08 16:31 UTC (permalink / raw)
To: Xen Devel; +Cc: Ian Jackson
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@citrix.com>
diff -r e14a3c281982 tools/libxl/libxlu_cfg.c
--- a/tools/libxl/libxlu_cfg.c Wed Sep 08 16:54:16 2010 +0100
+++ b/tools/libxl/libxlu_cfg.c Wed Sep 08 17:27:05 2010 +0100
@@ -151,7 +151,18 @@ int xlu_cfg_get_string(const XLU_Config
*value_r= set->values[0];
return 0;
}
-
+
+int xlu_cfg_replace_string(const XLU_Config *cfg, const char *n,
+ char **value_r) {
+ XLU_ConfigSetting *set;
+ int e;
+
+ e= find_atom(cfg,n,&set); if (e) return e;
+ free(*value_r);
+ *value_r= strdup(set->values[0]);
+ return 0;
+}
+
int xlu_cfg_get_long(const XLU_Config *cfg, const char *n,
long *value_r) {
long l;
diff -r e14a3c281982 tools/libxl/libxlutil.h
--- a/tools/libxl/libxlutil.h Wed Sep 08 16:54:16 2010 +0100
+++ b/tools/libxl/libxlutil.h Wed Sep 08 17:27:05 2010 +0100
@@ -46,6 +46,7 @@ void xlu_cfg_destroy(XLU_Config*);
*/
int xlu_cfg_get_string(const XLU_Config*, const char *n, const char **value_r);
+int xlu_cfg_replace_string(const XLU_Config *cfg, const char *n, char **value_r); /* free/strdup version */
int xlu_cfg_get_long(const XLU_Config*, const char *n, long *value_r);
int xlu_cfg_get_list(const XLU_Config*, const char *n,
diff -r e14a3c281982 tools/libxl/xl_cmdimpl.c
--- a/tools/libxl/xl_cmdimpl.c Wed Sep 08 16:54:16 2010 +0100
+++ b/tools/libxl/xl_cmdimpl.c Wed Sep 08 17:27:05 2010 +0100
@@ -297,7 +297,7 @@ static void init_dm_info(libxl_device_mo
libxl_uuid_generate(&dm_info->uuid);
dm_info->dom_name = c_info->name;
- dm_info->device_model = "qemu-dm";
+ dm_info->device_model = strdup("qemu-dm");
dm_info->videoram = b_info->video_memkb / 1024;
dm_info->apic = b_info->u.hvm.apic;
dm_info->vcpus = b_info->max_vcpus;
@@ -305,7 +305,7 @@ static void init_dm_info(libxl_device_mo
dm_info->stdvga = 0;
dm_info->vnc = 1;
- dm_info->vnclisten = "127.0.0.1";
+ dm_info->vnclisten = strdup("127.0.0.1");
dm_info->vncdisplay = 0;
dm_info->vncunused = 1;
dm_info->keymap = NULL;
@@ -313,7 +313,7 @@ static void init_dm_info(libxl_device_mo
dm_info->opengl = 0;
dm_info->nographic = 0;
dm_info->serial = NULL;
- dm_info->boot = "cda";
+ dm_info->boot = strdup("cda");
dm_info->usb = 0;
dm_info->usbdevice = NULL;
dm_info->xen_platform_pci = 1;
@@ -1022,38 +1022,30 @@ skip_vfb:
init_dm_info(dm_info, c_info, b_info);
/* then process config related to dm */
- if (!xlu_cfg_get_string (config, "device_model", &buf))
- dm_info->device_model = strdup(buf);
+ !xlu_cfg_replace_string (config, "device_model", &dm_info->device_model);
if (!xlu_cfg_get_long (config, "stdvga", &l))
dm_info->stdvga = l;
if (!xlu_cfg_get_long (config, "vnc", &l))
dm_info->vnc = l;
- if (!xlu_cfg_get_string (config, "vnclisten", &buf))
- dm_info->vnclisten = strdup(buf);
- if (!xlu_cfg_get_string (config, "vncpasswd", &buf))
- dm_info->vncpasswd = strdup(buf);
+ xlu_cfg_replace_string (config, "vnclisten", &dm_info->vnclisten);
+ xlu_cfg_replace_string (config, "vncpasswd", &dm_info->vncpasswd);
if (!xlu_cfg_get_long (config, "vncdisplay", &l))
dm_info->vncdisplay = l;
if (!xlu_cfg_get_long (config, "vncunused", &l))
dm_info->vncunused = l;
- if (!xlu_cfg_get_string (config, "keymap", &buf))
- dm_info->keymap = strdup(buf);
+ xlu_cfg_replace_string (config, "keymap", &dm_info->keymap);
if (!xlu_cfg_get_long (config, "sdl", &l))
dm_info->sdl = l;
if (!xlu_cfg_get_long (config, "opengl", &l))
dm_info->opengl = l;
if (!xlu_cfg_get_long (config, "nographic", &l))
dm_info->nographic = l;
- if (!xlu_cfg_get_string (config, "serial", &buf))
- dm_info->serial = strdup(buf);
- if (!xlu_cfg_get_string (config, "boot", &buf))
- dm_info->boot = strdup(buf);
+ xlu_cfg_replace_string (config, "serial", &dm_info->serial);
+ xlu_cfg_replace_string (config, "boot", &dm_info->boot);
if (!xlu_cfg_get_long (config, "usb", &l))
dm_info->usb = l;
- if (!xlu_cfg_get_string (config, "usbdevice", &buf))
- dm_info->usbdevice = strdup(buf);
- if (!xlu_cfg_get_string (config, "soundhw", &buf))
- dm_info->soundhw = strdup(buf);
+ xlu_cfg_replace_string (config, "usbdevice", &dm_info->usbdevice);
+ xlu_cfg_replace_string (config, "soundhw", &dm_info->soundhw);
if (!xlu_cfg_get_long (config, "xen_platform_pci", &l))
dm_info->xen_platform_pci = l;
}
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH,v2]: xl: don't free string literals
2010-09-08 16:31 [PATCH,v2]: xl: don't free string literals Gianni Tedesco
@ 2010-09-08 16:46 ` Ian Campbell
2010-09-08 16:59 ` Gianni Tedesco
2010-09-08 16:46 ` Ian Campbell
1 sibling, 1 reply; 6+ messages in thread
From: Ian Campbell @ 2010-09-08 16:46 UTC (permalink / raw)
To: Gianni Tedesco; +Cc: Xen Devel, Ian Jackson
On Wed, 2010-09-08 at 17:31 +0100, Gianni Tedesco wrote:
>
> /* then process config related to dm */
> - if (!xlu_cfg_get_string (config, "device_model", &buf))
> - dm_info->device_model = strdup(buf);
> + !xlu_cfg_replace_string (config, "device_model",
> &dm_info->device_model);
Stray ! at the start of the new line?
Ian.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH,v2]: xl: don't free string literals
2010-09-08 16:31 [PATCH,v2]: xl: don't free string literals Gianni Tedesco
2010-09-08 16:46 ` Ian Campbell
@ 2010-09-08 16:46 ` Ian Campbell
2010-09-08 16:58 ` Gianni Tedesco
2010-09-09 17:00 ` Ian Jackson
1 sibling, 2 replies; 6+ messages in thread
From: Ian Campbell @ 2010-09-08 16:46 UTC (permalink / raw)
To: Gianni Tedesco; +Cc: Xen Devel, Ian Jackson
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@citrix.com>
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@citrix.com>
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)) {
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH,v2]: xl: don't free string literals
2010-09-08 16:46 ` Ian Campbell
@ 2010-09-08 16:58 ` Gianni Tedesco
2010-09-09 17:00 ` Ian Jackson
1 sibling, 0 replies; 6+ messages in thread
From: Gianni Tedesco @ 2010-09-08 16:58 UTC (permalink / raw)
To: Ian Campbell; +Cc: Xen Devel, Ian Jackson
On Wed, 2010-09-08 at 17:46 +0100, Ian Campbell wrote:
> 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@citrix.com>
>
> About to bail for the day, but I guess this (untested) patch makes sense
> on top of this?
Yes, very much so
> 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...
Yeah, that looks suspect to me too.
Gianni
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH,v2]: xl: don't free string literals
2010-09-08 16:46 ` Ian Campbell
@ 2010-09-08 16:59 ` Gianni Tedesco
0 siblings, 0 replies; 6+ messages in thread
From: Gianni Tedesco @ 2010-09-08 16:59 UTC (permalink / raw)
To: Ian Campbell; +Cc: Xen Devel, Ian Jackson
On Wed, 2010-09-08 at 17:46 +0100, Ian Campbell wrote:
> On Wed, 2010-09-08 at 17:31 +0100, Gianni Tedesco wrote:
> >
> > /* then process config related to dm */
> > - if (!xlu_cfg_get_string (config, "device_model", &buf))
> > - dm_info->device_model = strdup(buf);
> > + !xlu_cfg_replace_string (config, "device_model",
> > &dm_info->device_model);
>
> Stray ! at the start of the new line?
Indeed.
Jackson, I can re-post this or probably easier for you to fix up
manually before committing?
Gianni
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH,v2]: xl: don't free string literals
2010-09-08 16:46 ` Ian Campbell
2010-09-08 16:58 ` Gianni Tedesco
@ 2010-09-09 17:00 ` Ian Jackson
1 sibling, 0 replies; 6+ messages in thread
From: Ian Jackson @ 2010-09-09 17:00 UTC (permalink / raw)
To: Ian Campbell; +Cc: Xen Devel, Gianni Tedesco
Ian Campbell writes ("Re: [Xen-devel] [PATCH,v2]: xl: don't free string literals"):
> About to bail for the day, but I guess this (untested) patch makes sense
> on top of this?
...
> Subject: xl: use xlu_cfg_replace_string in a few more places.
Yes, I have applied it.
> 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...
Yes. Really we should be rejecting duplicate names.
Ian.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2010-09-09 17:00 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-09-08 16:31 [PATCH,v2]: xl: don't free string literals Gianni Tedesco
2010-09-08 16:46 ` Ian Campbell
2010-09-08 16:59 ` Gianni Tedesco
2010-09-08 16:46 ` Ian Campbell
2010-09-08 16:58 ` Gianni Tedesco
2010-09-09 17:00 ` Ian Jackson
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.