[libvirt] [PATCH LIBVIRT v2] libxl: Support cmdline= in xl config files
... 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). In order to introduce a use of VIR_WARN for logging I had to add virerror.h and VIR_LOG_INIT. Signed-off-by: Ian Campbell <ian.campbell@citrix.com> --- NB: I was unsure if I was supposed to change VIR_FROM_NONE into VIR_FROM_XEN, so I didn't (but will on advice). v2: Use VIR_INFO (adding necessary infra) Don't initialise things to NULL when there is no need. --- src/xenconfig/xen_xl.c | 66 +++++++++++++++++++++++++++++++------------------- 1 file changed, 41 insertions(+), 25 deletions(-) diff --git a/src/xenconfig/xen_xl.c b/src/xenconfig/xen_xl.c index 91cdff6..3d820cc 100644 --- a/src/xenconfig/xen_xl.c +++ b/src/xenconfig/xen_xl.c @@ -27,6 +27,7 @@ #include "virconf.h" #include "virerror.h" +#include "virlog.h" #include "domain_conf.h" #include "viralloc.h" #include "virstring.h" @@ -35,6 +36,8 @@ #define VIR_FROM_THIS VIR_FROM_NONE +VIR_LOG_INIT("xen.xen_xl"); + /* * Xen provides a libxl utility library, with several useful functions, * specifically xlu_disk_parse for parsing xl disk config strings. @@ -58,11 +61,46 @@ extern int xlu_disk_parse(XLU_Config *cfg, libxl_device_disk *disk); #endif +static int xenParseCmdline(virConfPtr conf, char **r_cmdline) +{ + char *cmdline; + const char *root, *extra, *buf; + + 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; + if (root || extra) + VIR_WARN("ignoring root= and extra= in favour of cmdline="); + } 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 +122,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 +159,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; -- 2.1.4
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).
In order to introduce a use of VIR_WARN for logging I had to add virerror.h and VIR_LOG_INIT.
Signed-off-by: Ian Campbell <ian.campbell@citrix.com> --- NB: I was unsure if I was supposed to change VIR_FROM_NONE into VIR_FROM_XEN, so I didn't (but will on advice).
It seems some of the VIR_FROM_ needs cleanup throughout the various Xen-related files. We already have VIR_FROM_SEXPR and VIR_FROM_XENXM. src/xenconfig/xen_sxpr.c should use the former, src/xenconfig/xen_xm.c the latter. And given the pattern, I think we should introduce VIR_FROM_XENXL to cover xl.cfg related code. I can take care of this.
v2: Use VIR_INFO (adding necessary infra) Don't initialise things to NULL when there is no need. --- src/xenconfig/xen_xl.c | 66 +++++++++++++++++++++++++++++++------------------- 1 file changed, 41 insertions(+), 25 deletions(-)
diff --git a/src/xenconfig/xen_xl.c b/src/xenconfig/xen_xl.c index 91cdff6..3d820cc 100644 --- a/src/xenconfig/xen_xl.c +++ b/src/xenconfig/xen_xl.c @@ -27,6 +27,7 @@
#include "virconf.h" #include "virerror.h" +#include "virlog.h" #include "domain_conf.h" #include "viralloc.h" #include "virstring.h" @@ -35,6 +36,8 @@
#define VIR_FROM_THIS VIR_FROM_NONE
+VIR_LOG_INIT("xen.xen_xl"); + /* * Xen provides a libxl utility library, with several useful functions, * specifically xlu_disk_parse for parsing xl disk config strings. @@ -58,11 +61,46 @@ extern int xlu_disk_parse(XLU_Config *cfg, libxl_device_disk *disk); #endif
+static int xenParseCmdline(virConfPtr conf, char **r_cmdline) +{ + char *cmdline;
One too many initializers removed :-).
+ const char *root, *extra, *buf; + + 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; + if (root || extra) + VIR_WARN("ignoring root= and extra= in favour of cmdline="); + } 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;
If none of cmdline, extra, or root are set in the config, def->os.cmdline gets set to garbage. xlconfigtest explodes when running 'make check'. Sorry for not mentioning it in my previous review, but we should add a test for cmdline, root, and extra. Regards, Jim
On Wed, 2016-01-20 at 11:16 -0700, Jim Fehlig wrote:
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).
In order to introduce a use of VIR_WARN for logging I had to add virerror.h and VIR_LOG_INIT.
Signed-off-by: Ian Campbell <ian.campbell@citrix.com> --- NB: I was unsure if I was supposed to change VIR_FROM_NONE into VIR_FROM_XEN, so I didn't (but will on advice).
It seems some of the VIR_FROM_ needs cleanup throughout the various Xen-related files. We already have VIR_FROM_SEXPR and VIR_FROM_XENXM. src/xenconfig/xen_sxpr.c should use the former, src/xenconfig/xen_xm.c the latter. And given the pattern, I think we should introduce VIR_FROM_XENXL to cover xl.cfg related code. I can take care of this.
I've acked you patches about this.
+static int xenParseCmdline(virConfPtr conf, char **r_cmdline) +{ + char *cmdline;
One too many initializers removed :-).
+ const char *root, *extra, *buf; + + 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; + if (root || extra) + VIR_WARN("ignoring root= and extra= in favour of cmdline="); + } 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;
If none of cmdline, extra, or root are set in the config, def->os.cmdline gets set to garbage. xlconfigtest explodes when running 'make check'.
I even thought about this quite carefully but missed this case :-/ Would you prefer and = NULL on the decl or an else cmdline = NULL at the end of that chain?
Sorry for not mentioning it in my previous review, but we should add a test for cmdline, root, and extra.
Ack, will do so for v3. Ian.
Ian Campbell wrote:
On Wed, 2016-01-20 at 11:16 -0700, Jim Fehlig wrote:
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).
In order to introduce a use of VIR_WARN for logging I had to add virerror.h and VIR_LOG_INIT.
Signed-off-by: Ian Campbell <ian.campbell@citrix.com> --- NB: I was unsure if I was supposed to change VIR_FROM_NONE into VIR_FROM_XEN, so I didn't (but will on advice). It seems some of the VIR_FROM_ needs cleanup throughout the various Xen-related files. We already have VIR_FROM_SEXPR and VIR_FROM_XENXM. src/xenconfig/xen_sxpr.c should use the former, src/xenconfig/xen_xm.c the latter. And given the pattern, I think we should introduce VIR_FROM_XENXL to cover xl.cfg related code. I can take care of this.
I've acked you patches about this.
Thanks. I'll push those trivial patches shortly.
+static int xenParseCmdline(virConfPtr conf, char **r_cmdline) +{ + char *cmdline; One too many initializers removed :-).
+ const char *root, *extra, *buf; + + 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; + if (root || extra) + VIR_WARN("ignoring root= and extra= in favour of cmdline="); + } 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; If none of cmdline, extra, or root are set in the config, def->os.cmdline gets set to garbage. xlconfigtest explodes when running 'make check'.
I even thought about this quite carefully but missed this case :-/
Would you prefer and = NULL on the decl or an else cmdline = NULL at the end of that chain?
'char *cmdline = NULL;' is fine.
Sorry for not mentioning it in my previous review, but we should add a test for cmdline, root, and extra.
Ack, will do so for v3.
Thanks! Regards, Jim
participants (2)
-
Ian Campbell -
Jim Fehlig