[libvirt] [PATCH v3 0/4] Followup for unpriv_sgio changes
v2: http://www.redhat.com/archives/libvir-list/2015-July/msg00204.html I pushed the first 3 patches already since they were ACK'd This is patches 4-8 adjusted for code review comments Patch1 (former patch4) - change the function to use qemuCheckUnprivSGIO not qemuCheckUnprivSGIO. Also used a -1 or -2 return value for handling the mismatched sgio setting error which requires the caller to have a message including the device with the issue Former patch 5 abandoned Patch2 (former patch6) - completely removed qemuGetSharedHostdevKey and replaced with inline code. Although this looks odd now, a future patch will use the device name on the add side. Patch3 (former patch7) - restored the {} in the right place Patch4 (former patch8) - remove val initialization - already ACK'd, but easier for me to just keep in the current order. Former patches 9 & 10 - keeping separate for downstream John Ferlan (4): qemu: Refactor qemuCheckSharedDisk to create qemuCheckUnprivSGIO qemu: Inline qemuGetHostdevPath qemu: Refactor qemuSetUnprivSGIO return values qemu: Fix integer/boolean logic in qemuSetUnprivSGIO src/qemu/qemu_conf.c | 151 ++++++++++++++++++++++++++++++--------------------- 1 file changed, 89 insertions(+), 62 deletions(-) -- 2.1.0
Split out the current function in order to share the code with hostdev in a future patch. Failure to match the expected sgio value against what is stored will cause an error which the caller would need to handle since only the caller has the disk (or eventually hostdev) specific data in order to uniquely identify the disk in an error message. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_conf.c | 90 +++++++++++++++++++++++++++++++++++----------------- 1 file changed, 61 insertions(+), 29 deletions(-) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 391fb2c..7506895 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1018,26 +1018,28 @@ 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. +/* + * Make necessary checks for the need to check and for the current setting + * of the 'unpriv_sgio' value for the device_path passed. * - * Returns 0 if no conflicts, otherwise returns -1. + * Returns: + * 0 - Success + * -1 - Some failure which would already have been messaged + * -2 - Mismatch with the "shared" sgio setting - needs to be messaged + * by caller since it has context of which type of disk resource is + * being used and in the future the hostdev information. */ static int -qemuCheckSharedDisk(virHashTablePtr sharedDevices, - virDomainDiskDefPtr disk) +qemuCheckUnprivSGIO(virHashTablePtr sharedDevices, + 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 +1048,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,27 +1057,18 @@ 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 + */ 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))) { + ret = -2; goto cleanup; } @@ -1088,6 +1081,45 @@ 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; + + if (disk->device != VIR_DOMAIN_DISK_DEVICE_LUN) + return 0; + + if ((ret = qemuCheckUnprivSGIO(sharedDevices, disk->src->path, + disk->sgio)) < 0) { + if (ret == -2) { + 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); + } + } + return -1; + } + + return 0; +} + + bool qemuSharedDeviceEntryDomainExists(qemuSharedDeviceEntryPtr entry, const char *name, -- 2.1.0
On Wed, Jul 08, 2015 at 15:30:25 -0400, John Ferlan wrote:
Split out the current function in order to share the code with hostdev in a future patch. Failure to match the expected sgio value against what is stored will cause an error which the caller would need to handle since only the caller has the disk (or eventually hostdev) specific data in order to uniquely identify the disk in an error message.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_conf.c | 90 +++++++++++++++++++++++++++++++++++----------------- 1 file changed, 61 insertions(+), 29 deletions(-)
ACK, but this patch seems kind of useless without the followup patches that were dropped due to lack of upstream support. Peter
Since a future patch will need the device path generated when adding a shared host device, remove the qemuAddSharedHostdev and inline the two calls into qemuAddSharedHostdev and qemuRemoveSharedHostdev Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_conf.c | 39 ++++++++++++++++----------------------- 1 file changed, 16 insertions(+), 23 deletions(-) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 7506895..3fa00f0 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1267,43 +1267,30 @@ qemuGetHostdevPath(virDomainHostdevDefPtr hostdev) } -static char * -qemuGetSharedHostdevKey(virDomainHostdevDefPtr hostdev) -{ - char *key = NULL; - char *dev_path = NULL; - - if (!(dev_path = qemuGetHostdevPath(hostdev))) - goto cleanup; - - if (!(key = qemuGetSharedDeviceKey(dev_path))) - goto cleanup; - - cleanup: - VIR_FREE(dev_path); - - return key; -} - - static int qemuAddSharedHostdev(virQEMUDriverPtr driver, virDomainHostdevDefPtr hostdev, const char *name) { + char *dev_path = NULL; char *key = NULL; int ret = -1; if (!qemuIsSharedHostdev(hostdev)) return 0; - if (!(key = qemuGetSharedHostdevKey(hostdev))) - return -1; + if (!(dev_path = qemuGetHostdevPath(hostdev))) + goto cleanup; + + if (!(key = qemuGetSharedDeviceKey(dev_path))) + goto cleanup; qemuDriverLock(driver); ret = qemuSharedDeviceEntryInsert(driver, key, name); qemuDriverUnlock(driver); + cleanup: + VIR_FREE(dev_path); VIR_FREE(key); return ret; } @@ -1392,19 +1379,25 @@ qemuRemoveSharedHostdev(virQEMUDriverPtr driver, virDomainHostdevDefPtr hostdev, const char *name) { + char *dev_path = NULL; char *key = NULL; int ret; if (!qemuIsSharedHostdev(hostdev)) return 0; - if (!(key = qemuGetSharedHostdevKey(hostdev))) - return -1; + if (!(dev_path = qemuGetHostdevPath(hostdev))) + goto cleanup; + + if (!(key = qemuGetSharedDeviceKey(dev_path))) + goto cleanup; qemuDriverLock(driver); ret = qemuSharedDeviceEntryRemove(driver, key, name); qemuDriverUnlock(driver); + cleanup: + VIR_FREE(dev_path); VIR_FREE(key); return ret; } -- 2.1.0
On Wed, Jul 08, 2015 at 15:30:26 -0400, John Ferlan wrote:
Since a future patch will need the device path generated when adding a shared host device, remove the qemuAddSharedHostdev and inline the two calls into qemuAddSharedHostdev and qemuRemoveSharedHostdev
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_conf.c | 39 ++++++++++++++++----------------------- 1 file changed, 16 insertions(+), 23 deletions(-)
ACK
Set to ret = -1 and prove otherwise, like usual Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_conf.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 3fa00f0..ddaf7f8 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1434,7 +1434,7 @@ qemuSetUnprivSGIO(virDomainDeviceDefPtr dev) char *sysfs_path = NULL; const char *path = NULL; int val = -1; - int ret = 0; + int ret = -1; /* "sgio" is only valid for block disk; cdrom * and floopy disk can have empty source. @@ -1457,7 +1457,6 @@ qemuSetUnprivSGIO(virDomainDeviceDefPtr dev) virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("'sgio' is not supported for SCSI " "generic device yet ")); - ret = -1; goto cleanup; } @@ -1466,11 +1465,8 @@ qemuSetUnprivSGIO(virDomainDeviceDefPtr dev) return 0; } - sysfs_path = virGetUnprivSGIOSysfsPath(path, NULL); - if (sysfs_path == NULL) { - ret = -1; + if (!(sysfs_path = virGetUnprivSGIOSysfsPath(path, NULL))) goto cleanup; - } /* By default, filter the SG_IO commands, i.e. set unpriv_sgio to 0. */ val = (disk->sgio == VIR_DOMAIN_DEVICE_SGIO_UNFILTERED); @@ -1481,7 +1477,9 @@ qemuSetUnprivSGIO(virDomainDeviceDefPtr dev) */ if ((virFileExists(sysfs_path) || val == 1) && virSetDeviceUnprivSGIO(path, NULL, val) < 0) - ret = -1; + goto cleanup; + + ret = 0; cleanup: VIR_FREE(sysfs_path); -- 2.1.0
On Wed, Jul 08, 2015 at 15:30:27 -0400, John Ferlan wrote:
Set to ret = -1 and prove otherwise, like usual
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_conf.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-)
ACK
Setting of 'val' is a boolean expression, so handle it that way and adjust the check/return logic to be clearer Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_conf.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index ddaf7f8..2ab5494 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1433,7 +1433,7 @@ qemuSetUnprivSGIO(virDomainDeviceDefPtr dev) virDomainHostdevDefPtr hostdev = NULL; char *sysfs_path = NULL; const char *path = NULL; - int val = -1; + bool val; int ret = -1; /* "sgio" is only valid for block disk; cdrom @@ -1475,8 +1475,12 @@ qemuSetUnprivSGIO(virDomainDeviceDefPtr dev) * whitelist is enabled. But if requesting unfiltered access, always call * virSetDeviceUnprivSGIO, to report an error for unsupported unpriv_sgio. */ - if ((virFileExists(sysfs_path) || val == 1) && - virSetDeviceUnprivSGIO(path, NULL, val) < 0) + if (!val || !virFileExists(sysfs_path)) { + ret = 0; + goto cleanup; + } + + if (virSetDeviceUnprivSGIO(path, NULL, 1) < 0) goto cleanup; ret = 0; -- 2.1.0
On Wed, Jul 08, 2015 at 15:30:28 -0400, John Ferlan wrote:
Setting of 'val' is a boolean expression, so handle it that way and adjust the check/return logic to be clearer
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_conf.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index ddaf7f8..2ab5494 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1433,7 +1433,7 @@ qemuSetUnprivSGIO(virDomainDeviceDefPtr dev) virDomainHostdevDefPtr hostdev = NULL; char *sysfs_path = NULL; const char *path = NULL; - int val = -1; + bool val; int ret = -1;
/* "sgio" is only valid for block disk; cdrom @@ -1475,8 +1475,12 @@ qemuSetUnprivSGIO(virDomainDeviceDefPtr dev) * whitelist is enabled. But if requesting unfiltered access, always call * virSetDeviceUnprivSGIO, to report an error for unsupported unpriv_sgio. */ - if ((virFileExists(sysfs_path) || val == 1) && - virSetDeviceUnprivSGIO(path, NULL, val) < 0) + if (!val || !virFileExists(sysfs_path)) {
With this control flow the @val variable can be entirely avoided since it's just set once and read once.
+ ret = 0; + goto cleanup; + } + + if (virSetDeviceUnprivSGIO(path, NULL, 1) < 0) goto cleanup;
ret = 0;
ACK regardles if you choose to optimize @val out. Peter
participants (2)
-
John Ferlan -
Peter Krempa