On Mon, Jul 06, 2015 at 13:08:32 -0400, John Ferlan wrote:
> Split out the SGIO check for sharing with hostdev in future patches
>
> Signed-off-by: John Ferlan <jferlan@xxxxxxxxxx>
> ---
> src/qemu/qemu_conf.c | 88 ++++++++++++++++++++++++++++++++++------------------
> 1 file changed, 57 insertions(+), 31 deletions(-)
>
> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
> index f82244f..eb0b34f 100644
> --- a/src/qemu/qemu_conf.c
> +++ b/src/qemu/qemu_conf.c
> @@ -1018,26 +1018,21 @@ qemuGetSharedDeviceKey(const char *device_path)
> return key;
> }
>
> -/* 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 if the device type is disk.
> - *
> - * Returns 0 if no conflicts, otherwise returns -1.
> +/*
> + * Make necessary checks for the need to check and for the current setting
> + * of the 'unpriv_sgio' value for the device_path passed.
This comment will need to be greatly improved due to the very magic
handling of errors ... see below.
> */
> static int
> -qemuCheckSharedDisk(virHashTablePtr sharedDevices,
> - virDomainDiskDefPtr disk)
> +virCheckUnprivSGIO(virHashTablePtr sharedDevices,
This is a qemu specific function so we should keep 'qemu' in the name.
> + const char *device_path,
> + int sgio)
> {
> char *sysfs_path = NULL;
> char *key = NULL;
> int val;
> int ret = -1;
>
> - if (disk->device != VIR_DOMAIN_DISK_DEVICE_LUN)
> - return 0;
> -
> - if (!(sysfs_path = virGetUnprivSGIOSysfsPath(disk->src->path, NULL)))
> + if (!(sysfs_path = virGetUnprivSGIOSysfsPath(device_path, NULL)))
> goto cleanup;
>
> /* It can't be conflict if unpriv_sgio is not supported by kernel. */
> @@ -1046,7 +1041,7 @@ qemuCheckSharedDisk(virHashTablePtr sharedDevices,
> goto cleanup;
> }
>
> - if (!(key = qemuGetSharedDeviceKey(disk->src->path)))
> + if (!(key = qemuGetSharedDeviceKey(device_path)))
> goto cleanup;
>
> /* It can't be conflict if no other domain is sharing it. */
> @@ -1055,29 +1050,19 @@ qemuCheckSharedDisk(virHashTablePtr sharedDevices,
> goto cleanup;
> }
>
> - if (virGetDeviceUnprivSGIO(disk->src->path, NULL, &val) < 0)
> + if (virGetDeviceUnprivSGIO(device_path, NULL, &val) < 0)
> goto cleanup;
>
> + /* Error message on failure needs to be handled in caller
> + * since there is more specific knowledge of device
> + */
> + virResetLastError();
> if (!((val == 0 &&
> - (disk->sgio == VIR_DOMAIN_DEVICE_SGIO_FILTERED ||
> - disk->sgio == VIR_DOMAIN_DEVICE_SGIO_DEFAULT)) ||
> + (sgio == VIR_DOMAIN_DEVICE_SGIO_FILTERED ||
> + sgio == VIR_DOMAIN_DEVICE_SGIO_DEFAULT)) ||
> (val == 1 &&
> - disk->sgio == VIR_DOMAIN_DEVICE_SGIO_UNFILTERED))) {
> -
> - if (virDomainDiskGetType(disk) == VIR_STORAGE_TYPE_VOLUME) {
> - virReportError(VIR_ERR_OPERATION_INVALID,
> - _("sgio of shared disk 'pool=%s' 'volume=%s' conflicts "
> - "with other active domains"),
> - disk->src->srcpool->pool,
> - disk->src->srcpool->volume);
> - } else {
> - virReportError(VIR_ERR_OPERATION_INVALID,
> - _("sgio of shared disk '%s' conflicts with other "
> - "active domains"), disk->src->path);
> - }
> -
> + sgio == VIR_DOMAIN_DEVICE_SGIO_UNFILTERED)))
> goto cleanup;
> - }
>
> ret = 0;
>
> @@ -1088,6 +1073,47 @@ qemuCheckSharedDisk(virHashTablePtr sharedDevices,
> }
>
>
> +/* 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 if the device type is disk.
> + *
> + * Returns 0 if no conflicts, otherwise returns -1.
> + */
> +static int
> +qemuCheckSharedDisk(virHashTablePtr sharedDevices,
> + virDomainDiskDefPtr disk)
> +{
> + int ret = -1;
> +
> + if (disk->device != VIR_DOMAIN_DISK_DEVICE_LUN)
> + return 0;
> +
> + if (virCheckUnprivSGIO(sharedDevices, disk->src->path, disk->sgio) < 0) {
> + if (virGetLastError() == NULL) {
This use case is extremely unusual. Our functions either report errors
or not. Having something like this will require very thorough
documentation of the behavior. Additionally, since virCheckUnprivSGIO
returns an integer that is either -1 or 0, and additionally reports the
state via presence or missing of an error message you migh as well as
return 3 distinct values and decide in the caller via that rather than
checking the error.
> + if (virDomainDiskGetType(disk) == VIR_STORAGE_TYPE_VOLUME) {
> + virReportError(VIR_ERR_OPERATION_INVALID,
> + _("sgio of shared disk 'pool=%s' 'volume=%s' "
> + "conflicts with other active domains"),
> + disk->src->srcpool->pool,
> + disk->src->srcpool->volume);
> + } else {
> + virReportError(VIR_ERR_OPERATION_INVALID,
> + _("sgio of shared disk '%s' conflicts with "
> + "other active domains"),
> + disk->src->path);
> + }
> + }
> + goto cleanup;
> + }
> +
> + ret = 0;
> +
> + cleanup:
> + return ret;
> +}
> +
> +
> bool
> qemuSharedDeviceEntryDomainExists(qemuSharedDeviceEntryPtr entry,
> const char *name,
Peter
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list