10:15 p.m.
Hi,
we have found an issue caused by 321a28c6 "libxl: set default disk format
in device post-parse" (and maybe more changes I couldn't identify yet).
TL;DR: The auto-added driver section in xen changed and now causes issues.
An example to to trigger:
1. Create a XEN xml with a disk device of type cdrom, but do not add a
driver section, like.
<disk type='file' device='cdrom'>
<target dev='hdb' bus='ide'/>
<readonly/>
</disk>
2. on virsh define if that XML
2b. You could also "virsh edit" any working xml file, remove the
<driver../> which will trigger the same
2c. You could also define via virt-manager as it does not explicitly
specify the device section
What happens is that before the changes this auto-added a driver section
like:
<driver name='qemu' type='raw'/>
But now it does only add
<driver type='raw'/>
Which fails to verify like:
Interestingly the same is not true for KVM, there the section is as it was
in the past which made it more likely the post-parse step might be involved.
Also rather expected, if one adds a full <driver name='qemu'
type='raw'/>
in the XML libvirt doesn't have to provide (broken) defaults and things
work as they should - yet from a user with formerly working XMLs or using
virt-manager this is a regression.
I have beg your pardon I seem to not be experienced enough on this part of
libvirt to provide a valid fix - I tried but nothing good came out so far.
For now I reverted 321a28c6 which also affects a test later added by
4cd3f241. With those changes it will not add anything which lets us start
Xen VMs for now, but it clearly isn't a solution.
I wanted to ask you if you could take a look into this?
--
Christian Ehrhardt
Software Engineer, Ubuntu Server
Canonical Ltd
11:35 p.m.
Not sure how stupid it might be so clearly just a very humble RFC, but the
following seems to work for me:
Therefore no nicely polished patch, but just inline diff
--- a/src/libxl/libxl_domain.c
+++ b/src/libxl/libxl_domain.c
@@ -367,8 +367,9 @@
int actual_type = virStorageSourceGetActualType(disk->src);
int format = virDomainDiskGetFormat(disk);
- /* for network-based disks, set 'qemu' as the default driver */
- if (actual_type == VIR_STORAGE_TYPE_NETWORK) {
+ /* for network-based disk and cdrom, set 'qemu' as the default
driver */
+ if (actual_type == VIR_STORAGE_TYPE_NETWORK ||
+ disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM) {
if (!virDomainDiskGetDriver(disk) &&
virDomainDiskSetDriver(disk, "qemu") < 0)
return -1;
Opinions?
If it seems remotely reasonable I'm totally fine submitting a patch in more
style with proper headers and such.
6:36 a.m.
On 07/12/2017 09:35 AM, Christian Ehrhardt wrote:
Not sure how stupid it might be so clearly just a very humble RFC,
but the
following seems to work for me:
Therefore no nicely polished patch, but just inline diff
--- a/src/libxl/libxl_domain.c
+++ b/src/libxl/libxl_domain.c
@@ -367,8 +367,9 @@
int actual_type = virStorageSourceGetActualType(disk->src);
int format = virDomainDiskGetFormat(disk);
- /* for network-based disks, set 'qemu' as the default driver */
- if (actual_type == VIR_STORAGE_TYPE_NETWORK) {
+ /* for network-based disk and cdrom, set 'qemu' as the default driver
*/
+ if (actual_type == VIR_STORAGE_TYPE_NETWORK ||
+ disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM) {
if (!virDomainDiskGetDriver(disk) &&
virDomainDiskSetDriver(disk, "qemu") < 0)
return -1;
This might be useful regardless of the answer to my question about the
disk/driver/@name attribute. AFAIK, the only backend in Xen that supports CDROM
is qemu.
Opinions?
If it seems remotely reasonable I'm totally fine submitting a patch in more
style with proper headers and such.
Unless my statement above is incorrect, I think submitting a formal patch would
be fine.
Regards,
Jim
10:42 p.m.
New subject: [libvirt] [PATCH] libxl: fix cdrom default driver name
In recent libvirt libxl does fill non-existant driver elements with type='raw',
but lacks the name='qemu' attribute that it had in the past.
Without that the xml fails to validate, therefore old XML files can't be
edited/defined anymore.
Since the cdrom device type only works with the qemu driver, add that as
the default attribute just as it was added in the past.
Example of the verification error:
$ virt-xml-validate mytest.xml
Relax-NG validity error : Extra element devices in interleave
Signed-off-by: Christian Ehrhardt <christian.ehrhardt(a)canonical.com>
---
src/libxl/libxl_domain.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
index 68a501c..70ddcea 100644
--- a/src/libxl/libxl_domain.c
+++ b/src/libxl/libxl_domain.c
@@ -367,8 +367,9 @@ libxlDomainDeviceDefPostParse(virDomainDeviceDefPtr dev,
int actual_type = virStorageSourceGetActualType(disk->src);
int format = virDomainDiskGetFormat(disk);
- /* for network-based disks, set 'qemu' as the default driver */
- if (actual_type == VIR_STORAGE_TYPE_NETWORK) {
+ /* for network-based disk and cdrom, set 'qemu' as the default driver */
+ if (actual_type == VIR_STORAGE_TYPE_NETWORK ||
+ disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM) {
if (!virDomainDiskGetDriver(disk) &&
virDomainDiskSetDriver(disk, "qemu") < 0)
return -1;
--
2.7.4
1:10 a.m.
New subject: [libvirt] [PATCH] libxl: fix cdrom default driver name
On 07/17/2017 10:42 AM, Christian Ehrhardt wrote:
In recent libvirt libxl does fill non-existant driver elements with
type='raw',
but lacks the name='qemu' attribute that it had in the past.
Without that the xml fails to validate, therefore old XML files can't be
edited/defined anymore.
Since the cdrom device type only works with the qemu driver, add that as
the default attribute just as it was added in the past.
Example of the verification error:
$ virt-xml-validate mytest.xml
Relax-NG validity error : Extra element devices in interleave
Signed-off-by: Christian Ehrhardt <christian.ehrhardt(a)canonical.com>
---
src/libxl/libxl_domain.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
index 68a501c..70ddcea 100644
--- a/src/libxl/libxl_domain.c
+++ b/src/libxl/libxl_domain.c
@@ -367,8 +367,9 @@ libxlDomainDeviceDefPostParse(virDomainDeviceDefPtr dev,
int actual_type = virStorageSourceGetActualType(disk->src);
int format = virDomainDiskGetFormat(disk);
- /* for network-based disks, set 'qemu' as the default driver */
- if (actual_type == VIR_STORAGE_TYPE_NETWORK) {
+ /* for network-based disk and cdrom, set 'qemu' as the default driver
*/
+ if (actual_type == VIR_STORAGE_TYPE_NETWORK ||
+ disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM) {
if (!virDomainDiskGetDriver(disk) &&
virDomainDiskSetDriver(disk, "qemu") < 0)
return -1;
So is it the case that libxl never uses this attribute, and it's just
being added to the config in order to pass RNG validation? If so, then
my opinion would be that the RNG is too strict, and that attribute
should be made optional.
1:49 a.m.
New subject: [libvirt] [PATCH] libxl: fix cdrom default driver name
On 07/18/2017 11:10 AM, Laine Stump wrote:
On 07/17/2017 10:42 AM, Christian Ehrhardt wrote:
> In recent libvirt libxl does fill non-existant driver elements with
type='raw',
> but lacks the name='qemu' attribute that it had in the past.
> Without that the xml fails to validate, therefore old XML files can't be
> edited/defined anymore.
>
> Since the cdrom device type only works with the qemu driver, add that as
> the default attribute just as it was added in the past.
>
> Example of the verification error:
> $ virt-xml-validate mytest.xml
> Relax-NG validity error : Extra element devices in interleave
>
> Signed-off-by: Christian Ehrhardt <christian.ehrhardt(a)canonical.com>
> ---
> src/libxl/libxl_domain.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
> index 68a501c..70ddcea 100644
> --- a/src/libxl/libxl_domain.c
> +++ b/src/libxl/libxl_domain.c
> @@ -367,8 +367,9 @@ libxlDomainDeviceDefPostParse(virDomainDeviceDefPtr dev,
> int actual_type = virStorageSourceGetActualType(disk->src);
> int format = virDomainDiskGetFormat(disk);
>
> - /* for network-based disks, set 'qemu' as the default driver */
> - if (actual_type == VIR_STORAGE_TYPE_NETWORK) {
> + /* for network-based disk and cdrom, set 'qemu' as the default
driver */
> + if (actual_type == VIR_STORAGE_TYPE_NETWORK ||
> + disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM) {
> if (!virDomainDiskGetDriver(disk) &&
> virDomainDiskSetDriver(disk, "qemu") < 0)
> return -1;
So is it the case that libxl never uses this attribute, and it's just
being added to the config in order to pass RNG validation? If so, then
my opinion would be that the RNG is too strict, and that attribute
should be made optional.
Yep, agreed. I'd rather relax the RNG than restrict CDROM devices to the qemu
backend. Although IIRC qemu is currently the only usable backend wrt eject and
other CDROM-specific operations, blkback should be able to read them just fine.
And perhaps another CDROM backend will appear in Xen in the future. Instead of
forcing the qemu backend here, I've sent a patch to relax the RNG
https://www.redhat.com/archives/libvir-list/2017-July/msg00670.html
Regards,
Jim
6:27 a.m.
On 07/11/2017 08:15 AM, Christian Ehrhardt wrote:
Hi,
Hi,
Thanks for the report and sorry for delayed response. I'm slowly catching up on
libvirt mail after being away for a while...
we have found an issue caused by 321a28c6 "libxl: set default
disk format in
device post-parse" (and maybe more changes I couldn't identify yet).
TL;DR: The auto-added driver section in xen changed and now causes issues.
An example to to trigger:
1. Create a XEN xml with a disk device of type cdrom, but do not add a driver
section, like.
<disk type='file' device='cdrom'>
<target dev='hdb' bus='ide'/>
<readonly/>
</disk>
2. on virsh define if that XML
2b. You could also "virsh edit" any working xml file, remove the
<driver../>
which will trigger the same
2c. You could also define via virt-manager as it does not explicitly specify the
device section
What happens is that before the changes this auto-added a driver section like:
<driver name='qemu' type='raw'/>
But now it does only add
<driver type='raw'/>
Which fails to verify like:
I think you forgot to paste the example here, but I assume it is something like
# virt-xml-validate test.xml
Relax-NG validity error : Extra element devices in interleave
test.xml:21: element devices: Relax-NG validity error : Element domain failed to
validate content
test.xml fails to validate
where test.xml contains disk config
<disk type='file' device='cdrom'>
<driver type='raw'/>
<target dev='hdb' bus='ide'/>
<readonly/>
<address type='drive' controller='0' bus='0'
target='0' unit='1'/>
</disk>
It is not clear to me if 'name' is a required attribute of <disk>. The doc
says
"If the hypervisor supports multiple backend drivers, then the name attribute
selects the primary backend driver name". It doesn't mention "required"
or
"mandatory". We'd have a schema bug if 'name' is optional. I guess
it is a good
question for danpb. Dan, is disk/driver/@name a required attribute?
Regards,
Jim
10:42 p.m.
On Sat, Jul 15, 2017 at 12:27 AM, Jim Fehlig <jfehlig(a)suse.com> wrote:
On 07/11/2017 08:15 AM, Christian Ehrhardt wrote:
>
> What happens is that before the changes this auto-added a driver section
> like:
> <driver name='qemu' type='raw'/>
> But now it does only add
> <driver type='raw'/>
> Which fails to verify like:
>
I think you forgot to paste the example here, but I assume it is something
like
# virt-xml-validate test.xml
Relax-NG validity error : Extra element devices in interleave
test.xml:21: element devices: Relax-NG validity error : Element domain
failed to validate content
test.xml fails to validate
Yes that was the case I've seen.
where test.xml contains disk config
<disk type='file' device='cdrom'>
<driver type='raw'/>
<target dev='hdb' bus='ide'/>
<readonly/>
<address type='drive' controller='0' bus='0'
target='0' unit='1'/>
</disk>
As your example or even no <driver> at all - that in the past also got the
defaults and no isn't.
It is not clear to me if 'name' is a required attribute of
<disk>. The doc
says "If the hypervisor supports multiple backend drivers, then the name
attribute selects the primary backend driver name". It doesn't mention
"required" or "mandatory". We'd have a schema bug if
'name' is optional. I
guess it is a good question for danpb. Dan, is disk/driver/@name a required
attribute?
Prepping my patch, but surely if the fix is to just relax the relaxng to
not have it required I'm good as well.
2662
days inactive
2669
days old
7 comments
3 participants
participants (3)
-
Christian Ehrhardt -
Jim Fehlig -
Laine Stump