[libvirt] [PATCH] qemuDomainAttachDeviceLive: Check provided disk address
https://bugzilla.redhat.com/show_bug.cgi?id=1257844 Imagine an user who is trying to attach a disk to a domain with the following XML: <disk type='block' device='disk'> <driver name='qemu' type='raw'/> <source dev='/dev/sr0'/> <target dev='vde' bus='virtio'/> <address type='drive' controller='0' bus='0' target='0' unit='0'/> </disk> The XML is obviously wrong. It's trying to attach a virtio disk onto non-PCI bus. We should forbid that. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_hotplug.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index afc5408..fc8e21b 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -336,6 +336,13 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn, if (!qemuCheckCCWS390AddressSupport(vm->def, disk->info, priv->qemuCaps, disk->dst)) goto cleanup; + if (disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI && + disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390 && + disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("virtio disks must have PCI or CCW address")); + goto cleanup; + } } for (i = 0; i < vm->def->ndisks; i++) { -- 2.4.9
On Thu, Sep 24, 2015 at 05:43:08PM +0200, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1257844
Imagine an user who is trying to attach a disk to a domain with the following XML:
<disk type='block' device='disk'> <driver name='qemu' type='raw'/> <source dev='/dev/sr0'/> <target dev='vde' bus='virtio'/> <address type='drive' controller='0' bus='0' target='0' unit='0'/> </disk>
The XML is obviously wrong. It's trying to attach a virtio disk onto non-PCI bus. We should forbid that.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_hotplug.c | 7 +++++++ 1 file changed, 7 insertions(+)
How come this is not handled in qemuDomainAssignAddresses(), it doesn't get called? There's a check for exactly that in qemuAssignDevicePCISlots().
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index afc5408..fc8e21b 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -336,6 +336,13 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn, if (!qemuCheckCCWS390AddressSupport(vm->def, disk->info, priv->qemuCaps, disk->dst)) goto cleanup; + if (disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI && + disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390 && + disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("virtio disks must have PCI or CCW address")); + goto cleanup; + } }
for (i = 0; i < vm->def->ndisks; i++) { -- 2.4.9
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 25.09.2015 11:36, Martin Kletzander wrote:
On Thu, Sep 24, 2015 at 05:43:08PM +0200, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1257844
Imagine an user who is trying to attach a disk to a domain with the following XML:
<disk type='block' device='disk'> <driver name='qemu' type='raw'/> <source dev='/dev/sr0'/> <target dev='vde' bus='virtio'/> <address type='drive' controller='0' bus='0' target='0' unit='0'/> </disk>
The XML is obviously wrong. It's trying to attach a virtio disk onto non-PCI bus. We should forbid that.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_hotplug.c | 7 +++++++ 1 file changed, 7 insertions(+)
How come this is not handled in qemuDomainAssignAddresses(), it doesn't get called? There's a check for exactly that in qemuAssignDevicePCISlots().
Exactly! qemuAssignDevicePCISlots() is called only in case of --config. Michal
On 09/25/2015 05:38 AM, Michal Privoznik wrote:
On 25.09.2015 11:36, Martin Kletzander wrote:
On Thu, Sep 24, 2015 at 05:43:08PM +0200, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1257844
Imagine an user who is trying to attach a disk to a domain with the following XML:
<disk type='block' device='disk'> <driver name='qemu' type='raw'/> <source dev='/dev/sr0'/> <target dev='vde' bus='virtio'/> <address type='drive' controller='0' bus='0' target='0' unit='0'/> </disk>
The XML is obviously wrong. It's trying to attach a virtio disk onto non-PCI bus. We should forbid that.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_hotplug.c | 7 +++++++ 1 file changed, 7 insertions(+)
How come this is not handled in qemuDomainAssignAddresses(), it doesn't get called? There's a check for exactly that in qemuAssignDevicePCISlots().
Exactly! qemuAssignDevicePCISlots() is called only in case of --config.
Seems to me this may be more of a generic problem - a user providing the wrong address type for the type of device. I have a recollection of discussing this while having a patch series reviewed in the last month or two. Still searching for that conversation - I thought it was during my series with SCSI hostdev and disk address assignment conflicts, but that was geared more towards two user address supplied disks could have the same address and we don't check that. It may also have been during the similar CCW/s390 series where a ccw/s390 address was used when it shouldn't have been (bug 1258361), which is more alike this bug. John
On Fri, Sep 25, 2015 at 06:41:44AM -0400, John Ferlan wrote:
On 09/25/2015 05:38 AM, Michal Privoznik wrote:
On 25.09.2015 11:36, Martin Kletzander wrote:
On Thu, Sep 24, 2015 at 05:43:08PM +0200, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1257844
Imagine an user who is trying to attach a disk to a domain with the following XML:
<disk type='block' device='disk'> <driver name='qemu' type='raw'/> <source dev='/dev/sr0'/> <target dev='vde' bus='virtio'/> <address type='drive' controller='0' bus='0' target='0' unit='0'/> </disk>
The XML is obviously wrong. It's trying to attach a virtio disk onto non-PCI bus. We should forbid that.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_hotplug.c | 7 +++++++ 1 file changed, 7 insertions(+)
How come this is not handled in qemuDomainAssignAddresses(), it doesn't get called? There's a check for exactly that in qemuAssignDevicePCISlots().
Exactly! qemuAssignDevicePCISlots() is called only in case of --config.
Seems to me this may be more of a generic problem - a user providing the wrong address type for the type of device. I have a recollection of
Yes, and since we have checks for those, it's confusing to me why would qemuAssignDevicePCISlots() be called only with --config. Can we use that code which checks for more things already? For example, the here-missing virtio-mmio.
discussing this while having a patch series reviewed in the last month or two. Still searching for that conversation - I thought it was during my series with SCSI hostdev and disk address assignment conflicts, but that was geared more towards two user address supplied disks could have the same address and we don't check that. It may also have been during the similar CCW/s390 series where a ccw/s390 address was used when it shouldn't have been (bug 1258361), which is more alike this bug.
John
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 25.09.2015 14:45, Martin Kletzander wrote:
On Fri, Sep 25, 2015 at 06:41:44AM -0400, John Ferlan wrote:
On 09/25/2015 05:38 AM, Michal Privoznik wrote:
On 25.09.2015 11:36, Martin Kletzander wrote:
On Thu, Sep 24, 2015 at 05:43:08PM +0200, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1257844
Imagine an user who is trying to attach a disk to a domain with the following XML:
<disk type='block' device='disk'> <driver name='qemu' type='raw'/> <source dev='/dev/sr0'/> <target dev='vde' bus='virtio'/> <address type='drive' controller='0' bus='0' target='0' unit='0'/> </disk>
The XML is obviously wrong. It's trying to attach a virtio disk onto non-PCI bus. We should forbid that.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_hotplug.c | 7 +++++++ 1 file changed, 7 insertions(+)
How come this is not handled in qemuDomainAssignAddresses(), it doesn't get called? There's a check for exactly that in qemuAssignDevicePCISlots().
Exactly! qemuAssignDevicePCISlots() is called only in case of --config.
Seems to me this may be more of a generic problem - a user providing the wrong address type for the type of device. I have a recollection of
Yes, and since we have checks for those, it's confusing to me why would qemuAssignDevicePCISlots() be called only with --config. Can we use that code which checks for more things already? For example, the here-missing virtio-mmio.
Yes and no. qemuDomainAssignAddresses() expects to see vm->def which already has the device attached. Moreover, it not only works over all devices in the domain (filling in all the missing addresses), but possibly creating new controllers too (e.g. plugging new pci bridges if running out of addresses on a bus). Does anybody have a bright idea how this can be fixed apart from obvious one - breaking whole address assignment code into parts and reassembling it back together again? Michal
On Tue, Sep 29, 2015 at 05:03:07PM +0200, Michal Privoznik wrote:
On 25.09.2015 14:45, Martin Kletzander wrote:
On Fri, Sep 25, 2015 at 06:41:44AM -0400, John Ferlan wrote:
On 09/25/2015 05:38 AM, Michal Privoznik wrote:
On 25.09.2015 11:36, Martin Kletzander wrote:
On Thu, Sep 24, 2015 at 05:43:08PM +0200, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1257844
Imagine an user who is trying to attach a disk to a domain with the following XML:
<disk type='block' device='disk'> <driver name='qemu' type='raw'/> <source dev='/dev/sr0'/> <target dev='vde' bus='virtio'/> <address type='drive' controller='0' bus='0' target='0' unit='0'/> </disk>
The XML is obviously wrong. It's trying to attach a virtio disk onto non-PCI bus. We should forbid that.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_hotplug.c | 7 +++++++ 1 file changed, 7 insertions(+)
How come this is not handled in qemuDomainAssignAddresses(), it doesn't get called? There's a check for exactly that in qemuAssignDevicePCISlots().
Exactly! qemuAssignDevicePCISlots() is called only in case of --config.
Seems to me this may be more of a generic problem - a user providing the wrong address type for the type of device. I have a recollection of
Yes, and since we have checks for those, it's confusing to me why would qemuAssignDevicePCISlots() be called only with --config. Can we use that code which checks for more things already? For example, the here-missing virtio-mmio.
Yes and no. qemuDomainAssignAddresses() expects to see vm->def which already has the device attached. Moreover, it not only works over all devices in the domain (filling in all the missing addresses), but possibly creating new controllers too (e.g. plugging new pci bridges if running out of addresses on a bus).
Does anybody have a bright idea how this can be fixed apart from obvious one - breaking whole address assignment code into parts and reassembling it back together again?
Unfortunately, not. Well, maybe the address allocation I was trying to push as GSoC and/or Outreachy project could've fixed that, but nobody was able to finish that, so we don't know =)
Michal
On 09/29/2015 11:03 AM, Michal Privoznik wrote:
On 25.09.2015 14:45, Martin Kletzander wrote:
On Fri, Sep 25, 2015 at 06:41:44AM -0400, John Ferlan wrote:
On 09/25/2015 05:38 AM, Michal Privoznik wrote:
On 25.09.2015 11:36, Martin Kletzander wrote:
On Thu, Sep 24, 2015 at 05:43:08PM +0200, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1257844
Imagine an user who is trying to attach a disk to a domain with the following XML:
<disk type='block' device='disk'> <driver name='qemu' type='raw'/> <source dev='/dev/sr0'/> <target dev='vde' bus='virtio'/> <address type='drive' controller='0' bus='0' target='0' unit='0'/> </disk>
The XML is obviously wrong. It's trying to attach a virtio disk onto non-PCI bus. We should forbid that.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_hotplug.c | 7 +++++++ 1 file changed, 7 insertions(+)
How come this is not handled in qemuDomainAssignAddresses(), it doesn't get called? There's a check for exactly that in qemuAssignDevicePCISlots().
Exactly! qemuAssignDevicePCISlots() is called only in case of --config.
Seems to me this may be more of a generic problem - a user providing the wrong address type for the type of device. I have a recollection of
Yes, and since we have checks for those, it's confusing to me why would qemuAssignDevicePCISlots() be called only with --config. Can we use that code which checks for more things already? For example, the here-missing virtio-mmio.
Yes and no. qemuDomainAssignAddresses() expects to see vm->def which already has the device attached. Moreover, it not only works over all devices in the domain (filling in all the missing addresses), but possibly creating new controllers too (e.g. plugging new pci bridges if running out of addresses on a bus).
Does anybody have a bright idea how this can be fixed apart from obvious one - breaking whole address assignment code into parts and reassembling it back together again?
From the v4 from Ruifeng Beng:
http://www.redhat.com/archives/libvir-list/2015-September/msg00524.html which has some validity w/r/t using qemuCheckDiskConfig; however, I'm wondering now if that's far too late in processing. Is it agreeable that the checks in virDomainDeviceAddressTypeIsValid will cover the cases when the incoming device has an address in the XML? Why not use the power of virDomainDeviceDefPostParseInternal? There's a check in that code: if (disk->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE && virDomainDiskDefAssignAddress(xmlopt, disk, def) < 0) return -1; why not make an "else" which says, let's be sure the disk->info.type provided is valid? I've attached a patch which works for the cases shown in the bz (both no params and --config). This would solve attach, update, and detach since each would call the virDomainDeviceDefParse which calls the PostParse code. John NOTE: The change to the test is because the failure now occurs during parse rather than at run (e.g. earlier, where I think it should).
On Tue, Sep 29, 2015 at 05:27:58PM -0400, John Ferlan wrote:
On 09/29/2015 11:03 AM, Michal Privoznik wrote:
On 25.09.2015 14:45, Martin Kletzander wrote:
On Fri, Sep 25, 2015 at 06:41:44AM -0400, John Ferlan wrote:
On 09/25/2015 05:38 AM, Michal Privoznik wrote:
On 25.09.2015 11:36, Martin Kletzander wrote:
On Thu, Sep 24, 2015 at 05:43:08PM +0200, Michal Privoznik wrote: > https://bugzilla.redhat.com/show_bug.cgi?id=1257844 > > Imagine an user who is trying to attach a disk to a domain with > the following XML: > > <disk type='block' device='disk'> > <driver name='qemu' type='raw'/> > <source dev='/dev/sr0'/> > <target dev='vde' bus='virtio'/> > <address type='drive' controller='0' bus='0' target='0' unit='0'/> > </disk> > > The XML is obviously wrong. It's trying to attach a virtio disk > onto non-PCI bus. We should forbid that. > > Signed-off-by: Michal Privoznik <mprivozn@redhat.com> > --- > src/qemu/qemu_hotplug.c | 7 +++++++ > 1 file changed, 7 insertions(+) >
How come this is not handled in qemuDomainAssignAddresses(), it doesn't get called? There's a check for exactly that in qemuAssignDevicePCISlots().
Exactly! qemuAssignDevicePCISlots() is called only in case of --config.
Seems to me this may be more of a generic problem - a user providing the wrong address type for the type of device. I have a recollection of
Yes, and since we have checks for those, it's confusing to me why would qemuAssignDevicePCISlots() be called only with --config. Can we use that code which checks for more things already? For example, the here-missing virtio-mmio.
Yes and no. qemuDomainAssignAddresses() expects to see vm->def which already has the device attached. Moreover, it not only works over all devices in the domain (filling in all the missing addresses), but possibly creating new controllers too (e.g. plugging new pci bridges if running out of addresses on a bus).
Does anybody have a bright idea how this can be fixed apart from obvious one - breaking whole address assignment code into parts and reassembling it back together again?
From the v4 from Ruifeng Beng:
http://www.redhat.com/archives/libvir-list/2015-September/msg00524.html
which has some validity w/r/t using qemuCheckDiskConfig; however, I'm wondering now if that's far too late in processing. Is it agreeable that the checks in virDomainDeviceAddressTypeIsValid will cover the cases when the incoming device has an address in the XML?
Why not use the power of virDomainDeviceDefPostParseInternal? There's a check in that code:
if (disk->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE && virDomainDiskDefAssignAddress(xmlopt, disk, def) < 0) return -1;
why not make an "else" which says, let's be sure the disk->info.type provided is valid?
I've attached a patch which works for the cases shown in the bz (both no params and --config).
This would solve attach, update, and detach since each would call the virDomainDeviceDefParse which calls the PostParse code.
John
NOTE: The change to the test is because the failure now occurs during parse rather than at run (e.g. earlier, where I think it should).
I agree, and this sounds good, I'd have just two minor additions, see below.
From a03a8aa073eb410d6b713e6f77572edcf0631263 Mon Sep 17 00:00:00 2001 From: John Ferlan <jferlan@redhat.com> Date: Tue, 29 Sep 2015 17:04:11 -0400 Subject: [PATCH] bug 1257844
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/domain_conf.c | 43 +++++++++++++++++++++++++++++++++++++++++++ tests/qemuxml2argvtest.c | 2 +- 2 files changed, 44 insertions(+), 1 deletion(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 393ece7..21439c7 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3962,6 +3962,41 @@ virDomainDefPostParseInternal(virDomainDefPtr def, }
+/* Check if a provided address is valid */ +static bool +virDomainDeviceAddressTypeIsValid(virDomainDiskDefPtr disk)
1) The name suggests it checks the address type of any device, I'd somehow add a "Disk" in the name =) 2) Until anything similar to my proposal [1] is added, this would make daemon lose such domain on reload. Martin [1] https://www.redhat.com/archives/libvir-list/2015-September/msg00698.html
+{ + if (disk->info.type) { + switch (disk->bus) { + case VIR_DOMAIN_DISK_BUS_IDE: + case VIR_DOMAIN_DISK_BUS_SCSI: + case VIR_DOMAIN_DISK_BUS_SATA: + case VIR_DOMAIN_DISK_BUS_FDC: + if (disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE) + return false; + break; + case VIR_DOMAIN_DISK_BUS_USB: + if (disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB) + return false; + break; + case VIR_DOMAIN_DISK_BUS_VIRTIO: + if (disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW && + disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390 && + disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO && + disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) + return false; + break; + case VIR_DOMAIN_DISK_BUS_XEN: + case VIR_DOMAIN_DISK_BUS_UML: + case VIR_DOMAIN_DISK_BUS_SD: + /* no address type currently, return false if address provided */ + return false; + } + } + return true; +} + + /* Check if a drive type address $controller:$bus:$target:$unit is already * taken by a disk or not. */ @@ -4207,6 +4242,14 @@ virDomainDeviceDefPostParseInternal(virDomainDeviceDefPtr dev, if (disk->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE && virDomainDiskDefAssignAddress(xmlopt, disk, def) < 0) return -1; + else if (!virDomainDeviceAddressTypeIsValid(disk)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("disk target '%s' using bus '%s' cannot have " + "an address of type '%s'"), + disk->dst, virDomainDiskBusTypeToString(disk->bus), + virDomainDeviceAddressTypeToString(disk->info.type)); + return -1; + } }
if (dev->type == VIR_DOMAIN_DEVICE_VIDEO) { diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index ae67779..5c0186a 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -843,7 +843,7 @@ mymain(void) DO_TEST("disk-usb-device-removable", QEMU_CAPS_DRIVE, QEMU_CAPS_DEVICE, QEMU_CAPS_DEVICE_USB_STORAGE, QEMU_CAPS_USB_STORAGE_REMOVABLE, QEMU_CAPS_NODEFCONFIG); - DO_TEST_FAILURE("disk-usb-pci", + DO_TEST_PARSE_ERROR("disk-usb-pci", QEMU_CAPS_DRIVE, QEMU_CAPS_DEVICE, QEMU_CAPS_DEVICE_USB_STORAGE, QEMU_CAPS_NODEFCONFIG); DO_TEST("disk-scsi-device", -- 2.1.0
On 09/30/2015 12:43 AM, Martin Kletzander wrote:
On Tue, Sep 29, 2015 at 05:27:58PM -0400, John Ferlan wrote:
[...]
NOTE: The change to the test is because the failure now occurs during parse rather than at run (e.g. earlier, where I think it should).
I agree, and this sounds good, I'd have just two minor additions, see below.
From a03a8aa073eb410d6b713e6f77572edcf0631263 Mon Sep 17 00:00:00 2001 From: John Ferlan <jferlan@redhat.com> Date: Tue, 29 Sep 2015 17:04:11 -0400 Subject: [PATCH] bug 1257844
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/domain_conf.c | 43 +++++++++++++++++++++++++++++++++++++++++++ tests/qemuxml2argvtest.c | 2 +- 2 files changed, 44 insertions(+), 1 deletion(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 393ece7..21439c7 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3962,6 +3962,41 @@ virDomainDefPostParseInternal(virDomainDefPtr def, }
+/* Check if a provided address is valid */ +static bool +virDomainDeviceAddressTypeIsValid(virDomainDiskDefPtr disk)
1) The name suggests it checks the address type of any device, I'd somehow add a "Disk" in the name =)
yeah - I was merely copying from Ruifeng's code... When I generate an official patch I would attribute the code appropriately...
2) Until anything similar to my proposal [1] is added, this would make daemon lose such domain on reload.
Hmm.. right... yuck - how easy one forgets about that. Although it does seem you have some traction with the other series. Or perhaps new logic that includes a flag that could get passed along to the PostParse function as well and checked in the "new" else condition from only the Live attach, update, detach paths (seems like overkill though). One thing still not resolved is that outside of virsh edit, one wouldn't be able to remove the device using virsh detach-device John
Martin
[1] https://www.redhat.com/archives/libvir-list/2015-September/msg00698.html
[...]
On Wed, Sep 30, 2015 at 09:44:27AM -0400, John Ferlan wrote:
On 09/30/2015 12:43 AM, Martin Kletzander wrote:
On Tue, Sep 29, 2015 at 05:27:58PM -0400, John Ferlan wrote:
[...]
NOTE: The change to the test is because the failure now occurs during parse rather than at run (e.g. earlier, where I think it should).
I agree, and this sounds good, I'd have just two minor additions, see below.
From a03a8aa073eb410d6b713e6f77572edcf0631263 Mon Sep 17 00:00:00 2001 From: John Ferlan <jferlan@redhat.com> Date: Tue, 29 Sep 2015 17:04:11 -0400 Subject: [PATCH] bug 1257844
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/domain_conf.c | 43 +++++++++++++++++++++++++++++++++++++++++++ tests/qemuxml2argvtest.c | 2 +- 2 files changed, 44 insertions(+), 1 deletion(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 393ece7..21439c7 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3962,6 +3962,41 @@ virDomainDefPostParseInternal(virDomainDefPtr def, }
+/* Check if a provided address is valid */ +static bool +virDomainDeviceAddressTypeIsValid(virDomainDiskDefPtr disk)
1) The name suggests it checks the address type of any device, I'd somehow add a "Disk" in the name =)
yeah - I was merely copying from Ruifeng's code... When I generate an official patch I would attribute the code appropriately...
2) Until anything similar to my proposal [1] is added, this would make daemon lose such domain on reload.
Hmm.. right... yuck - how easy one forgets about that. Although it does seem you have some traction with the other series. Or perhaps new logic that includes a flag that could get passed along to the PostParse function as well and checked in the "new" else condition from only the Live attach, update, detach paths (seems like overkill though).
One thing still not resolved is that outside of virsh edit, one wouldn't be able to remove the device using virsh detach-device
I'd say it's OK that you have to re-define it. You'd have to re-define it anyway. While on that note, I'd gladly welcome any opinions in that thread about the invalid domain XML loading ;)
John
Martin
[1] https://www.redhat.com/archives/libvir-list/2015-September/msg00698.html
[...]
participants (3)
-
John Ferlan -
Martin Kletzander -
Michal Privoznik