[libvirt] [PATCH] Reject duplicate disk targets
Martin Kletzander
mkletzan at redhat.com
Wed Jul 10 13:51:45 UTC 2013
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.
> Also, it seems like this problem is more general than just
> disks. Any type of device can have an <address> element
> set and cause a clash, not merely disks attached to controllers.
>
The address duplication problem is already dealt with, but the id we are
creating for disks are dependent on disk->dst. I added the other
address types because if two same disks are connected to different
controllers/addresses, there is no problem.
> So I'd say we want something that iterates over all devices
> in the domaindef and validates every address element.
>
> Yes, we might catch PCI address clashes later in QEMU code,
> but there's no harm in detecting them up front, if we can do
> so in a way that is generally applicable to all address types
>
> Daniel
>
More information about the libvir-list
mailing list