[libvirt] [PATCH 0/5] Reject duplicate drive addresses
Reject duplicate drive addresses at domain definition. Hot-plug for disks and hostdevs is still to fix and this is why the old version of drive address checking is still there. Additionally, it isn't that easy to be sure that the changes won't break other drivers as these are common code changes. Marc Hartmayer (5): conf: simplify functions virDomainSCSIDriveAddressIsUsedBy*() conf: virDomainDriveAddressIsUsedByDisk: Rename type to bus_type tests: don't use duplicate disk addresses conf: add global check for duplicate drive addresses tests: add test cases for address conflicts src/conf/domain_conf.c | 184 +++++++++++++++++---- .../qemuxml2argv-disk-drive-address-conflict.xml | 27 +++ ...xml2argv-disk-hostdev-scsi-address-conflict.xml | 30 ++++ ...emuxml2argv-hostdevs-drive-address-conflict.xml | 33 ++++ .../qemuxml2argv-seclabel-dynamic-override.args | 4 +- .../qemuxml2argv-seclabel-dynamic-override.xml | 2 +- tests/qemuxml2argvtest.c | 8 + .../qemuxml2xmlout-seclabel-dynamic-override.xml | 2 +- 8 files changed, 251 insertions(+), 39 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-address-conflict.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-hostdev-scsi-address-conflict.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hostdevs-drive-address-conflict.xml -- 2.5.5
Pass the virDomainDeviceDriveAddress as a struct instead of individual arguments. Reworked the function descriptions. Signed-off-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> --- src/conf/domain_conf.c | 76 ++++++++++++++++++++++++++++---------------------- 1 file changed, 43 insertions(+), 33 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b001efc..6836d4e 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4016,16 +4016,19 @@ virDomainDefPostParseGraphics(virDomainDef *def) } -/* Check if a drive type address $controller:$bus:$target:$unit is already - * taken by a disk or not. +/** + * virDomainDriveAddressIsUsedByDisk: + * @def: domain definition containing the disks to check + * @type: bus type + * @addr: address to check for duplicates + * + * Return true if any disk is already using the given address on the + * given bus, false otherwise. */ static bool virDomainDriveAddressIsUsedByDisk(const virDomainDef *def, virDomainDiskBus type, - unsigned int controller, - unsigned int bus, - unsigned int target, - unsigned int unit) + const virDomainDeviceDriveAddress *addr) { virDomainDiskDefPtr disk; size_t i; @@ -4037,10 +4040,10 @@ virDomainDriveAddressIsUsedByDisk(const virDomainDef *def, disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE) continue; - if (disk->info.addr.drive.controller == controller && - disk->info.addr.drive.unit == unit && - disk->info.addr.drive.bus == bus && - disk->info.addr.drive.target == target) + if (disk->info.addr.drive.controller == addr->controller && + disk->info.addr.drive.unit == addr->unit && + disk->info.addr.drive.bus == addr->bus && + disk->info.addr.drive.target == addr->target) return true; } @@ -4048,16 +4051,19 @@ virDomainDriveAddressIsUsedByDisk(const virDomainDef *def, } -/* Check if a drive type address $controller:$target:$bus:$unit is already - * taken by a host device or not. +/** + * virDomainDriveAddressIsUsedByHostdev: + * @def: domain definition containing the hostdevs to check + * @type: bus type + * @addr: address to check for duplicates + * + * Return true if any hostdev is already using the given address on the + * given bus, false otherwise. */ static bool virDomainDriveAddressIsUsedByHostdev(const virDomainDef *def, virDomainHostdevSubsysType type, - unsigned int controller, - unsigned int bus, - unsigned int target, - unsigned int unit) + const virDomainDeviceDriveAddress *addr) { virDomainHostdevDefPtr hostdev; size_t i; @@ -4069,10 +4075,10 @@ virDomainDriveAddressIsUsedByHostdev(const virDomainDef *def, hostdev->info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE) continue; - if (hostdev->info->addr.drive.controller == controller && - hostdev->info->addr.drive.unit == unit && - hostdev->info->addr.drive.bus == bus && - hostdev->info->addr.drive.target == target) + if (hostdev->info->addr.drive.controller == addr->controller && + hostdev->info->addr.drive.unit == addr->unit && + hostdev->info->addr.drive.bus == addr->bus && + hostdev->info->addr.drive.target == addr->target) return true; } @@ -4080,24 +4086,29 @@ virDomainDriveAddressIsUsedByHostdev(const virDomainDef *def, } +/** + * virDomainSCSIDriveAddressIsUsed: + * @def: domain definition to check against + * @addr: address to check for duplicates + * + * Return true if the SCSI drive address is already in use, false + * otherwise. + */ static bool virDomainSCSIDriveAddressIsUsed(const virDomainDef *def, - unsigned int controller, - unsigned int bus, - unsigned int target, - unsigned int unit) + const virDomainDeviceDriveAddress *addr) { /* In current implementation, the maximum unit number of a controller * is either 16 or 7 (narrow SCSI bus), and if the maximum unit number * is 16, the controller itself is on unit 7 */ - if (unit == 7) + if (addr->unit == 7) return true; if (virDomainDriveAddressIsUsedByDisk(def, VIR_DOMAIN_DISK_BUS_SCSI, - controller, bus, target, unit) || + addr) || virDomainDriveAddressIsUsedByHostdev(def, VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI, - controller, bus, target, unit)) + addr)) return true; return false; @@ -4114,7 +4125,8 @@ virDomainControllerSCSINextUnit(const virDomainDef *def, for (i = 0; i < max_unit; i++) { /* Default to assigning addresses using bus = target = 0 */ - if (!virDomainSCSIDriveAddressIsUsed(def, controller, 0, 0, i)) + const virDomainDeviceDriveAddress addr = {controller, 0, 0, i}; + if (!virDomainSCSIDriveAddressIsUsed(def, &addr)) return i; } @@ -4270,10 +4282,7 @@ virDomainDeviceDefPostParseInternal(virDomainDeviceDefPtr dev, virDomainDeviceDriveAddressPtr addr = &hdev->info->addr.drive; if (virDomainDriveAddressIsUsedByDisk(def, VIR_DOMAIN_DISK_BUS_SCSI, - addr->controller, - addr->bus, - addr->target, - addr->unit)) { + addr)) { virReportError(VIR_ERR_XML_ERROR, _("SCSI host address controller='%u' " "bus='%u' target='%u' unit='%u' in " @@ -6559,9 +6568,10 @@ virDomainDiskDefAssignAddress(virDomainXMLOptionPtr xmlopt, unit = idx % 7; } + const virDomainDeviceDriveAddress addr = {controller, 0, 0, unit}; if (virDomainDriveAddressIsUsedByHostdev(vmdef, VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI, - controller, 0, 0, unit)) { + &addr)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("using disk target name '%s' conflicts with " "SCSI host device address controller='%u' " -- 2.5.5
Comparing the parameter 'type' against the member 'bus' instead of against the member 'type' is quite confusing. Rename the parameter 'type' to 'bus_type' to clarify its meaning. Signed-off-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> --- src/conf/domain_conf.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 6836d4e..cd30728 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4019,7 +4019,7 @@ virDomainDefPostParseGraphics(virDomainDef *def) /** * virDomainDriveAddressIsUsedByDisk: * @def: domain definition containing the disks to check - * @type: bus type + * @bus_type: bus type * @addr: address to check for duplicates * * Return true if any disk is already using the given address on the @@ -4027,7 +4027,7 @@ virDomainDefPostParseGraphics(virDomainDef *def) */ static bool virDomainDriveAddressIsUsedByDisk(const virDomainDef *def, - virDomainDiskBus type, + virDomainDiskBus bus_type, const virDomainDeviceDriveAddress *addr) { virDomainDiskDefPtr disk; @@ -4036,7 +4036,7 @@ virDomainDriveAddressIsUsedByDisk(const virDomainDef *def, for (i = 0; i < def->ndisks; i++) { disk = def->disks[i]; - if (disk->bus != type || + if (disk->bus != bus_type || disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE) continue; -- 2.5.5
Don't use duplicate disk addresses in test cases unless it's useful. At least the test case will break once we have a check for uniqueness of addresses at time of domain definition. Signed-off-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com> --- tests/qemuxml2argvdata/qemuxml2argv-seclabel-dynamic-override.args | 4 ++-- tests/qemuxml2argvdata/qemuxml2argv-seclabel-dynamic-override.xml | 2 +- tests/qemuxml2xmloutdata/qemuxml2xmlout-seclabel-dynamic-override.xml | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/qemuxml2argvdata/qemuxml2argv-seclabel-dynamic-override.args b/tests/qemuxml2argvdata/qemuxml2argv-seclabel-dynamic-override.args index 6cf8cd8..02fa000 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-seclabel-dynamic-override.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-seclabel-dynamic-override.args @@ -19,6 +19,6 @@ QEMU_AUDIO_DRV=none \ -usb \ -drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \ -device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \ --drive file=/dev/HostVG/QEMUGuest2,format=raw,if=none,id=drive-ide0-0-0 \ --device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \ +-drive file=/dev/HostVG/QEMUGuest2,format=raw,if=none,id=drive-ide0-0-1 \ +-device ide-drive,bus=ide.0,unit=1,drive=drive-ide0-0-1,id=ide0-0-1 \ -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-seclabel-dynamic-override.xml b/tests/qemuxml2argvdata/qemuxml2argv-seclabel-dynamic-override.xml index 448ff3a..71d3a5f 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-seclabel-dynamic-override.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-seclabel-dynamic-override.xml @@ -28,7 +28,7 @@ </seclabel> </source> <target dev='hdb' bus='ide'/> - <address type='drive' controller='0' bus='0' target='0' unit='0'/> + <address type='drive' controller='0' bus='0' target='0' unit='1'/> </disk> <controller type='usb' index='0'/> <controller type='ide' index='0'/> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-seclabel-dynamic-override.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-seclabel-dynamic-override.xml index f84b17c..2b753a5 100644 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-seclabel-dynamic-override.xml +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-seclabel-dynamic-override.xml @@ -28,7 +28,7 @@ </seclabel> </source> <target dev='hdb' bus='ide'/> - <address type='drive' controller='0' bus='0' target='0' unit='0'/> + <address type='drive' controller='0' bus='0' target='0' unit='1'/> </disk> <controller type='usb' index='0'> <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> -- 2.5.5
Add a global check for duplicate drive addresses. This will fix the problem of duplicate disk and hostdev drive addresses. Example for duplicate drive addresses: <disk> ... <target name='sda'/> </disk> <disk> ... <target name='sdb'/> <address type='drive' controller=0 bus=0 target=0 unit=0/> </disk> Another example: <hostdev mode='subsystem' type='scsi' managed='no'> <source> ... </source> <address type='drive' controller='0' bus='0' target='0' unit='0'/> </hostdev> <hostdev mode='subsystem' type='scsi' managed='no'> <source> ... </source> <address type='drive' controller='0' bus='0' target='0' unit='0'/> </hostdev> Unfortunately the fixes (1b08cc170a84077afd4d15f4639a9a2cf398e9a2, 8d46386bfe01b84982e25e915ad9cfbae5cf4cb1) weren't enough to catch these cases and it isn't possible to add additional checks in virDomainDeviceDefPostParseInternal() for SCSI hostdevs or virDomainDiskDefAssignAddress() for SCSI/IDE/FDC/SATA disks without adding another parse flag (virDomainDefParseFlags) to disable this validation while updating or detaching a disk or hostdev. Signed-off-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> --- src/conf/domain_conf.c | 104 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 104 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index cd30728..cb47980 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4841,6 +4841,107 @@ virDomainDefCheckDuplicateDiskInfo(const virDomainDef *def) return 0; } +/** + * virDomainDefCheckDuplicateDriveAddresses: + * @def: domain definition to check against + * + * This function checks @def for duplicate drive addresses. Drive + * addresses are only in use for disks and hostdevs at the moment. + * + * Returns 0 in case of there are no duplicate drive addresses, -1 + * otherwise. + */ +static int +virDomainDefCheckDuplicateDriveAddresses(const virDomainDef *def) +{ + size_t i; + size_t j; + + for (i = 0; i < def->ndisks; i++) { + virDomainDiskDefPtr disk_i = def->disks[i]; + virDomainDeviceInfoPtr disk_info_i = &disk_i->info; + if (disk_info_i->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE) + continue; + + for (j = i + 1; j < def->ndisks; j++) { + virDomainDiskDefPtr disk_j = def->disks[j]; + virDomainDeviceInfoPtr disk_info_j = &disk_j->info; + if (disk_i->bus != disk_j->bus) + continue; + + if (disk_info_j->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE) + continue; + + if (virDomainDeviceInfoAddressIsEqual(disk_info_i, disk_info_j)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Found duplicate drive address for disk with " + "target name '%s' controller='%u' bus='%u' " + "target='%u' unit='%u'"), + disk_i->dst, + disk_info_i->addr.drive.controller, + disk_info_i->addr.drive.bus, + disk_info_i->addr.drive.target, + disk_info_i->addr.drive.unit); + return -1; + } + } + + /* Note: There is no need to check for conflicts with SCSI + * hostdevs above, because conflicts with hostdevs are checked + * in the next loop. + */ + } + + for (i = 0; i < def->nhostdevs; i++) { + virDomainHostdevDefPtr hdev_i = def->hostdevs[i]; + virDomainDeviceInfoPtr hdev_info_i = hdev_i->info; + virDomainDeviceDriveAddressPtr hdev_addr_i; + if (!virHostdevIsSCSIDevice(hdev_i)) + continue; + + if (hdev_i->info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE) + continue; + + hdev_addr_i = &hdev_info_i->addr.drive; + for (j = i + 1; j < def->nhostdevs; j++) { + virDomainHostdevDefPtr hdev_j = def->hostdevs[j]; + virDomainDeviceInfoPtr hdev_info_j = hdev_j->info; + if (!virHostdevIsSCSIDevice(hdev_j)) + continue; + + /* Address type check for hdev_j will be done implicitly + * in virDomainDeviceInfoAddressIsEqual() */ + + if (virDomainDeviceInfoAddressIsEqual(hdev_info_i, hdev_info_j)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("SCSI host address controller='%u' " + "bus='%u' target='%u' unit='%u' in " + "use by another SCSI host device"), + hdev_addr_i->bus, + hdev_addr_i->controller, + hdev_addr_i->target, + hdev_addr_i->unit); + return -1; + } + } + + if (virDomainDriveAddressIsUsedByDisk(def, VIR_DOMAIN_DISK_BUS_SCSI, + hdev_addr_i)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("SCSI host address controller='%u' " + "bus='%u' target='%u' unit='%u' in " + "use by another SCSI disk"), + hdev_addr_i->bus, + hdev_addr_i->controller, + hdev_addr_i->target, + hdev_addr_i->unit); + return -1; + } + } + + return 0; +} + static int virDomainDefValidateInternal(const virDomainDef *def) @@ -4848,6 +4949,9 @@ virDomainDefValidateInternal(const virDomainDef *def) if (virDomainDefCheckDuplicateDiskInfo(def) < 0) return -1; + if (virDomainDefCheckDuplicateDriveAddresses(def) < 0) + return -1; + if (virDomainDefGetVcpusTopology(def, NULL) < 0) return -1; -- 2.5.5
On 30.11.2016 12:47, Marc Hartmayer wrote:
Add a global check for duplicate drive addresses. This will fix the problem of duplicate disk and hostdev drive addresses.
Example for duplicate drive addresses: <disk> ... <target name='sda'/> </disk> <disk> ... <target name='sdb'/> <address type='drive' controller=0 bus=0 target=0 unit=0/> </disk>
Another example: <hostdev mode='subsystem' type='scsi' managed='no'> <source> ... </source> <address type='drive' controller='0' bus='0' target='0' unit='0'/> </hostdev> <hostdev mode='subsystem' type='scsi' managed='no'> <source> ... </source> <address type='drive' controller='0' bus='0' target='0' unit='0'/> </hostdev>
Unfortunately the fixes (1b08cc170a84077afd4d15f4639a9a2cf398e9a2, 8d46386bfe01b84982e25e915ad9cfbae5cf4cb1) weren't enough to catch these cases and it isn't possible to add additional checks in virDomainDeviceDefPostParseInternal() for SCSI hostdevs or virDomainDiskDefAssignAddress() for SCSI/IDE/FDC/SATA disks without adding another parse flag (virDomainDefParseFlags) to disable this validation while updating or detaching a disk or hostdev.
Signed-off-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> --- src/conf/domain_conf.c | 104 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 104 insertions(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index cd30728..cb47980 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4841,6 +4841,107 @@ virDomainDefCheckDuplicateDiskInfo(const virDomainDef *def) return 0; }
+/** + * virDomainDefCheckDuplicateDriveAddresses: + * @def: domain definition to check against + * + * This function checks @def for duplicate drive addresses. Drive + * addresses are only in use for disks and hostdevs at the moment. + * + * Returns 0 in case of there are no duplicate drive addresses, -1 + * otherwise. + */ +static int +virDomainDefCheckDuplicateDriveAddresses(const virDomainDef *def) +{ + size_t i; + size_t j; + + for (i = 0; i < def->ndisks; i++) { + virDomainDiskDefPtr disk_i = def->disks[i]; + virDomainDeviceInfoPtr disk_info_i = &disk_i->info; + if (disk_info_i->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE) + continue;
We like to have an empty line between variable declarations and the actual code. Just like you did at the beginning of this function. But that's something I can fix before pushing.
+ + for (j = i + 1; j < def->ndisks; j++) { + virDomainDiskDefPtr disk_j = def->disks[j]; + virDomainDeviceInfoPtr disk_info_j = &disk_j->info; + if (disk_i->bus != disk_j->bus) + continue; + + if (disk_info_j->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE) + continue; + + if (virDomainDeviceInfoAddressIsEqual(disk_info_i, disk_info_j)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Found duplicate drive address for disk with " + "target name '%s' controller='%u' bus='%u' " + "target='%u' unit='%u'"), + disk_i->dst, + disk_info_i->addr.drive.controller, + disk_info_i->addr.drive.bus, + disk_info_i->addr.drive.target, + disk_info_i->addr.drive.unit); + return -1; + } + } + + /* Note: There is no need to check for conflicts with SCSI + * hostdevs above, because conflicts with hostdevs are checked + * in the next loop. + */ + }
Michal
Add test cases for address conflicts between disks and hostdevs that are using drive addresses. Signed-off-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com> --- .../qemuxml2argv-disk-drive-address-conflict.xml | 27 ++++++++++++++++++ ...xml2argv-disk-hostdev-scsi-address-conflict.xml | 30 ++++++++++++++++++++ ...emuxml2argv-hostdevs-drive-address-conflict.xml | 33 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 8 ++++++ 4 files changed, 98 insertions(+) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-address-conflict.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-hostdev-scsi-address-conflict.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hostdevs-drive-address-conflict.xml diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-address-conflict.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-address-conflict.xml new file mode 100644 index 0000000..83426ab --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-address-conflict.xml @@ -0,0 +1,27 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='sda' bus='sata'/> + </disk> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest2'/> + <target dev='sdb' bus='sata'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-hostdev-scsi-address-conflict.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-hostdev-scsi-address-conflict.xml new file mode 100644 index 0000000..b38ad95 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-hostdev-scsi-address-conflict.xml @@ -0,0 +1,30 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='sda' bus='scsi'/> + </disk> + <hostdev mode='subsystem' type='scsi' managed='no'> + <source> + <adapter name='scsi_host2'/> + <address bus='0' target='0' unit='1'/> + </source> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </hostdev> + <controller type='scsi' index='0' model='virtio-scsi'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hostdevs-drive-address-conflict.xml b/tests/qemuxml2argvdata/qemuxml2argv-hostdevs-drive-address-conflict.xml new file mode 100644 index 0000000..00ac498 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-hostdevs-drive-address-conflict.xml @@ -0,0 +1,33 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <hostdev mode='subsystem' type='scsi' managed='no'> + <source> + <adapter name='scsi_host2'/> + <address bus='0' target='0' unit='1'/> + </source> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </hostdev> + <hostdev mode='subsystem' type='scsi' managed='no'> + <source> + <adapter name='scsi_host2'/> + <address bus='0' target='0' unit='2'/> + </source> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </hostdev> + <controller type='scsi' index='0' model='virtio-scsi'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index d12077c..cd2c4a1 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -975,6 +975,14 @@ mymain(void) DO_TEST_PARSE_ERROR("disk-same-targets", QEMU_CAPS_SCSI_LSI, QEMU_CAPS_DEVICE_USB_STORAGE, QEMU_CAPS_NODEFCONFIG); + DO_TEST_PARSE_ERROR("disk-drive-address-conflict", + QEMU_CAPS_ICH9_AHCI); + DO_TEST_PARSE_ERROR("disk-hostdev-scsi-address-conflict", + QEMU_CAPS_VIRTIO_SCSI, + QEMU_CAPS_DEVICE_SCSI_GENERIC); + DO_TEST_PARSE_ERROR("hostdevs-drive-address-conflict", + QEMU_CAPS_VIRTIO_SCSI, + QEMU_CAPS_DEVICE_SCSI_GENERIC); DO_TEST("event_idx", QEMU_CAPS_VIRTIO_BLK_EVENT_IDX, QEMU_CAPS_VIRTIO_NET_EVENT_IDX, -- 2.5.5
On 30.11.2016 12:47, Marc Hartmayer wrote:
Reject duplicate drive addresses at domain definition. Hot-plug for disks and hostdevs is still to fix and this is why the old version of drive address checking is still there. Additionally, it isn't that easy to be sure that the changes won't break other drivers as these are common code changes.
Marc Hartmayer (5): conf: simplify functions virDomainSCSIDriveAddressIsUsedBy*() conf: virDomainDriveAddressIsUsedByDisk: Rename type to bus_type tests: don't use duplicate disk addresses conf: add global check for duplicate drive addresses tests: add test cases for address conflicts
src/conf/domain_conf.c | 184 +++++++++++++++++---- .../qemuxml2argv-disk-drive-address-conflict.xml | 27 +++ ...xml2argv-disk-hostdev-scsi-address-conflict.xml | 30 ++++ ...emuxml2argv-hostdevs-drive-address-conflict.xml | 33 ++++ .../qemuxml2argv-seclabel-dynamic-override.args | 4 +- .../qemuxml2argv-seclabel-dynamic-override.xml | 2 +- tests/qemuxml2argvtest.c | 8 + .../qemuxml2xmlout-seclabel-dynamic-override.xml | 2 +- 8 files changed, 251 insertions(+), 39 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-address-conflict.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-hostdev-scsi-address-conflict.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hostdevs-drive-address-conflict.xml
ACK series. We are currently in freeze so I will push this after the release. Technically this could go in as it is a bug fix, but rather a big one so I'd rather push it after the release, if you don't mind. Michal
On 02.12.2016 17:28, Michal Privoznik wrote:
On 30.11.2016 12:47, Marc Hartmayer wrote:
Reject duplicate drive addresses at domain definition. Hot-plug for disks and hostdevs is still to fix and this is why the old version of drive address checking is still there. Additionally, it isn't that easy to be sure that the changes won't break other drivers as these are common code changes.
Marc Hartmayer (5): conf: simplify functions virDomainSCSIDriveAddressIsUsedBy*() conf: virDomainDriveAddressIsUsedByDisk: Rename type to bus_type tests: don't use duplicate disk addresses conf: add global check for duplicate drive addresses tests: add test cases for address conflicts
src/conf/domain_conf.c | 184 +++++++++++++++++---- .../qemuxml2argv-disk-drive-address-conflict.xml | 27 +++ ...xml2argv-disk-hostdev-scsi-address-conflict.xml | 30 ++++ ...emuxml2argv-hostdevs-drive-address-conflict.xml | 33 ++++ .../qemuxml2argv-seclabel-dynamic-override.args | 4 +- .../qemuxml2argv-seclabel-dynamic-override.xml | 2 +- tests/qemuxml2argvtest.c | 8 + .../qemuxml2xmlout-seclabel-dynamic-override.xml | 2 +- 8 files changed, 251 insertions(+), 39 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-address-conflict.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-hostdev-scsi-address-conflict.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hostdevs-drive-address-conflict.xml
ACK series. We are currently in freeze so I will push this after the release. Technically this could go in as it is a bug fix, but rather a big one so I'd rather push it after the release, if you don't mind.
I've just pushed these. Thank you for the contribution. Michal
On Mon, Dec 05, 2016 at 11:12 AM +0100, Michal Privoznik <mprivozn@redhat.com> wrote:
ACK series. We are currently in freeze so I will push this after the release. Technically this could go in as it is a bug fix, but rather a big one so I'd rather push it after the release, if you don't mind.
I've just pushed these. Thank you for the contribution.
Michal
Thanks. Marc -- Beste Grüße / Kind regards Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
participants (2)
-
Marc Hartmayer -
Michal Privoznik