[libvirt] [PATCH 25/25] qemu: Check conflicts for shared scsi host device

Osier Yang jyang at redhat.com
Fri May 17 11:31:14 UTC 2013


On 08/05/13 08:05, John Ferlan wrote:
> On 05/03/2013 02:07 PM, Osier Yang wrote:
>> Just like previous patches, this changes qemuCheckSharedDisk
>> into qemuCheckSharedDevice, which takes a virDomainDeviceDefPtr
>> argument instead.
>> ---
>>   src/qemu/qemu_conf.c | 86 +++++++++++++++++++++++++++++++++++++---------------
>>   1 file changed, 61 insertions(+), 25 deletions(-)
>>
> Ahhh finally - never thought I'd get to the last one :-)  Taken longer
> than I wanted!
>
>> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
>> index cf1c7ee..f8264f6 100644
>> --- a/src/qemu/qemu_conf.c
>> +++ b/src/qemu/qemu_conf.c
>> @@ -943,29 +943,55 @@ qemuGetSharedDeviceKey(const char *device_path)
>>       return key;
>>   }
>>   
>> -/* Check if a shared disk's setting conflicts with the conf
>> +/* Check if a shared device's setting conflicts with the conf
>>    * used by other domain(s). Currently only checks the sgio
>>    * setting. Note that this should only be called for disk with
>> - * block source.
>> + * block source if the device type is disk.
>>    *
>>    * Returns 0 if no conflicts, otherwise returns -1.
>>    */
>>   static int
>> -qemuCheckSharedDisk(virHashTablePtr sharedDevices,
>> -                    virDomainDiskDefPtr disk)
>> +qemuCheckSharedDevice(virHashTablePtr sharedDevices,
>> +                      virDomainDeviceDefPtr dev)
>>   {
>> +    virDomainDiskDefPtr disk = NULL;
>> +    virDomainHostdevDefPtr hostdev = NULL;
>>       char *sysfs_path = NULL;
>>       char *key = NULL;
>> +    char *hostdev_name = NULL;
>> +    char *hostdev_path = NULL;
>> +    char *device_path = NULL;
>>       int val;
>>       int ret = 0;
>>   
>> -    /* The only conflicts between shared disk we care about now
>> -     * is sgio setting, which is only valid for device='lun'.
>> -     */
>> -    if (disk->device != VIR_DOMAIN_DISK_DEVICE_LUN)
>> -        return 0;
> Coverity note #1:
>
> (2) Event cond_false: 	Condition "dev->type == VIR_DOMAIN_DEVICE_DISK",
> taking false branch
>
>
>> +    if (dev->type == VIR_DOMAIN_DEVICE_DISK) {
>> +        disk = dev->data.disk;
>> +
>> +        /* The only conflicts between shared disk we care about now
>> +         * is sgio setting, which is only valid for device='lun'.
>> +         */
>> +        if (disk->device != VIR_DOMAIN_DISK_DEVICE_LUN)
>> +            return 0;
>> +
>> +        device_path = disk->src;
> Coverity note #2:
>
> (3) Event cond_false: 	Condition "dev->type ==
> VIR_DOMAIN_DEVICE_HOSTDEV", taking false branch
>
>> +    } else if (dev->type == VIR_DOMAIN_DEVICE_HOSTDEV) {
>> +        hostdev = dev->data.hostdev;
>> +
>> +        if (!(hostdev_name = virSCSIDeviceGetDevName(hostdev->source.subsys.u.scsi.adapter,
>> +                                                     hostdev->source.subsys.u.scsi.bus,
>> +                                                     hostdev->source.subsys.u.scsi.target,
>> +                                                     hostdev->source.subsys.u.scsi.unit)))
>> +            goto cleanup;
>> +
>> +        if (virAsprintf(&hostdev_path, "/dev/%s", hostdev_name) < 0) {
>> +            virReportOOMError();
>> +            goto cleanup;
>> +        }
>> +
>> +        device_path = hostdev_path;
>> +    }
> Coverity Note #3
>
> In the "else" condition (not here) - that means "device_path = NULL"
> which is going to be a problem shortly....
>
> Should we return 0 as an "else" condition?
>
>
>>   
>> -    if (!(sysfs_path = virGetUnprivSGIOSysfsPath(disk->src, NULL))) {
>> +    if (!(sysfs_path = virGetUnprivSGIOSysfsPath(device_path, NULL))) {
>>           ret = -1;
>>           goto cleanup;
>>       }
>> @@ -976,7 +1002,7 @@ qemuCheckSharedDisk(virHashTablePtr sharedDevices,
>>       if (!virFileExists(sysfs_path))
>>           goto cleanup;
>>   
> Coverity complains:
>
> (8) Event var_deref_model: 	Passing null pointer "device_path" to
> function "qemuGetSharedDeviceKey(char const *)", which dereferences it.
> (The dereference is assumed on the basis of the 'nonnull' parameter
> attribute.)
> Also see events: 	[assign_zero]
>
>
> Fix that and it's an ACK
>

To not introduce coverity errors anymore, I setup the coverity
env on my local box, and the errors in this patch are disapeared
after add the else branch:
} else {
     return 0;
}

So pushed. Thanks.

Osier




More information about the libvir-list mailing list