On 02/13/2013 09:57 AM, Osier Yang wrote:
> Based on moving various checking into qemuAddSharedDisk, this
> avoids the caller using it in wrong ways.
> ---
> src/qemu/qemu_conf.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++
> src/qemu/qemu_driver.c | 5 ---
> src/qemu/qemu_process.c | 53 -------------------------------------
> src/qemu/qemu_process.h | 3 --
> 4 files changed, 66 insertions(+), 61 deletions(-)
>
> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
> index d0ee80b..6e735c6 100644
> --- a/src/qemu/qemu_conf.c
> +++ b/src/qemu/qemu_conf.c
> @@ -821,6 +821,69 @@ qemuGetSharedDiskKey(const char *disk_path)
> return key;
> }
>
> +/* Check if a shared disk'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.
> + *
> + * Returns 0 if no conflicts, otherwise returns -1.
> + */
> +static int
> +qemuCheckSharedDisk(virHashTablePtr sharedDisks,
> + virDomainDiskDefPtr disk)
> +{
> + int val;
> + size_t *ref = NULL;
> + char *sysfs_path = NULL;
> + char *key = NULL;
> + int ret = 0;
> +
> + if (!(sysfs_path = virGetUnprivSGIOSysfsPath(disk->src, NULL))) {
> + ret = -1;
> + goto cleanup;
> + }
> +
> + /* It can't be conflict if unpriv_sgio is not supported
> + * by kernel.
> + */
> + if (!virFileExists(sysfs_path))
> + goto cleanup;
> +
> +
> + if (!(key = qemuGetSharedDiskKey(disk->src))) {
> + ret = -1;
> + goto cleanup;
> + }
> +
> + /* It can't be conflict if no other domain is
> + * is sharing it.
> + */
> + if (!(ref = virHashLookup(sharedDisks, key)))
> + goto cleanup;
The code you deleted below checked if (ref == (void)0x1) - so that's not
necessary now? "ref" isn't used as far as I can see (oh and of course
the next patch removes that).
> +
> + if (virGetDeviceUnprivSGIO(disk->src, NULL, &val) < 0) {
> + ret = -1;
> + goto cleanup;
> + }
> +
> + if ((val == 0 &&
> + (disk->sgio == VIR_DOMAIN_DISK_SGIO_FILTERED ||
> + disk->sgio == VIR_DOMAIN_DISK_SGIO_DEFAULT)) ||
> + (val == 1 &&
> + disk->sgio == VIR_DOMAIN_DISK_SGIO_UNFILTERED))
> + goto cleanup;
> +
> + virReportError(VIR_ERR_OPERATION_INVALID,
> + _("sgio of shared disk '%s' conflicts with other "
> + "active domains"), disk->src);
> + ret = -1;
> +
> +cleanup:
> + VIR_FREE(sysfs_path);
> + VIR_FREE(key);
> + return ret;
> +}
> +
> /* Increase ref count if the entry already exists, otherwise
> * add a new entry.
> */
> @@ -840,6 +903,9 @@ qemuAddSharedDisk(virQEMUDriverPtr driver,
> !disk->shared || !disk->src)
> return 0;
>
> + if (qemuCheckSharedDisk(driver->sharedDisks, disk) < 0)
> + return -1;
> +
> if (!(key = qemuGetSharedDiskKey(disk->src)))
> goto cleanup;
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index f98bd9b..86f9674 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -5825,11 +5825,6 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn,
> goto end;
> }
>
> - if (disk->type == VIR_DOMAIN_DISK_TYPE_BLOCK &&
> - disk->shared &&
> - (qemuCheckSharedDisk(driver, disk) < 0))
> - goto end;
> -
There's a lot of work going on in this module that all becomes for
naught if it's not a shared disk. In fact if we fail to add the disk in
this module, I also note that this code path would still call
qemuSetUnprivSGIO() (in the live case, if AddShared) - so if it's not a
sharable disk, then we're setting something we may expect to set, right?
> if (qemuDomainDetermineDiskChain(driver, disk, false) < 0)
> goto end;
>
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 8425cbb..6ffb680 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -3556,56 +3556,6 @@ qemuSetUnprivSGIO(virDomainDiskDefPtr disk)
> return 0;
> }
>
> -/* Check if a shared disk'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.
> - *
> - * Returns 0 if no conflicts, otherwise returns -1.
> - */
> -int
> -qemuCheckSharedDisk(virQEMUDriverPtr driver,
> - virDomainDiskDefPtr disk)
> -{
> - int val;
> - size_t *ref = NULL;
> - char *key = NULL;
> - int ret = 0;
> -
> - if (!(key = qemuGetSharedDiskKey(disk->src)))
> - return -1;
> -
> - /* It can't be conflict if no other domain is
> - * is sharing it.
> - */
> - if (!(ref = virHashLookup(driver->sharedDisks, key)))
> - goto cleanup;
> -
> - if (ref == (void *)0x1)
> - goto cleanup;
> -
> - if (virGetDeviceUnprivSGIO(disk->src, NULL, &val) < 0) {
> - ret = -1;
> - goto cleanup;
> - }
> -
> - if ((val == 0 &&
> - (disk->sgio == VIR_DOMAIN_DISK_SGIO_FILTERED ||
> - disk->sgio == VIR_DOMAIN_DISK_SGIO_DEFAULT)) ||
> - (val == 1 &&
> - disk->sgio == VIR_DOMAIN_DISK_SGIO_UNFILTERED))
> - goto cleanup;
> -
> - virReportError(VIR_ERR_OPERATION_INVALID,
> - _("sgio of shared disk '%s' conflicts with other "
> - "active domains"), disk->src);
> - ret = -1;
> -
> -cleanup:
> - VIR_FREE(key);
> - return ret;
> -}
> -
> int qemuProcessStart(virConnectPtr conn,
> virQEMUDriverPtr driver,
> virDomainObjPtr vm,
> @@ -3962,9 +3912,6 @@ int qemuProcessStart(virConnectPtr conn,
> if (qemuAddSharedDisk(driver, disk) < 0)
> goto cleanup;
>
> - if (qemuCheckSharedDisk(driver, disk) < 0)
> - goto cleanup;
> -
> if (qemuSetUnprivSGIO(disk) < 0)
> goto cleanup;
> }
> diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h
> index cbdab24..7c620d4 100644
> --- a/src/qemu/qemu_process.h
> +++ b/src/qemu/qemu_process.h
> @@ -100,7 +100,4 @@ virBitmapPtr qemuPrepareCpumap(virQEMUDriverPtr driver,
> virBitmapPtr nodemask);
> int qemuSetUnprivSGIO(virDomainDiskDefPtr disk);
>
> -int qemuCheckSharedDisk(virQEMUDriverPtr driver,
> - virDomainDiskDefPtr disk);
> -
> #endif /* __QEMU_PROCESS_H__ */
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list