[libvirt] [PATCH] Reject duplicate disk targets

[Martin Kletzander]

On 07/10/2013 04:03 PM, Daniel P. Berrange wrote:
> On Wed, Jul 10, 2013 at 03:51:45PM +0200, Martin Kletzander wrote:
>> On 07/10/2013 02:39 PM, Daniel P. Berrange wrote:
>>> On Wed, Jul 10, 2013 at 02:34:30PM +0200, Martin Kletzander wrote:
>>>> We never check for disks with duplicate targets connected to the same
>>>> controller/bus/etc.  That means we go ahead, create the same IDs for
>>>> them and pass them to qemu, which subsequently fails to start.
>>>>
>>>> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=968899
>>>> Signed-off-by: Martin Kletzander <mkletzan at redhat.com>
>>>> ---
>>>>  src/conf/domain_conf.c | 42 +++++++++++++++++++++++++++++++++++++++++-
>>>>  1 file changed, 41 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>>>> index 3398d8b..01720e1 100644
>>>> --- a/src/conf/domain_conf.c
>>>> +++ b/src/conf/domain_conf.c
>>>> @@ -2629,6 +2629,45 @@ virDomainDeviceInfoIterate(virDomainDefPtr def,
>>>>
>>>>
>>>>  static int
>>>> +virDomainDefRejectDuplicateDiskTargets(virDomainDefPtr def)
>>>> +{
>>>> +    char *disk_id = NULL;
>>>> +    int ret = -1;
>>>> +    size_t i = 0;
>>>> +    virHashTablePtr targets = NULL;
>>>> +
>>>> +    if (!(targets = virHashCreate(def->ndisks, NULL)))
>>>> +        goto cleanup;
>>>> +
>>>> +    for (i = 0; i < def->ndisks; i++) {
>>>> +        virDomainDiskDefPtr disk = def->disks[i];
>>>> +
>>>> +        if (virAsprintf(&disk_id, "%d%s%d%d%d%d",
>>>> +                        disk->bus,
>>>> +                        NULLSTR(disk->dst),
>>>> +                        disk->info.addr.drive.controller,
>>>> +                        disk->info.addr.drive.bus,
>>>> +                        disk->info.addr.drive.target,
>>>> +                        disk->info.addr.drive.unit) < 0)
>>>
>>> Err, disk->info.addr is a union of many different types of
>>> address. You can't just arbitrarily reference info.addr.drive
>>> without first validating the type of the union.
>>>
>>
>> I jumped straight into that because we know it is disk and we do it the
>> same way when assigning aliases.  I'd be glad to see what option I
>> missed, thanks for any pointers.
> 
> Assigning aliases is not the same, because the alias is stored
> in the same part of the struct, regardless of address type.
> 
> Any disk with bus=virtio uses PCI addressing. Any disk with
> bus=usb uses USB addresing.
> 

In order to make this as unified as possible, I suggest we check it
when the domain is starting, right after these aliases are assigned.

Reasoning -- there is no problem with using same addresses, this is
already being checked for.  The problem is only with our internal
naming which is externally seen only when we use it for describing
disks to qemu.  Because the way we choose how to describe is based upon
qemu capabilities, it would make sense to do it when starting the
domain then.  Looking back at this patch, it might break some non-qemu
machines.  The patch would then look like the following, would you be
OK with that?

Martin

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 325ef38..f19c83f 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -864,6 +864,32 @@ qemuAssignDeviceControllerAlias(virDomainControllerDefPtr controller)
 }


+static int
+qemuRejectDuplicateDiskIDs(virDomainDefPtr def)
+{
+    int ret = -1;
+    size_t i = 0;
+    virHashTablePtr targets = NULL;
+
+    if (!(targets = virHashCreate(def->ndisks, NULL)))
+        goto cleanup;
+
+    for (i = 0; i < def->ndisks; i++) {
+        if (virHashAddEntry(targets, def->disks[i]->info.alias, NULL) < 0) {
+            virReportError(VIR_ERR_XML_ERROR,
+                           _("Multiple disks with the same alias ('%s')"),
+                           def->disks[i]->info.alias);
+            goto cleanup;
+        }
+    }
+
+    ret = 0;
+ cleanup:
+    virHashFree(targets);
+    return ret;
+}
+
+
 int
 qemuAssignDeviceAliases(virDomainDefPtr def, virQEMUCapsPtr qemuCaps)
 {
@@ -886,6 +912,9 @@ qemuAssignDeviceAliases(virDomainDefPtr def, virQEMUCapsPtr qemuCaps)
         }
     }

+    if (qemuRejectDuplicateDiskIDs(def) < 0)
+        return -1;
+
     if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE))
         return 0;

--