[libvirt] [PATCH] qemuDomainAttachDeviceLive: Check provided disk address
Martin Kletzander
mkletzan at redhat.com
Wed Sep 30 04:43:49 UTC 2015
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 at 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 at redhat.com>
>Date: Tue, 29 Sep 2015 17:04:11 -0400
>Subject: [PATCH] bug 1257844
>
>Signed-off-by: John Ferlan <jferlan at 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
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20150930/04de480b/attachment-0001.sig>
More information about the libvir-list
mailing list