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

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