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