From e0e290552bbbbd449399c9144b6b5b442b973de7 Mon Sep 17 00:00:00 2001 From: John Ferlan Date: Thu, 26 Feb 2015 12:20:01 -0500 Subject: [PATCH] disk: Disallow duplicated target 'dev' values https://bugzilla.redhat.com/show_bug.cgi?id=1142631 This patch resolves a situation where the same "" can be used for multiple disks in the domain. While the $name is "mostly" advisory regarding the expected order that the disk is added to the domain and not guaranteed to map to the device name in the guest OS, it still should be unique enough such that other domblk* type operations can be performed. Without the patch, the domblklist will list the same Target twice: $ virsh domblklist $dom Target Source ------------------------------------------------ sda /var/lib/libvirt/images/file.qcow2 sda /var/lib/libvirt/images/file.img Additionally, getting domblkstat, domblkerror, domblkinfo, and other block* type calls will not be able to reference the second target. Fortunately, hotplug disallows adding a "third" sda value: $ qemu-img create -f raw /var/lib/libvirt/images/file2.img 10M $ virsh attach-disk $dom /var/lib/libvirt/images/file2.img sda error: Failed to attach disk error: operation failed: target sda already exists $ BUT, it since 'sdb' doesn't exist one would get the following on the same hotplug attempt, but changing to use 'sdb' instead of 'sda' $ virsh attach-disk $dom /var/lib/libvirt/images/file2.img sdb error: Failed to attach disk error: internal error: unable to execute QEMU command 'device_add': Duplicate ID 'scsi0-0-1' for device $ Since we cannot fix this issue at parsing time, the best that can be done so as to not "lose" a domain is to make the check prior to starting the guest with the results as follows: $ virsh start $dom error: Failed to start domain $dom error: XML error: target 'sda' duplicated for disk sources '/var/lib/libvirt/images/file.qcow2' and '/var/lib/libvirt/images/file.img' $ Running 'make check' found a few more instances in the tests where this duplicated target dev value was being used. These also exhibited some duplicated 'id=' values (negating the uniqueness argument of aliases) in the corresponding .args file and of course the *xmlout version of a few input XML files. --- src/conf/domain_conf.c | 27 +++++++++++++++++++ src/conf/domain_conf.h | 1 + src/libvirt_private.syms | 1 + src/qemu/qemu_command.c | 3 +++ .../qemuxml2argv-disk-scsi-disk-split.xml | 2 +- ...ml2argv-disk-scsi-lun-passthrough-sgio.xml | 2 +- ...qemuxml2argv-disk-scsi-lun-passthrough.xml | 2 +- .../qemuxml2argv-disk-source-pool-mode.xml | 2 +- .../qemuxml2argv-disk-source-pool.xml | 2 +- .../qemuxml2argv-pci-bridge-many-disks.args | 4 +-- .../qemuxml2argv-pci-bridge-many-disks.xml | 2 +- .../qemuxml2argv-pci-many.args | 8 +++--- .../qemuxml2argv-pci-many.xml | 4 +-- .../qemuxml2xmlout-disk-source-pool.xml | 2 +- .../qemuxml2xmlout-pci-bridge-many-disks.xml | 2 +- 15 files changed, 48 insertions(+), 16 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 9b7ae3f2ae..c73a221dda 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -11680,6 +11680,33 @@ virDomainDiskControllerMatch(int controller_type, int disk_bus) return false; } +/* Return true if there's a duplicate disk[]->dst name for the same bus */ +bool +virDomainDiskDefDstDuplicates(virDomainDefPtr def) +{ + size_t i, j; + + /* optimization */ + if (def->ndisks <= 1) + return false; + + for (i = 1; i < def->ndisks; i++) { + for (j = 0; j < i; j++) { + if (def->disks[i]->bus == def->disks[j]->bus && + STREQ(def->disks[i]->dst, def->disks[j]->dst)) { + virReportError(VIR_ERR_XML_ERROR, + _("target '%s' duplicated for disk sources " + "'%s' and '%s'"), + def->disks[i]->dst, + NULLSTR(virDomainDiskGetSource(def->disks[i])), + NULLSTR(virDomainDiskGetSource(def->disks[j]))); + return true; + } + } + } + return false; +} + int virDomainDiskIndexByAddress(virDomainDefPtr def, virDevicePCIAddressPtr pci_address, diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 02ddd93a4d..ee95f19d2b 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2574,6 +2574,7 @@ int virDomainEmulatorPinDel(virDomainDefPtr def); void virDomainRNGDefFree(virDomainRNGDefPtr def); +bool virDomainDiskDefDstDuplicates(virDomainDefPtr def); int virDomainDiskIndexByAddress(virDomainDefPtr def, virDevicePCIAddressPtr pci_controller, unsigned int bus, unsigned int target, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 496b0522bb..7b6a386548 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -224,6 +224,7 @@ virDomainDiskBusTypeToString; virDomainDiskCacheTypeFromString; virDomainDiskCacheTypeToString; virDomainDiskDefAssignAddress; +virDomainDiskDefDstDuplicates; virDomainDiskDefForeachPath; virDomainDiskDefFree; virDomainDiskDefNew; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 549853c904..c112619669 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3882,6 +3882,9 @@ qemuBuildDriveDevStr(virDomainDefPtr def, const char *bus = virDomainDiskQEMUBusTypeToString(disk->bus); int controllerModel; + if (virDomainDiskDefDstDuplicates(def)) + goto error; + if (qemuCheckDiskConfig(disk) < 0) goto error; diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-disk-split.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-disk-split.xml index 4402f3eda3..20b6f287f9 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-disk-split.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-disk-split.xml @@ -21,7 +21,7 @@ - +
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-lun-passthrough-sgio.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-lun-passthrough-sgio.xml index 7cf57ec985..4dbbd2995a 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-lun-passthrough-sgio.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-lun-passthrough-sgio.xml @@ -21,7 +21,7 @@ - +
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-lun-passthrough.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-lun-passthrough.xml index 8111ea3d23..3858ede77d 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-lun-passthrough.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-scsi-lun-passthrough.xml @@ -21,7 +21,7 @@ - +
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool-mode.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool-mode.xml index 9f902936bb..c791717232 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool-mode.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool-mode.xml @@ -30,7 +30,7 @@ - +
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool.xml index 95d5be2904..ef095a0920 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool.xml @@ -34,7 +34,7 @@ - +
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pci-bridge-many-disks.args b/tests/qemuxml2argvdata/qemuxml2argv-pci-bridge-many-disks.args index 893eaa15a8..fcd9692407 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-pci-bridge-many-disks.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-pci-bridge-many-disks.args @@ -8,8 +8,8 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \ -device pci-bridge,chassis_nr=3,id=pci.3,bus=pci.0,addr=0x5 \ -usb -drive file=/var/lib/libvirt/images/test.img,if=none,id=drive-virtio-disk0 \ -device virtio-blk-pci,bus=pci.0,addr=0x6,drive=drive-virtio-disk0,id=virtio-disk0 \ --drive file=/var/lib/libvirt/images/disk-a-a.img,if=none,id=drive-virtio-disk0 \ --device virtio-blk-pci,bus=pci.0,addr=0x7,drive=drive-virtio-disk0,id=virtio-disk0 \ +-drive file=/var/lib/libvirt/images/disk-a-a.img,if=none,id=drive-virtio-disk26 \ +-device virtio-blk-pci,bus=pci.0,addr=0x7,drive=drive-virtio-disk26,id=virtio-disk26 \ -drive file=/var/lib/libvirt/images/disk-a-b.img,if=none,id=drive-virtio-disk27 \ -device virtio-blk-pci,bus=pci.0,addr=0x8,drive=drive-virtio-disk27,id=virtio-disk27 \ -drive file=/var/lib/libvirt/images/disk-a-c.img,if=none,id=drive-virtio-disk28 \ diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pci-bridge-many-disks.xml b/tests/qemuxml2argvdata/qemuxml2argv-pci-bridge-many-disks.xml index 1f3ad6ecbc..bc429216e1 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-pci-bridge-many-disks.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-pci-bridge-many-disks.xml @@ -29,7 +29,7 @@ - + diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pci-many.args b/tests/qemuxml2argvdata/qemuxml2argv-pci-many.args index 9551c91de3..0f33065425 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-pci-many.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-pci-many.args @@ -6,10 +6,10 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \ -device ide-drive,bus=ide.1,unit=0,drive=drive-ide0-1-0,id=ide0-1-0 \ -drive file=/var/lib/libvirt/images/test.img,if=none,id=drive-virtio-disk0 \ -device virtio-blk-pci,bus=pci.0,addr=0x3,drive=drive-virtio-disk0,id=virtio-disk0 \ --drive file=/var/lib/libvirt/images/test1.img,if=none,id=drive-virtio-disk0 \ --device virtio-blk-pci,bus=pci.0,addr=0x4,drive=drive-virtio-disk0,id=virtio-disk0 \ --drive file=/var/lib/libvirt/images/disk-a-a.img,if=none,id=drive-virtio-disk0 \ --device virtio-blk-pci,bus=pci.0,addr=0x5,drive=drive-virtio-disk0,id=virtio-disk0 \ +-drive file=/var/lib/libvirt/images/test1.img,if=none,id=drive-virtio-disk1 \ +-device virtio-blk-pci,bus=pci.0,addr=0x4,drive=drive-virtio-disk1,id=virtio-disk1 \ +-drive file=/var/lib/libvirt/images/disk-a-a.img,if=none,id=drive-virtio-disk26 \ +-device virtio-blk-pci,bus=pci.0,addr=0x5,drive=drive-virtio-disk26,id=virtio-disk26 \ -drive file=/var/lib/libvirt/images/disk-a-b.img,if=none,id=drive-virtio-disk27 \ -device virtio-blk-pci,bus=pci.0,addr=0x7,drive=drive-virtio-disk27,id=virtio-disk27 \ -drive file=/var/lib/libvirt/images/disk-a-c.img,if=none,id=drive-virtio-disk28 \ diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pci-many.xml b/tests/qemuxml2argvdata/qemuxml2argv-pci-many.xml index 400470e578..b3c9ab1378 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-pci-many.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-pci-many.xml @@ -42,13 +42,13 @@ - +
- +
diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-source-pool.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-source-pool.xml index e96f76eae8..31e4928caf 100644 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-source-pool.xml +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-source-pool.xml @@ -32,7 +32,7 @@ - +
diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-pci-bridge-many-disks.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-pci-bridge-many-disks.xml index d49f5f4d07..060fe870e6 100644 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-pci-bridge-many-disks.xml +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-pci-bridge-many-disks.xml @@ -30,7 +30,7 @@ - +