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
John
> - if (!(key = qemuGetSharedDeviceKey(disk->src))) {
> + if (!(key = qemuGetSharedDeviceKey(device_path))) {
> ret = -1;
> goto cleanup;
> }
> @@ -987,7 +1013,7 @@ qemuCheckSharedDisk(virHashTablePtr sharedDevices,
> if (!(virHashLookup(sharedDevices, key)))
> goto cleanup;
>
> - if (virGetDeviceUnprivSGIO(disk->src, NULL, &val) < 0) {
> + if (virGetDeviceUnprivSGIO(device_path, NULL, &val) < 0) {
> ret = -1;
> goto cleanup;
> }
> @@ -999,26 +1025,36 @@ qemuCheckSharedDisk(virHashTablePtr sharedDevices,
> disk->sgio == VIR_DOMAIN_DEVICE_SGIO_UNFILTERED))
> goto cleanup;
>
> - if (disk->type == VIR_DOMAIN_DISK_TYPE_VOLUME) {
> - virReportError(VIR_ERR_OPERATION_INVALID,
> - _("sgio of shared disk 'pool=%s' 'volume=%s' conflicts "
> - "with other active domains"),
> - disk->srcpool->pool,
> - disk->srcpool->volume);
> + if (dev->type == VIR_DOMAIN_DEVICE_DISK) {
> + if (disk->type == VIR_DOMAIN_DISK_TYPE_VOLUME) {
> + virReportError(VIR_ERR_OPERATION_INVALID,
> + _("sgio of shared disk 'pool=%s' 'volume=%s' conflicts "
> + "with other active domains"),
> + disk->srcpool->pool,
> + disk->srcpool->volume);
> + } else {
> + virReportError(VIR_ERR_OPERATION_INVALID,
> + _("sgio of shared disk '%s' conflicts with other "
> + "active domains"), disk->src);
> + }
> } else {
> virReportError(VIR_ERR_OPERATION_INVALID,
> - _("sgio of shared disk '%s' conflicts with other "
> - "active domains"), disk->src);
> + _("sgio of shared scsi host device '%s-%d-%d-%d' conflicts "
> + "with other active domains"),
> + hostdev->source.subsys.u.scsi.adapter,
> + hostdev->source.subsys.u.scsi.bus,
> + hostdev->source.subsys.u.scsi.target,
> + hostdev->source.subsys.u.scsi.unit);
> }
>
> ret = -1;
> -
> cleanup:
> + VIR_FREE(hostdev_name);
> + VIR_FREE(hostdev_path);
> VIR_FREE(sysfs_path);
> VIR_FREE(key);
> return ret;
> }
> -
> bool
> qemuSharedDeviceEntryDomainExists(qemuSharedDeviceEntryPtr entry,
> const char *name,
> @@ -1133,10 +1169,10 @@ qemuAddSharedDevice(virQEMUDriverPtr driver,
> }
>
> qemuDriverLock(driver);
> - if (dev->type == VIR_DOMAIN_DEVICE_DISK) {
> - if (qemuCheckSharedDisk(driver->sharedDevices, disk) < 0)
> - goto cleanup;
> + if (qemuCheckSharedDevice(driver->sharedDevices, dev) < 0)
> + goto cleanup;
>
> + if (dev->type == VIR_DOMAIN_DEVICE_DISK) {
> if (!(key = qemuGetSharedDeviceKey(disk->src)))
> goto cleanup;
> } else {
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list