Re: [libvirt] [PATCH libvirt] xen: handle root= in xen-xm configuration files.

Re: [libvirt] [PATCH libvirt] xen: handle root= in xen-xm configuration files.

[Eric Blake]

[-- Attachment #1.1: Type: text/plain, Size: 2412 bytes --]

On 06/17/2014 09:24 AM, Ian Campbell wrote:
> In addition to extra= xm supported a root= option which was supposed
> to be incorporated into the final command line. Handle that for "virsh
> domxml-from-native xen-xm". Tested with the libxl backend.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  .gnulib            |    2 +-
>  src/xenxs/xen_xm.c |   14 +++++++++++++-
>  2 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/.gnulib b/.gnulib
> index d55899f..e8e0eb6 160000
> --- a/.gnulib
> +++ b/.gnulib
> @@ -1 +1 @@
> -Subproject commit d55899fd2c5794ac85ecb14d5e2f646a89e4b4dd
> +Subproject commit e8e0eb6bfb728685ec8d5afd924e41b18e9d928d

Was the submodule bump intended?

> diff --git a/src/xenxs/xen_xm.c b/src/xenxs/xen_xm.c
> index b2db97d..2cd6d4c 100644
> --- a/src/xenxs/xen_xm.c
> +++ b/src/xenxs/xen_xm.c
> @@ -339,6 +339,8 @@ xenParseXM(virConfPtr conf, int xendConfigVersion,
>              def->os.nBootDevs++;
>          }
>      } else {
> +        const char *extra, *root;
> +
>          if (xenXMConfigCopyStringOpt(conf, "bootloader", &def->os.bootloader) < 0)
>              goto cleanup;
>          if (xenXMConfigCopyStringOpt(conf, "bootargs", &def->os.bootloaderArgs) < 0)
> @@ -348,8 +350,18 @@ xenParseXM(virConfPtr conf, int xendConfigVersion,
>              goto cleanup;
>          if (xenXMConfigCopyStringOpt(conf, "ramdisk", &def->os.initrd) < 0)
>              goto cleanup;
> -        if (xenXMConfigCopyStringOpt(conf, "extra", &def->os.cmdline) < 0)
> +        if (xenXMConfigGetString(conf, "extra", &extra, NULL) < 0)
> +            goto cleanup;
> +        if (xenXMConfigGetString(conf, "root", &root, NULL) < 0)
>              goto cleanup;

What's the difference between xenXMConfigCopyStringOpt and
xenXMConfigGetString?  Once I understand that, then this patch (minus
the .gnulib bump) seems okay.

> +
> +        if (root) {
> +            if (virAsprintf(&def->os.cmdline, "root=%s %s", root, extra) < 0)
> +                goto cleanup;
> +        } else {
> +            if (VIR_STRDUP(def->os.cmdline, extra) < 0)
> +                goto cleanup;
> +        }
>      }
>  
>      if (xenXMConfigGetULongLong(conf, "memory", &def->mem.cur_balloon,
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [libvirt] [PATCH libvirt] xen: handle root= in xen-xm configuration files.

[Jim Fehlig]

Eric Blake wrote:
> On 06/17/2014 09:24 AM, Ian Campbell wrote:
>   
>> In addition to extra= xm supported a root= option which was supposed
>> to be incorporated into the final command line. Handle that for "virsh
>> domxml-from-native xen-xm". Tested with the libxl backend.
>>
>> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
>> ---
>>  .gnulib            |    2 +-
>>  src/xenxs/xen_xm.c |   14 +++++++++++++-
>>  2 files changed, 14 insertions(+), 2 deletions(-)
>>
>> diff --git a/.gnulib b/.gnulib
>> index d55899f..e8e0eb6 160000
>> --- a/.gnulib
>> +++ b/.gnulib
>> @@ -1 +1 @@
>> -Subproject commit d55899fd2c5794ac85ecb14d5e2f646a89e4b4dd
>> +Subproject commit e8e0eb6bfb728685ec8d5afd924e41b18e9d928d
>>     
>
> Was the submodule bump intended?
>
>   
>> diff --git a/src/xenxs/xen_xm.c b/src/xenxs/xen_xm.c
>> index b2db97d..2cd6d4c 100644
>> --- a/src/xenxs/xen_xm.c
>> +++ b/src/xenxs/xen_xm.c
>> @@ -339,6 +339,8 @@ xenParseXM(virConfPtr conf, int xendConfigVersion,
>>              def->os.nBootDevs++;
>>          }
>>      } else {
>> +        const char *extra, *root;
>> +
>>          if (xenXMConfigCopyStringOpt(conf, "bootloader", &def->os.bootloader) < 0)
>>              goto cleanup;
>>          if (xenXMConfigCopyStringOpt(conf, "bootargs", &def->os.bootloaderArgs) < 0)
>> @@ -348,8 +350,18 @@ xenParseXM(virConfPtr conf, int xendConfigVersion,
>>              goto cleanup;
>>          if (xenXMConfigCopyStringOpt(conf, "ramdisk", &def->os.initrd) < 0)
>>              goto cleanup;
>> -        if (xenXMConfigCopyStringOpt(conf, "extra", &def->os.cmdline) < 0)
>> +        if (xenXMConfigGetString(conf, "extra", &extra, NULL) < 0)
>> +            goto cleanup;
>> +        if (xenXMConfigGetString(conf, "root", &root, NULL) < 0)
>>              goto cleanup;
>>     
>
> What's the difference between xenXMConfigCopyStringOpt and
> xenXMConfigGetString?

The first thing both do is set the pointer in the third param to NULL. 
If the conf setting specified by the second param does not exist or is
empty, both will return success.  The only difference is
xenXMConfigGetString will set the pointer in the third param to the
value provided in the fourth param.  Oh, and xenXMConfigGetString
doesn't alloc any memory, so no need to free 'extra' and 'root'.

>   Once I understand that, then this patch (minus
> the .gnulib bump) seems okay.
>   

Nod, I think it is fine after removing the gnulib hunk.  I tested with
the old Xen driver too.

BTW, if <cmdline> contains root=, I noticed that domxml-to-native will
put it in extra= instead of creating a root= entry.  E.g.
<cmdline>root=/dev/bla foo=bar</cmdline> converts to
extra="root=/dev/bla foo=bar", which is still valid config so perhaps
not such a big deal.

Regards,
Jim

>   
>> +
>> +        if (root) {
>> +            if (virAsprintf(&def->os.cmdline, "root=%s %s", root, extra) < 0)
>> +                goto cleanup;
>> +        } else {
>> +            if (VIR_STRDUP(def->os.cmdline, extra) < 0)
>> +                goto cleanup;
>> +        }
>>      }
>>  
>>      if (xenXMConfigGetULongLong(conf, "memory", &def->mem.cur_balloon,
>>
>>     
>
>   

Re: [PATCH libvirt] xen: handle root= in xen-xm configuration files.

[Ian Campbell]

On Tue, 2014-06-17 at 16:24 +0100, Ian Campbell wrote:
> +        if (xenXMConfigGetString(conf, "extra", &extra, NULL) < 0)

This was subtly broken. The default needs to be "".

-----8<------

>From 539412a6deac8b928c82945d692ef20a49535d65 Mon Sep 17 00:00:00 2001
From: Ian Campbell <ian.campbell@citrix.com>
Date: Tue, 17 Jun 2014 15:48:48 +0100
Subject: [PATCH] xen: handle root= in xen-xm configuration files.

In addition to extra= xm supported a root= option which was supposed
to be incorporated into the final command line. Handle that for "virsh
domxml-from-native xen-xm". Tested with the libxl backend.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
v2: extra should default to "".
---
 .gnulib            |    2 +-

---> WTF. I stripped this out of the patch shown below...

 src/xenxs/xen_xm.c |   14 +++++++++++++-
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/src/xenxs/xen_xm.c b/src/xenxs/xen_xm.c
index b2db97d..745041b 100644
--- a/src/xenxs/xen_xm.c
+++ b/src/xenxs/xen_xm.c
@@ -339,6 +339,8 @@ xenParseXM(virConfPtr conf, int xendConfigVersion,
             def->os.nBootDevs++;
         }
     } else {
+        const char *extra, *root;
+
         if (xenXMConfigCopyStringOpt(conf, "bootloader", &def->os.bootloader) < 0)
             goto cleanup;
         if (xenXMConfigCopyStringOpt(conf, "bootargs", &def->os.bootloaderArgs) < 0)
@@ -348,8 +350,18 @@ xenParseXM(virConfPtr conf, int xendConfigVersion,
             goto cleanup;
         if (xenXMConfigCopyStringOpt(conf, "ramdisk", &def->os.initrd) < 0)
             goto cleanup;
-        if (xenXMConfigCopyStringOpt(conf, "extra", &def->os.cmdline) < 0)
+        if (xenXMConfigGetString(conf, "extra", &extra, "") < 0)
+            goto cleanup;
+        if (xenXMConfigGetString(conf, "root", &root, NULL) < 0)
             goto cleanup;
+
+        if (root) {
+            if (virAsprintf(&def->os.cmdline, "root=%s %s", root, extra) < 0)
+                goto cleanup;
+        } else {
+            if (VIR_STRDUP(def->os.cmdline, extra) < 0)
+                goto cleanup;
+        }
     }
 
     if (xenXMConfigGetULongLong(conf, "memory", &def->mem.cur_balloon,
-- 
1.7.10.4

Re: [libvirt] [PATCH libvirt] xen: handle root= in xen-xm configuration files.

[Ian Campbell]

On Tue, 2014-06-17 at 15:36 -0600, Jim Fehlig wrote:
> Eric Blake wrote:
> > On 06/17/2014 09:24 AM, Ian Campbell wrote:
> >   
> >> In addition to extra= xm supported a root= option which was supposed
> >> to be incorporated into the final command line. Handle that for "virsh
> >> domxml-from-native xen-xm". Tested with the libxl backend.
> >>
> >> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> >> ---
> >>  .gnulib            |    2 +-
> >>  src/xenxs/xen_xm.c |   14 +++++++++++++-
> >>  2 files changed, 14 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/.gnulib b/.gnulib
> >> index d55899f..e8e0eb6 160000
> >> --- a/.gnulib
> >> +++ b/.gnulib
> >> @@ -1 +1 @@
> >> -Subproject commit d55899fd2c5794ac85ecb14d5e2f646a89e4b4dd
> >> +Subproject commit e8e0eb6bfb728685ec8d5afd924e41b18e9d928d
> >>     
> >
> > Was the submodule bump intended?

No, sorry, I've no idea how that happened (/me curses git submodules yet
again).

> >   Once I understand that, then this patch (minus
> > the .gnulib bump) seems okay.

NB I just sent out a v2 -- extra should default to "" not NULL for this
to work as intended.

> BTW, if <cmdline> contains root=, I noticed that domxml-to-native will
> put it in extra= instead of creating a root= entry.  E.g.
> <cmdline>root=/dev/bla foo=bar</cmdline> converts to
> extra="root=/dev/bla foo=bar", which is still valid config so perhaps
> not such a big deal.

I think this is fine.

Personally I consider the root= stuff to be a weird wart, in that it
effectively exposes details of the Linux command line syntax in the
xm/xl cfg file. It's far better IMHO to ignore it and write root=foo in
the actual command line.

Ian.

Re: [PATCH libvirt] xen: handle root= in xen-xm configuration files.

[Jim Fehlig]

Ian Campbell wrote:
> On Tue, 2014-06-17 at 16:24 +0100, Ian Campbell wrote:
>   
>> +        if (xenXMConfigGetString(conf, "extra", &extra, NULL) < 0)
>>     
>
> This was subtly broken. The default needs to be "".
>
> -----8<------
>
> >From 539412a6deac8b928c82945d692ef20a49535d65 Mon Sep 17 00:00:00 2001
> From: Ian Campbell <ian.campbell@citrix.com>
> Date: Tue, 17 Jun 2014 15:48:48 +0100
> Subject: [PATCH] xen: handle root= in xen-xm configuration files.
>
> In addition to extra= xm supported a root= option which was supposed
> to be incorporated into the final command line. Handle that for "virsh
> domxml-from-native xen-xm". Tested with the libxl backend.
>
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
> v2: extra should default to "".
> ---
>  .gnulib            |    2 +-
>
> ---> WTF. I stripped this out of the patch shown below...
>
>  src/xenxs/xen_xm.c |   14 +++++++++++++-
>  2 files changed, 14 insertions(+), 2 deletions(-)
>   

ACK and pushed; thanks!

Regards,
Jim

> diff --git a/src/xenxs/xen_xm.c b/src/xenxs/xen_xm.c
> index b2db97d..745041b 100644
> --- a/src/xenxs/xen_xm.c
> +++ b/src/xenxs/xen_xm.c
> @@ -339,6 +339,8 @@ xenParseXM(virConfPtr conf, int xendConfigVersion,
>              def->os.nBootDevs++;
>          }
>      } else {
> +        const char *extra, *root;
> +
>          if (xenXMConfigCopyStringOpt(conf, "bootloader", &def->os.bootloader) < 0)
>              goto cleanup;
>          if (xenXMConfigCopyStringOpt(conf, "bootargs", &def->os.bootloaderArgs) < 0)
> @@ -348,8 +350,18 @@ xenParseXM(virConfPtr conf, int xendConfigVersion,
>              goto cleanup;
>          if (xenXMConfigCopyStringOpt(conf, "ramdisk", &def->os.initrd) < 0)
>              goto cleanup;
> -        if (xenXMConfigCopyStringOpt(conf, "extra", &def->os.cmdline) < 0)
> +        if (xenXMConfigGetString(conf, "extra", &extra, "") < 0)
> +            goto cleanup;
> +        if (xenXMConfigGetString(conf, "root", &root, NULL) < 0)
>              goto cleanup;
> +
> +        if (root) {
> +            if (virAsprintf(&def->os.cmdline, "root=%s %s", root, extra) < 0)
> +                goto cleanup;
> +        } else {
> +            if (VIR_STRDUP(def->os.cmdline, extra) < 0)
> +                goto cleanup;
> +        }
>      }
>  
>      if (xenXMConfigGetULongLong(conf, "memory", &def->mem.cur_balloon,
>   

Re: [libvirt] [PATCH libvirt] xen: handle root= in xen-xm configuration files.

[Eric Blake]

[-- Attachment #1.1: Type: text/plain, Size: 1467 bytes --]

On 06/18/2014 11:46 AM, Jim Fehlig wrote:

>>
>> In addition to extra= xm supported a root= option which was supposed
>> to be incorporated into the final command line. Handle that for "virsh
>> domxml-from-native xen-xm". Tested with the libxl backend.
>>
>> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
>> ---
>> v2: extra should default to "".
>> ---
>>  .gnulib            |    2 +-
>>
>> ---> WTF. I stripped this out of the patch shown below...
>>
>>  src/xenxs/xen_xm.c |   14 +++++++++++++-
>>  2 files changed, 14 insertions(+), 2 deletions(-)
>>   
> 
> ACK and pushed; thanks!

I'm now seeing test failures on Fedora 20, non-xen setup:

TEST: xmconfigtest
 1) Xen XM-2-XML Parse  paravirt-old-pvfb                             ... OK
 2) Xen XM-2-XML Format paravirt-old-pvfb                             ...
Offset 324
Expect [<]
Actual [  <cmdline></cmdline>
  <]

... FAILED
 3) Xen XM-2-XML Parse  paravirt-old-pvfb-vncdisplay                  ... OK
 4) Xen XM-2-XML Format paravirt-old-pvfb-vncdisplay                  ...
Offset 324
Expect [<]
Actual [  <cmdline></cmdline>
  <]

... FAILED
 5) Xen XM-2-XML Parse  paravirt-new-pvfb                             ... OK

and several others.  Looks like something in the code is outputting an
empty <cmdline> element when it is not needed.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [libvirt] [PATCH libvirt] xen: handle root= in xen-xm configuration files.

[Jim Fehlig]

Eric Blake wrote:
> On 06/18/2014 11:46 AM, Jim Fehlig wrote:
>
>   
>>> In addition to extra= xm supported a root= option which was supposed
>>> to be incorporated into the final command line. Handle that for "virsh
>>> domxml-from-native xen-xm". Tested with the libxl backend.
>>>
>>> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
>>> ---
>>> v2: extra should default to "".
>>> ---
>>>  .gnulib            |    2 +-
>>>
>>> ---> WTF. I stripped this out of the patch shown below...
>>>
>>>  src/xenxs/xen_xm.c |   14 +++++++++++++-
>>>  2 files changed, 14 insertions(+), 2 deletions(-)
>>>   
>>>       
>> ACK and pushed; thanks!
>>     
>
> I'm now seeing test failures on Fedora 20, non-xen setup:
>
> TEST: xmconfigtest
>   

I pushed a fix for this

https://www.redhat.com/archives/libvir-list/2014-June/msg00857.html

Regards,
Jim

Re: [PATCH libvirt] xen: handle root= in xen-xm configuration files.

[Jim Fehlig]

Ian Campbell wrote:
> On Tue, 2014-06-17 at 16:24 +0100, Ian Campbell wrote:
>   
>> +        if (xenXMConfigGetString(conf, "extra", &extra, NULL) < 0)
>>     
>
> This was subtly broken. The default needs to be "".
>   

Turns out, it wasn't :).  Prior to this patch, def->os.cmdline was set
to NULL if 'extra' did not exist or was empty.  I pushed a fix to
restore the behavior, as your original patch did

https://www.redhat.com/archives/libvir-list/2014-June/msg00857.html

Regards,
Jim

Re: [PATCH libvirt] xen: handle root= in xen-xm configuration files.

[Ian Campbell]

On Thu, 2014-06-19 at 00:15 -0600, Jim Fehlig wrote:
> Ian Campbell wrote:
> > On Tue, 2014-06-17 at 16:24 +0100, Ian Campbell wrote:
> >   
> >> +        if (xenXMConfigGetString(conf, "extra", &extra, NULL) < 0)
> >>     
> >
> > This was subtly broken. The default needs to be "".
> >   
> 
> Turns out, it wasn't :).  Prior to this patch, def->os.cmdline was set
> to NULL if 'extra' did not exist or was empty.  I pushed a fix to
> restore the behavior, as your original patch did
> 
> https://www.redhat.com/archives/libvir-list/2014-June/msg00857.html

Sorry about that and thanks for the fix.

I see VIR_STRDUP actually does handle NULL input correctly, which was
what I was worried about.

Ian.

Re: [libvirt] [PATCH libvirt] xen: handle root= in xen-xm configuration files.

[Eric Blake]

[-- Attachment #1.1: Type: text/plain, Size: 420 bytes --]

On 06/24/2014 03:20 AM, Ian Campbell wrote:
> 
> I see VIR_STRDUP actually does handle NULL input correctly, which was
> what I was worried about.

Yes, it is by design that VIR_STRDUP(NULL) works, and gives a different
return value (0) than when dup'ing a string (positive) or on failure
(negative).

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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