[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] Reject duplicate disk targets



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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]