[libvirt] [PATCH 2/2] disk: Disallow duplicated target 'dev' values

Martin Kletzander mkletzan at redhat.com
Mon Mar 2 15:13:53 UTC 2015


On Thu, Feb 26, 2015 at 04:59:03PM -0500, John Ferlan wrote:
>https://bugzilla.redhat.com/show_bug.cgi?id=1142631
>
>This patch resolves a situation where the same "<target dev='$name'...>"
>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.
>
>Signed-off-by: John Ferlan <jferlan at redhat.com>
>---
> src/conf/domain_conf.c                             | 29 ++++++++++++++++++++++
> src/conf/domain_conf.h                             |  1 +
> src/libvirt_private.syms                           |  1 +
> src/qemu/qemu_command.c                            |  3 +++
> .../qemuxml2argv-disk-scsi-disk-split.xml          |  2 +-
> ...qemuxml2argv-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 +-
> tests/qemuxml2argvdata/qemuxml2argv-pci-many.args  |  8 +++---
> tests/qemuxml2argvdata/qemuxml2argv-pci-many.xml   |  4 +--
> .../qemuxml2xmlout-disk-source-pool.xml            |  2 +-
> .../qemuxml2xmlout-pci-bridge-many-disks.xml       |  2 +-
> 15 files changed, 50 insertions(+), 16 deletions(-)
>
>diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>index d95dd3e..1a51c1d 100644
>--- a/src/conf/domain_conf.c
>+++ b/src/conf/domain_conf.c
>@@ -11676,6 +11676,35 @@ 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 = 0; i < def->ndisks; i++) {

you could do "i = 1" here ...

>+        for (j = 0; j < def->ndisks; j++) {

... and "j < i" to make it O(n!/2) :)

>+            if (i == j)
>+                continue; /* not ourselves */

And then you can drop this check.

>+            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,

This is kind of tricky.  It is not the target that's causing the
problem, but rather the address we generate.  I tried working it out
somehow, but my approach was not clean at all.  This seems like a more
stable and understandable solution.  Even though I'd like it to be
just temporary.

ACK with the above speed-up fixed.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20150302/5e7f4059/attachment-0001.sig>


More information about the libvir-list mailing list