[PATCH LIBVIRT v2] libxl: Support cmdline= in xl config files

[PATCH LIBVIRT v2] libxl: Support cmdline= in xl config files

[Ian Campbell]

... 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

Re: [PATCH LIBVIRT v2] libxl: Support cmdline= in xl config files

[Jim Fehlig]

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

Re: [PATCH LIBVIRT v2] libxl: Support cmdline= in xl config files

[Ian Campbell]

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.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH LIBVIRT v2] libxl: Support cmdline= in xl config files

[Jim Fehlig]

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