Re: [libvirt] [PATCH LIBVIRT] libxl: Support cmdline= in xl config files
I went to ping this but noticed that I had sent it to "jimfehlig" (i.e. no domain), so no wonder there was no reply! To: line fixed here, let me know if you would prefer a resend. Ian. On Wed, 2015-12-16 at 12:09 +0000, Ian Campbell wrote:
... and consolidate the cmdline/extra/root parsing to facilitate doing so.
The logic is the same as xl's parse_cmdline from the current xen.git master branch (e6f0e099d2c17de47fd86e817b1998db903cab61), except I was unable to figure out how/where to route the warning about ignoring root+extra if cmdline was specified.
Signed-off-by: Ian Campbell <ian.campbell@citrix.com> --- src/xenconfig/xen_xl.c | 62 ++++++++++++++++++++++++++++++------------ -------- 1 file changed, 37 insertions(+), 25 deletions(-)
diff --git a/src/xenconfig/xen_xl.c b/src/xenconfig/xen_xl.c index 91cdff6..ba8b938 100644 --- a/src/xenconfig/xen_xl.c +++ b/src/xenconfig/xen_xl.c @@ -58,11 +58,45 @@ extern int xlu_disk_parse(XLU_Config *cfg, libxl_device_disk *disk); #endif +static int xenParseCmdline(virConfPtr conf, char **r_cmdline) +{ + char *cmdline = NULL; + const char *root = NULL, *extra = NULL, *buf = NULL; + + if (xenConfigGetString(conf, "cmdline", &buf, NULL) < 0) + return -1; + + if (xenConfigGetString(conf, "root", &root, NULL) < 0) + return -1; + + if (xenConfigGetString(conf, "extra", &extra, NULL) < 0) + return -1; + + if (buf) { + if (VIR_STRDUP(cmdline, buf) < 0) + return -1; + /* root or extra are ignored in this case. */ + } else { + if (root && extra) { + if (virAsprintf(&cmdline, "root=%s %s", root, extra) < 0) + return -1; + } else if (root) { + if (virAsprintf(&cmdline, "root=%s", root) < 0) + return -1; + } else if (extra) { + if (VIR_STRDUP(cmdline, extra) < 0) + return -1; + } + } + + *r_cmdline = cmdline; + return 0; +} + static int xenParseXLOS(virConfPtr conf, virDomainDefPtr def, virCapsPtr caps) { size_t i; - const char *extra, *root; if (def->os.type == VIR_DOMAIN_OSTYPE_HVM) { const char *boot; @@ -84,19 +118,8 @@ xenParseXLOS(virConfPtr conf, virDomainDefPtr def, virCapsPtr caps) if (xenConfigCopyStringOpt(conf, "ramdisk", &def->os.initrd) < 0) return -1; - if (xenConfigGetString(conf, "extra", &extra, NULL) < 0) - return -1; - - if (xenConfigGetString(conf, "root", &root, NULL) < 0) + if (xenParseCmdline(conf, &def->os.cmdline) < 0) return -1; - - if (root) { - if (virAsprintf(&def->os.cmdline, "root=%s %s", root, extra) < 0) - return -1; - } else { - if (VIR_STRDUP(def->os.cmdline, extra) < 0) - return -1; - } #endif if (xenConfigGetString(conf, "boot", &boot, "c") < 0) @@ -132,19 +155,8 @@ xenParseXLOS(virConfPtr conf, virDomainDefPtr def, virCapsPtr caps) if (xenConfigCopyStringOpt(conf, "ramdisk", &def->os.initrd) < 0) return -1; - if (xenConfigGetString(conf, "extra", &extra, NULL) < 0) - return -1; - - if (xenConfigGetString(conf, "root", &root, NULL) < 0) + if (xenParseCmdline(conf, &def->os.cmdline) < 0) return -1; - - if (root) { - if (virAsprintf(&def->os.cmdline, "root=%s %s", root, extra) < 0) - return -1; - } else { - if (VIR_STRDUP(def->os.cmdline, extra) < 0) - return -1; - } } return 0;
On 01/19/2016 05:03 AM, Ian Campbell wrote:
I went to ping this but noticed that I had sent it to "jimfehlig" (i.e. no domain), so no wonder there was no reply!
To: line fixed here, let me know if you would prefer a resend.
That would be much appreciated, thanks!
Ian.
On Wed, 2015-12-16 at 12:09 +0000, Ian Campbell wrote:
... and consolidate the cmdline/extra/root parsing to facilitate doing so.
The logic is the same as xl's parse_cmdline from the current xen.git master branch (e6f0e099d2c17de47fd86e817b1998db903cab61), except I was unable to figure out how/where to route the warning about ignoring root+extra if cmdline was specified.
I think VIR_WARN() would be appropriate.
Signed-off-by: Ian Campbell <ian.campbell@citrix.com> --- src/xenconfig/xen_xl.c | 62 ++++++++++++++++++++++++++++++------------ -------- 1 file changed, 37 insertions(+), 25 deletions(-)
diff --git a/src/xenconfig/xen_xl.c b/src/xenconfig/xen_xl.c index 91cdff6..ba8b938 100644 --- a/src/xenconfig/xen_xl.c +++ b/src/xenconfig/xen_xl.c @@ -58,11 +58,45 @@ extern int xlu_disk_parse(XLU_Config *cfg, libxl_device_disk *disk); #endif
+static int xenParseCmdline(virConfPtr conf, char **r_cmdline) +{ + char *cmdline = NULL; + const char *root = NULL, *extra = NULL, *buf = NULL;
In theory, these three don't need to be initialized since xenConfigGetString will do that. But in practice, I worry that Coverity might complain :-/. Regards, Jim
+ + if (xenConfigGetString(conf, "cmdline", &buf, NULL) < 0) + return -1; + + if (xenConfigGetString(conf, "root", &root, NULL) < 0) + return -1; + + if (xenConfigGetString(conf, "extra", &extra, NULL) < 0) + return -1; + + if (buf) { + if (VIR_STRDUP(cmdline, buf) < 0) + return -1; + /* root or extra are ignored in this case. */ + } else { + if (root && extra) { + if (virAsprintf(&cmdline, "root=%s %s", root, extra) < 0) + return -1; + } else if (root) { + if (virAsprintf(&cmdline, "root=%s", root) < 0) + return -1; + } else if (extra) { + if (VIR_STRDUP(cmdline, extra) < 0) + return -1; + } + } + + *r_cmdline = cmdline; + return 0; +} + static int xenParseXLOS(virConfPtr conf, virDomainDefPtr def, virCapsPtr caps) { size_t i; - const char *extra, *root;
if (def->os.type == VIR_DOMAIN_OSTYPE_HVM) { const char *boot; @@ -84,19 +118,8 @@ xenParseXLOS(virConfPtr conf, virDomainDefPtr def, virCapsPtr caps) if (xenConfigCopyStringOpt(conf, "ramdisk", &def->os.initrd) < 0) return -1;
- if (xenConfigGetString(conf, "extra", &extra, NULL) < 0) - return -1; - - if (xenConfigGetString(conf, "root", &root, NULL) < 0) + if (xenParseCmdline(conf, &def->os.cmdline) < 0) return -1; - - if (root) { - if (virAsprintf(&def->os.cmdline, "root=%s %s", root, extra) < 0) - return -1; - } else { - if (VIR_STRDUP(def->os.cmdline, extra) < 0) - return -1; - } #endif
if (xenConfigGetString(conf, "boot", &boot, "c") < 0) @@ -132,19 +155,8 @@ xenParseXLOS(virConfPtr conf, virDomainDefPtr def, virCapsPtr caps) if (xenConfigCopyStringOpt(conf, "ramdisk", &def->os.initrd) < 0) return -1;
- if (xenConfigGetString(conf, "extra", &extra, NULL) < 0) - return -1; - - if (xenConfigGetString(conf, "root", &root, NULL) < 0) + if (xenParseCmdline(conf, &def->os.cmdline) < 0) return -1; - - if (root) { - if (virAsprintf(&def->os.cmdline, "root=%s %s", root, extra) < 0) - return -1; - } else { - if (VIR_STRDUP(def->os.cmdline, extra) < 0) - return -1; - } }
return 0;
Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
On Tue, 2016-01-19 at 21:46 -0700, Jim Fehlig wrote:
On 01/19/2016 05:03 AM, Ian Campbell wrote:
I went to ping this but noticed that I had sent it to "jimfehlig" (i.e. no domain), so no wonder there was no reply!
To: line fixed here, let me know if you would prefer a resend.
That would be much appreciated, thanks!
Ian.
On Wed, 2015-12-16 at 12:09 +0000, Ian Campbell wrote:
... and consolidate the cmdline/extra/root parsing to facilitate doing so.
The logic is the same as xl's parse_cmdline from the current xen.git master branch (e6f0e099d2c17de47fd86e817b1998db903cab61), except I was unable to figure out how/where to route the warning about ignoring root+extra if cmdline was specified.
I think VIR_WARN() would be appropriate.
Ok, will do, thanks.
Signed-off-by: Ian Campbell <ian.campbell@citrix.com> --- src/xenconfig/xen_xl.c | 62 ++++++++++++++++++++++++++++++-------- ---- -------- 1 file changed, 37 insertions(+), 25 deletions(-)
diff --git a/src/xenconfig/xen_xl.c b/src/xenconfig/xen_xl.c index 91cdff6..ba8b938 100644 --- a/src/xenconfig/xen_xl.c +++ b/src/xenconfig/xen_xl.c @@ -58,11 +58,45 @@ extern int xlu_disk_parse(XLU_Config *cfg, libxl_device_disk *disk); #endif +static int xenParseCmdline(virConfPtr conf, char **r_cmdline) +{ + char *cmdline = NULL; + const char *root = NULL, *extra = NULL, *buf = NULL;
In theory, these three don't need to be initialized since xenConfigGetString will do that. But in practice, I worry that Coverity might complain :-/.
It looks like some of the callers of xenConfigGetString initialise the value to NULL, while others don't. I can't see any public libvirt scan results to look if some of the ones which don't have been picked up or not. I've just noticed also that the code I am moving/removing didn't initialise to NULL, so I think I'll remove these initialisers. Ian.
participants (2)
-
Ian Campbell -
Jim Fehlig