Re: [libvirt] [PATCH] Reject duplicate disk targets
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(a)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;
--