[libvirt] [PATCH v2 00/10] Restore code to allow unpriv_sgio for hostdev SCSI generic
v1 here: http://www.redhat.com/archives/libvir-list/2015-June/msg00814.html Changes since v1: - Add doc patch 1 to indicate that this feature may only be supported by certain kernels - Adjust former patch 1 to add call to qemuIsSharedHostdev from qemuSetUnprivSGIO - Insert patches 7 & 8 which essentially refactor qemuSetUnprivSGIO a bit. There should be no functional difference - Patch 9 is now a much slimmer former patch 6 The end result is that 'generically speaking' if any kernel supports setting the unprivileged SGIO feature, then these patches provide the capability to do so. Although as pointed out in the review of v1 only one specific downstream kernel supports the feature, that doesn't mean other distros couldn't add support in the same manner. So rather than just remove all traces from libvirt completely, it seems it would be reasonable to keep the checks in place and if a kernel then decides to add support this code exists to assist. John Ferlan (10): docs: Clarify unprivileged sgio feature for host devices qemu: Introduce qemuIsSharedHostdev qemu: Introduce qemuGetHostdevPath qemu: Refactor qemuCheckSharedDisk to create virCheckUnprivSGIO qemu: Refactor qemuAddSharedHostdev and qemuRemoveSharedHostdev qemu: Extract qemuGetHostdevPath from qemuGetSharedHostdevKey qemu: Refactor qemuSetUnprivSGIO return values qemu: Fix integer/boolean logic in qemuSetUnprivSGIO qemu: Add ability to set sgio values for hostdev qemu: Add check for unpriv sgio for SCSI generic host device docs/formatdomain.html.in | 7 +- src/qemu/qemu_conf.c | 226 ++++++++++++++++++++++++++++++---------------- 2 files changed, 154 insertions(+), 79 deletions(-) -- 2.1.0
Not all kernels support SG_IO for host devices, so let's indicate so Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/formatdomain.html.in | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index ea2fff8..0fc5d85 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -3232,11 +3232,12 @@ </dd> <dt>scsi</dt> <dd>For SCSI devices, user is responsible to make sure the device - is not used by host. The optional <code>sgio</code> + is not used by host. If supported by the kernel, + the optional <code>sgio</code> (<span class="since">since 1.0.6</span>) attribute indicates whether the kernel will filter unprivileged SG_IO commands for - the disk, valid settings are "filtered" or "unfiltered". - The default is "filtered". The optional <code>rawio</code> + the disk. Valid settings are "filtered" or "unfiltered" where + the default is "filtered". The optional <code>rawio</code> (<span class="since">since 1.2.9</span>) attribute indicates whether the lun needs the rawio capability. Valid settings are "yes" or "no". See the rawio description within the -- 2.1.0
On Mon, Jul 06, 2015 at 13:08:29 -0400, John Ferlan wrote:
Not all kernels support SG_IO for host devices, so let's indicate so
Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/formatdomain.html.in | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index ea2fff8..0fc5d85 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -3232,11 +3232,12 @@ </dd> <dt>scsi</dt> <dd>For SCSI devices, user is responsible to make sure the device - is not used by host. The optional <code>sgio</code> + is not used by host. If supported by the kernel,
I'd rather see us state that it has to be supported by the hypervisor, rather than kernel since hostdevs may be supported on non-linux platforms too.
+ the optional <code>sgio</code> (<span class="since">since 1.0.6</span>) attribute indicates whether the kernel will filter unprivileged SG_IO commands for - the disk, valid settings are "filtered" or "unfiltered". - The default is "filtered". The optional <code>rawio</code> + the disk. Valid settings are "filtered" or "unfiltered" where + the default is "filtered". The optional <code>rawio</code> (<span class="since">since 1.2.9</span>) attribute indicates whether the lun needs the rawio capability. Valid settings are "yes" or "no". See the rawio description within thea
But since this is already linux specific I gues I don't care enough to enforce the above comment. ACK, Peter
On 07/08/2015 08:19 AM, Peter Krempa wrote:
On Mon, Jul 06, 2015 at 13:08:29 -0400, John Ferlan wrote:
Not all kernels support SG_IO for host devices, so let's indicate so
Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/formatdomain.html.in | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index ea2fff8..0fc5d85 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -3232,11 +3232,12 @@ </dd> <dt>scsi</dt> <dd>For SCSI devices, user is responsible to make sure the device - is not used by host. The optional <code>sgio</code> + is not used by host. If supported by the kernel,
I'd rather see us state that it has to be supported by the hypervisor, rather than kernel since hostdevs may be supported on non-linux platforms too.
+ the optional <code>sgio</code> (<span class="since">since 1.0.6</span>) attribute indicates whether the kernel will filter unprivileged SG_IO commands for - the disk, valid settings are "filtered" or "unfiltered". - The default is "filtered". The optional <code>rawio</code> + the disk. Valid settings are "filtered" or "unfiltered" where + the default is "filtered". The optional <code>rawio</code> (<span class="since">since 1.2.9</span>) attribute indicates whether the lun needs the rawio capability. Valid settings are "yes" or "no". See the rawio description within thea
But since this is already linux specific I gues I don't care enough to enforce the above comment.
I don't mind kernel or hypervisor, although it seems kernel is more technically correct. How about "hypervisor and OS" like is described for the "shareable" attribute which is related? So, how about the following: If supported by the hypervisor and OS, the optional <code>sgio</code> attribute indicates whether unprivileged SG_IO commands are filtered for the disk. Valid settings are "filtered" or "unfiltered", where the default is "filtered". Additionally, given the rest of the conversation, should the <disk> discussion of 'sgio' also be updated similarly? Currently it states: <dd> Indicates whether the kernel will filter unprivileged SG_IO commands for the disk, valid settings are "filtered" or "unfiltered". Defaults to "filtered". Similar to <code>rawio</code>, <code>sgio</code> is only valid for device 'lun'. </dd> e.g If supported by the hypervisor and OS, indicates whether unprivileged SG_IO commands are filtered for the disk. Valid settings are "filtered" or "unfiltered" where the default is "filtered". Only available when the <code>device</code> is 'lun'. John
On Wed, Jul 08, 2015 at 08:56:52 -0400, John Ferlan wrote: ...
I don't mind kernel or hypervisor, although it seems kernel is more technically correct. How about "hypervisor and OS" like is described for the "shareable" attribute which is related?
So, how about the following:
If supported by the hypervisor and OS, the optional <code>sgio</code> attribute indicates whether unprivileged SG_IO commands are filtered for the disk. Valid settings are "filtered" or "unfiltered", where the default is "filtered".
Additionally, given the rest of the conversation, should the <disk> discussion of 'sgio' also be updated similarly? Currently it states:
<dd> Indicates whether the kernel will filter unprivileged SG_IO commands for the disk, valid settings are "filtered" or "unfiltered". Defaults to "filtered". Similar to <code>rawio</code>, <code>sgio</code> is only valid for device 'lun'. </dd>
e.g
If supported by the hypervisor and OS, indicates whether unprivileged SG_IO commands are filtered for the disk. Valid settings are "filtered" or "unfiltered" where the default is "filtered". Only available when the <code>device</code> is 'lun'.
Both wordings sound okay to me. ACK
John
Add a single boolean function to handle whether the hostdev is shared or not. Use the new function for the qemu{Add|Remove}SharedHostdev calls as well as qemuSetUnprivSGIO. NB: This second usage fixes a possible bug where if this feature is enabled at some time in the future and the shareable flag wasn't set, the sgio would have been erroneously set. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_conf.c | 30 ++++++++++++++++++------------ 1 file changed, 18 insertions(+), 12 deletions(-) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index d521886..48fb74a 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1201,6 +1201,19 @@ qemuAddSharedDisk(virQEMUDriverPtr driver, } +static bool +qemuIsSharedHostdev(virDomainHostdevDefPtr hostdev) +{ + if (hostdev->shareable && + (hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS && + hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI && + hostdev->source.subsys.u.scsi.protocol != + VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI)) + return true; + return false; +} + + static char * qemuGetSharedHostdevKey(virDomainHostdevDefPtr hostdev) { @@ -1238,10 +1251,7 @@ qemuAddSharedHostdev(virQEMUDriverPtr driver, char *key = NULL; int ret = -1; - if (!hostdev->shareable || - !(hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS && - hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI && - hostdev->source.subsys.u.scsi.protocol != VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI)) + if (!qemuIsSharedHostdev(hostdev)) return 0; if (!(key = qemuGetSharedHostdevKey(hostdev))) @@ -1342,10 +1352,7 @@ qemuRemoveSharedHostdev(virQEMUDriverPtr driver, char *key = NULL; int ret; - if (!hostdev->shareable || - !(hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS && - hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI && - hostdev->source.subsys.u.scsi.protocol != VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI)) + if (!qemuIsSharedHostdev(hostdev)) return 0; if (!(key = qemuGetSharedHostdevKey(hostdev))) @@ -1407,11 +1414,10 @@ qemuSetUnprivSGIO(virDomainDeviceDefPtr dev) } else if (dev->type == VIR_DOMAIN_DEVICE_HOSTDEV) { hostdev = dev->data.hostdev; + if (!qemuIsSharedHostdev(hostdev)) + return 0; - if (hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS && - hostdev->source.subsys.type == - VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI && - hostdev->source.subsys.u.scsi.sgio) { + if (hostdev->source.subsys.u.scsi.sgio) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("'sgio' is not supported for SCSI " "generic device yet ")); -- 2.1.0
On Mon, Jul 06, 2015 at 13:08:30 -0400, John Ferlan wrote:
Add a single boolean function to handle whether the hostdev is shared or not.
Use the new function for the qemu{Add|Remove}SharedHostdev calls as well as qemuSetUnprivSGIO. NB: This second usage fixes a possible bug where
s/second/third/
if this feature is enabled at some time in the future and the shareable flag wasn't set, the sgio would have been erroneously set.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_conf.c | 30 ++++++++++++++++++------------ 1 file changed, 18 insertions(+), 12 deletions(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index d521886..48fb74a 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1201,6 +1201,19 @@ qemuAddSharedDisk(virQEMUDriverPtr driver, }
+static bool +qemuIsSharedHostdev(virDomainHostdevDefPtr hostdev) +{ + if (hostdev->shareable && + (hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS && + hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI && + hostdev->source.subsys.u.scsi.protocol != + VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI)) + return true; + return false;
Since the above condition is: if (condition) return true; else return false; You might as well as return the result of the boolean expression directly since it's equivalent.
+} + + static char * qemuGetSharedHostdevKey(virDomainHostdevDefPtr hostdev) { @@ -1238,10 +1251,7 @@ qemuAddSharedHostdev(virQEMUDriverPtr driver, char *key = NULL; int ret = -1;
- if (!hostdev->shareable || - !(hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS && - hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI && - hostdev->source.subsys.u.scsi.protocol != VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI)) + if (!qemuIsSharedHostdev(hostdev)) return 0;
if (!(key = qemuGetSharedHostdevKey(hostdev))) @@ -1342,10 +1352,7 @@ qemuRemoveSharedHostdev(virQEMUDriverPtr driver, char *key = NULL; int ret;
- if (!hostdev->shareable || - !(hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS && - hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI && - hostdev->source.subsys.u.scsi.protocol != VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI)) + if (!qemuIsSharedHostdev(hostdev)) return 0;
if (!(key = qemuGetSharedHostdevKey(hostdev))) @@ -1407,11 +1414,10 @@ qemuSetUnprivSGIO(virDomainDeviceDefPtr dev) } else if (dev->type == VIR_DOMAIN_DEVICE_HOSTDEV) { hostdev = dev->data.hostdev;
+ if (!qemuIsSharedHostdev(hostdev)) + return 0;
- if (hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS && - hostdev->source.subsys.type == - VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI && - hostdev->source.subsys.u.scsi.sgio) { + if (hostdev->source.subsys.u.scsi.sgio) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("'sgio' is not supported for SCSI " "generic device yet "));
ACK, Peter
Introduce a convenience function to handle formulating the hostdev path Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_conf.c | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 48fb74a..f82244f 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1213,15 +1213,13 @@ qemuIsSharedHostdev(virDomainHostdevDefPtr hostdev) return false; } - static char * -qemuGetSharedHostdevKey(virDomainHostdevDefPtr hostdev) +qemuGetHostdevPath(virDomainHostdevDefPtr hostdev) { virDomainHostdevSubsysSCSIPtr scsisrc = &hostdev->source.subsys.u.scsi; virDomainHostdevSubsysSCSIHostPtr scsihostsrc = &scsisrc->u.host; char *dev_name = NULL; char *dev_path = NULL; - char *key = NULL; if (!(dev_name = virSCSIDeviceGetDevName(NULL, scsihostsrc->adapter, @@ -1230,14 +1228,27 @@ qemuGetSharedHostdevKey(virDomainHostdevDefPtr hostdev) scsihostsrc->unit))) goto cleanup; - if (virAsprintf(&dev_path, "/dev/%s", dev_name) < 0) + ignore_value(virAsprintf(&dev_path, "/dev/%s", dev_name)); + + cleanup: + VIR_FREE(dev_name); + return dev_path; +} + + +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_name); VIR_FREE(dev_path); return key; -- 2.1.0
On Mon, Jul 06, 2015 at 13:08:31 -0400, John Ferlan wrote:
Introduce a convenience function to handle formulating the hostdev path
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_conf.c | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 48fb74a..f82244f 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1213,15 +1213,13 @@ qemuIsSharedHostdev(virDomainHostdevDefPtr hostdev) return false; }
-
Here you end up with one line between functions ...
static char * -qemuGetSharedHostdevKey(virDomainHostdevDefPtr hostdev) +qemuGetHostdevPath(virDomainHostdevDefPtr hostdev) { virDomainHostdevSubsysSCSIPtr scsisrc = &hostdev->source.subsys.u.scsi; virDomainHostdevSubsysSCSIHostPtr scsihostsrc = &scsisrc->u.host; char *dev_name = NULL; char *dev_path = NULL; - char *key = NULL;
if (!(dev_name = virSCSIDeviceGetDevName(NULL, scsihostsrc->adapter, @@ -1230,14 +1228,27 @@ qemuGetSharedHostdevKey(virDomainHostdevDefPtr hostdev) scsihostsrc->unit))) goto cleanup;
- if (virAsprintf(&dev_path, "/dev/%s", dev_name) < 0) + ignore_value(virAsprintf(&dev_path, "/dev/%s", dev_name)); + + cleanup: + VIR_FREE(dev_name); + return dev_path; +} + +
... and here you add two.
+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_name); VIR_FREE(dev_path);
return key;
ACK with whitespace consolidated. Peter
Split out the SGIO check for sharing with hostdev in future patches Signed-off-by: John Ferlan <jferlan@redhat.com> --- 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. */ static int -qemuCheckSharedDisk(virHashTablePtr sharedDevices, - virDomainDiskDefPtr disk) +virCheckUnprivSGIO(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 +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) { + 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, -- 2.1.0
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@redhat.com> --- 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
Refactor the functions to follow logic from qemuAddSharedDisk and qemuRemoveSharedDisk with respect to locking driver. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_conf.c | 27 ++++++++++++++++++--------- 1 file changed, 18 insertions(+), 9 deletions(-) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index eb0b34f..8afbddc 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1291,13 +1291,18 @@ qemuAddSharedHostdev(virQEMUDriverPtr driver, if (!qemuIsSharedHostdev(hostdev)) return 0; + qemuDriverLock(driver); + if (!(key = qemuGetSharedHostdevKey(hostdev))) - return -1; + goto cleanup; - qemuDriverLock(driver); - ret = qemuSharedDeviceEntryInsert(driver, key, name); - qemuDriverUnlock(driver); + if (qemuSharedDeviceEntryInsert(driver, key, name) < 0) + goto cleanup; + + ret = 0; + cleanup: + qemuDriverUnlock(driver); VIR_FREE(key); return ret; } @@ -1387,18 +1392,22 @@ qemuRemoveSharedHostdev(virQEMUDriverPtr driver, const char *name) { char *key = NULL; - int ret; + int ret = -1; if (!qemuIsSharedHostdev(hostdev)) return 0; + qemuDriverLock(driver); + if (!(key = qemuGetSharedHostdevKey(hostdev))) - return -1; + goto cleanup; - qemuDriverLock(driver); - ret = qemuSharedDeviceEntryRemove(driver, key, name); - qemuDriverUnlock(driver); + if (qemuSharedDeviceEntryRemove(driver, key, name) < 0) + goto cleanup; + ret = 0; + cleanup: + qemuDriverUnlock(driver); VIR_FREE(key); return ret; } -- 2.1.0
On Mon, Jul 06, 2015 at 13:08:33 -0400, John Ferlan wrote:
Refactor the functions to follow logic from qemuAddSharedDisk and qemuRemoveSharedDisk with respect to locking driver.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_conf.c | 27 ++++++++++++++++++--------- 1 file changed, 18 insertions(+), 9 deletions(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index eb0b34f..8afbddc 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1291,13 +1291,18 @@ qemuAddSharedHostdev(virQEMUDriverPtr driver, if (!qemuIsSharedHostdev(hostdev)) return 0;
+ qemuDriverLock(driver); + if (!(key = qemuGetSharedHostdevKey(hostdev)))
This function doesn't need necessarily be called from the critical section so I don't see a reason to carry this patch. Peter
The device path will be necessary for any future patch to allow sgio settings on a hostdev Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_conf.c | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 8afbddc..80b8926 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1263,20 +1263,14 @@ qemuGetHostdevPath(virDomainHostdevDefPtr hostdev) static char * -qemuGetSharedHostdevKey(virDomainHostdevDefPtr hostdev) +qemuGetSharedHostdevKey(const char *dev_path) { 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; } @@ -1285,6 +1279,7 @@ qemuAddSharedHostdev(virQEMUDriverPtr driver, virDomainHostdevDefPtr hostdev, const char *name) { + char *dev_path = NULL; char *key = NULL; int ret = -1; @@ -1293,7 +1288,10 @@ qemuAddSharedHostdev(virQEMUDriverPtr driver, qemuDriverLock(driver); - if (!(key = qemuGetSharedHostdevKey(hostdev))) + if (!(dev_path = qemuGetHostdevPath(hostdev))) + goto cleanup; + + if (!(key = qemuGetSharedHostdevKey(dev_path))) goto cleanup; if (qemuSharedDeviceEntryInsert(driver, key, name) < 0) @@ -1303,6 +1301,7 @@ qemuAddSharedHostdev(virQEMUDriverPtr driver, cleanup: qemuDriverUnlock(driver); + VIR_FREE(dev_path); VIR_FREE(key); return ret; } @@ -1391,6 +1390,7 @@ qemuRemoveSharedHostdev(virQEMUDriverPtr driver, virDomainHostdevDefPtr hostdev, const char *name) { + char *dev_path = NULL; char *key = NULL; int ret = -1; @@ -1399,7 +1399,10 @@ qemuRemoveSharedHostdev(virQEMUDriverPtr driver, qemuDriverLock(driver); - if (!(key = qemuGetSharedHostdevKey(hostdev))) + if (!(dev_path = qemuGetHostdevPath(hostdev))) + goto cleanup; + + if (!(key = qemuGetSharedHostdevKey(dev_path))) goto cleanup; if (qemuSharedDeviceEntryRemove(driver, key, name) < 0) @@ -1408,6 +1411,7 @@ qemuRemoveSharedHostdev(virQEMUDriverPtr driver, ret = 0; cleanup: qemuDriverUnlock(driver); + VIR_FREE(dev_path); VIR_FREE(key); return ret; } -- 2.1.0
On Mon, Jul 06, 2015 at 13:08:34 -0400, John Ferlan wrote:
The device path will be necessary for any future patch to allow sgio settings on a hostdev
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_conf.c | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 8afbddc..80b8926 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1263,20 +1263,14 @@ qemuGetHostdevPath(virDomainHostdevDefPtr hostdev)
static char * -qemuGetSharedHostdevKey(virDomainHostdevDefPtr hostdev) +qemuGetSharedHostdevKey(const char *dev_path) { 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); -
This function ends up as a no-value-added wrapper on top of qemuGetSharedDeviceKey. The callers might as well as call qemuGetSharedDeviceKey directly rather than this wrapper. Peter
Set to ret = -1 and prove otherwise, like usual Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_conf.c | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 80b8926..5ebf2cc 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1448,7 +1448,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. @@ -1467,24 +1467,19 @@ qemuSetUnprivSGIO(virDomainDeviceDefPtr dev) if (!qemuIsSharedHostdev(hostdev)) return 0; - if (hostdev->source.subsys.u.scsi.sgio) { + if (hostdev->source.subsys.u.scsi.sgio) virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("'sgio' is not supported for SCSI " "generic device yet ")); - ret = -1; goto cleanup; - } return 0; } else { 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); @@ -1495,7 +1490,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 Mon, Jul 06, 2015 at 13:08:35 -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 | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 80b8926..5ebf2cc 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1448,7 +1448,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. @@ -1467,24 +1467,19 @@ qemuSetUnprivSGIO(virDomainDeviceDefPtr dev) if (!qemuIsSharedHostdev(hostdev)) return 0;
- if (hostdev->source.subsys.u.scsi.sgio) { + if (hostdev->source.subsys.u.scsi.sgio) virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("'sgio' is not supported for SCSI " "generic device yet ")); - ret = -1; goto cleanup; - }
Wrong removal of the braces.
return 0; } else { return 0; }
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 5ebf2cc..589a6cf 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1447,7 +1447,7 @@ qemuSetUnprivSGIO(virDomainDeviceDefPtr dev) virDomainHostdevDefPtr hostdev = NULL; char *sysfs_path = NULL; const char *path = NULL; - int val = -1; + bool val = false; int ret = -1; /* "sgio" is only valid for block disk; cdrom @@ -1488,8 +1488,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 Mon, Jul 06, 2015 at 13:08:36 -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 5ebf2cc..589a6cf 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1447,7 +1447,7 @@ qemuSetUnprivSGIO(virDomainDeviceDefPtr dev) virDomainHostdevDefPtr hostdev = NULL; char *sysfs_path = NULL; const char *path = NULL; - int val = -1; + bool val = false;
No need to initialize val here, since it's written first.
int ret = -1;
/* "sgio" is only valid for block disk; cdrom @@ -1488,8 +1488,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;
ACK, Peter
Add necessary checks in order to allow setting sgio values for a scsi host device Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_conf.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 589a6cf..7df971b 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1446,6 +1446,7 @@ qemuSetUnprivSGIO(virDomainDeviceDefPtr dev) virDomainDiskDefPtr disk = NULL; virDomainHostdevDefPtr hostdev = NULL; char *sysfs_path = NULL; + char *hostdev_path = NULL; const char *path = NULL; bool val = false; int ret = -1; @@ -1467,13 +1468,10 @@ qemuSetUnprivSGIO(virDomainDeviceDefPtr dev) if (!qemuIsSharedHostdev(hostdev)) return 0; - if (hostdev->source.subsys.u.scsi.sgio) - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("'sgio' is not supported for SCSI " - "generic device yet ")); + if (!(hostdev_path = qemuGetHostdevPath(hostdev))) goto cleanup; - return 0; + path = hostdev_path; } else { return 0; } @@ -1482,7 +1480,11 @@ qemuSetUnprivSGIO(virDomainDeviceDefPtr dev) goto cleanup; /* By default, filter the SG_IO commands, i.e. set unpriv_sgio to 0. */ - val = (disk->sgio == VIR_DOMAIN_DEVICE_SGIO_UNFILTERED); + if (dev->type == VIR_DOMAIN_DEVICE_DISK) + val = (disk->sgio == VIR_DOMAIN_DEVICE_SGIO_UNFILTERED); + else + val = (hostdev->source.subsys.u.scsi.sgio == + VIR_DOMAIN_DEVICE_SGIO_UNFILTERED); /* Do not do anything if unpriv_sgio is not supported by the kernel and the * whitelist is enabled. But if requesting unfiltered access, always call @@ -1499,6 +1501,7 @@ qemuSetUnprivSGIO(virDomainDeviceDefPtr dev) ret = 0; cleanup: + VIR_FREE(hostdev_path); VIR_FREE(sysfs_path); return ret; } -- 2.1.0
Check if the hostdev has set the sgio filtered/unfiltered and handle appropriately. This restores functionality removed by commit id 'ce346623' to remove sgio support for the SCSI generic host device. For most kernels the result of this operation is a no-op; however, for those that do support it the check is necessary. Note that this patch fixes a bug found in the reverted change where if the virGetDeviceUnprivSGIO returned either 0 or 1 in 'val', the 'disk' would be dereferenced; however, since a hostdev didn't have one - that dereference would have caused a segfault. Instead these changes use the hostdev's address value. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_conf.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 7df971b..7b45a16 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1281,6 +1281,8 @@ qemuAddSharedHostdev(virQEMUDriverPtr driver, { char *dev_path = NULL; char *key = NULL; + virDomainHostdevSubsysSCSIPtr scsisrc = &hostdev->source.subsys.u.scsi; + virDomainHostdevSubsysSCSIHostPtr scsihostsrc = &scsisrc->u.host; int ret = -1; if (!qemuIsSharedHostdev(hostdev)) @@ -1291,6 +1293,18 @@ qemuAddSharedHostdev(virQEMUDriverPtr driver, if (!(dev_path = qemuGetHostdevPath(hostdev))) goto cleanup; + if (virCheckUnprivSGIO(driver->sharedDevices, dev_path, + scsisrc->sgio) < 0) { + if (virGetLastError() == NULL) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("sgio of shared scsi host device '%s-%u-%u-%llu' " + "conflicts with other active domains"), + scsihostsrc->adapter, scsihostsrc->bus, + scsihostsrc->target, scsihostsrc->unit); + } + goto cleanup; + } + if (!(key = qemuGetSharedHostdevKey(dev_path))) goto cleanup; -- 2.1.0
On Mon, Jul 06, 2015 at 13:08:28 -0400, John Ferlan wrote:
v1 here: http://www.redhat.com/archives/libvir-list/2015-June/msg00814.html
Changes since v1: - Add doc patch 1 to indicate that this feature may only be supported by certain kernels - Adjust former patch 1 to add call to qemuIsSharedHostdev from qemuSetUnprivSGIO - Insert patches 7 & 8 which essentially refactor qemuSetUnprivSGIO a bit. There should be no functional difference - Patch 9 is now a much slimmer former patch 6
The end result is that 'generically speaking' if any kernel supports setting the unprivileged SGIO feature, then these patches provide the capability to do so.
Although as pointed out in the review of v1 only one specific downstream kernel supports the feature, that doesn't mean other distros couldn't add support in the same manner. So rather than just remove all traces from libvirt completely, it seems it would be reasonable to keep the checks in place and if a kernel then decides to add support this code exists to assist.
Well, I'm not going to insist that we revert the existing code since it's possible that the feature might actually make it into the upstream linux kernel eventually. Until it's upstream I don't think though we should add support (even if we document that it will not work) for stuff that is not upstream since the design of the upstream interface might then differ, which will make us carry two implementations. Since there's already existing code that touches the kernel interface for unpriv_sgio, actually exposing the support will then require us to carry that part instead of changing it to the actual upstream impl. If a "hypothetical" downstream distro would add kernel patches to add the feature, they might as well as carry the downstream libvirt patches too. NACK series until upstream kernel support is present. (Some of the cleanup patches may be worth taking until the kernel issue gets settled though.) Peter
On 07/07/2015 08:19 AM, Peter Krempa wrote:
On Mon, Jul 06, 2015 at 13:08:28 -0400, John Ferlan wrote:
v1 here: http://www.redhat.com/archives/libvir-list/2015-June/msg00814.html
Changes since v1: - Add doc patch 1 to indicate that this feature may only be supported by certain kernels - Adjust former patch 1 to add call to qemuIsSharedHostdev from qemuSetUnprivSGIO - Insert patches 7 & 8 which essentially refactor qemuSetUnprivSGIO a bit. There should be no functional difference - Patch 9 is now a much slimmer former patch 6
The end result is that 'generically speaking' if any kernel supports setting the unprivileged SGIO feature, then these patches provide the capability to do so.
Although as pointed out in the review of v1 only one specific downstream kernel supports the feature, that doesn't mean other distros couldn't add support in the same manner. So rather than just remove all traces from libvirt completely, it seems it would be reasonable to keep the checks in place and if a kernel then decides to add support this code exists to assist.
Well, I'm not going to insist that we revert the existing code since it's possible that the feature might actually make it into the upstream linux kernel eventually.
Until it's upstream I don't think though we should add support (even if we document that it will not work) for stuff that is not upstream since the design of the upstream interface might then differ, which will make us carry two implementations.
This series merely addresses adding back the hostdev changes using the same processing as <disk> except for where the sgio bit is stored and handled for hostdev. The unpriv_sgio for <disk> is already in libvirt as of 1.0.2 and if I read between the lines correctly, that's not in the upstream kernel either, but I cannot state that for sure. If unpriv_sgio doesn't exist upstream for <disk> and could cause us to carry two implementations if done differently, then I don't see how having hostdev have the same implementation as disk is problematic. Or am I misreading what you wrote? My assumption being that whatever is done upstream would use the same mechanism for disk and hostdev as it does "now" for the downstream implementation.
Since there's already existing code that touches the kernel interface for unpriv_sgio, actually exposing the support will then require us to carry that part instead of changing it to the actual upstream impl.
If a "hypothetical" downstream distro would add kernel patches to add the feature, they might as well as carry the downstream libvirt patches too.
NACK series until upstream kernel support is present. (Some of the cleanup patches may be worth taking until the kernel issue gets settled though.)
Right w/r/t taking some of these patches... This is something I pointed out in the original series, but since this is a new series here's my thoughts... Patch 1 changes to entirely remove the text about sgio in hostdev. Patch 2 seems OK with a slight text adjustment on the commit message Patches 3-8 adjust the shared hostdev logic regardless of unpriv sgio Patches 9 & 10 could then become downstream only, at least for now. Much smaller subset of changes than the original add/revert (and there's I think 2 less bugs from what was there, plus the extra one you found in patch 7 that would have been cleaned up by patch 9 - although I'll contend that's an ordering thing - I did 9 first and then thought that 7 & 8 would things more logical). Since there are ACK's for 2 and 8, I can assume to a degree you agree with my thoughts through patch 8. There's no explicit ACK's on other patches, although there is an explicit NACK to the whole series, so I'll wait before doing anything with this to see your and others thoughts regarding what is or should be applicable. I've already adjusted in my local branch patch 2, 7, & 8. Tks - John
On Tue, Jul 07, 2015 at 11:29:11 -0400, John Ferlan wrote:
On 07/07/2015 08:19 AM, Peter Krempa wrote:
On Mon, Jul 06, 2015 at 13:08:28 -0400, John Ferlan wrote:
v1 here: http://www.redhat.com/archives/libvir-list/2015-June/msg00814.html
Changes since v1: - Add doc patch 1 to indicate that this feature may only be supported by certain kernels - Adjust former patch 1 to add call to qemuIsSharedHostdev from qemuSetUnprivSGIO - Insert patches 7 & 8 which essentially refactor qemuSetUnprivSGIO a bit. There should be no functional difference - Patch 9 is now a much slimmer former patch 6
The end result is that 'generically speaking' if any kernel supports setting the unprivileged SGIO feature, then these patches provide the capability to do so.
Although as pointed out in the review of v1 only one specific downstream kernel supports the feature, that doesn't mean other distros couldn't add support in the same manner. So rather than just remove all traces from libvirt completely, it seems it would be reasonable to keep the checks in place and if a kernel then decides to add support this code exists to assist.
Well, I'm not going to insist that we revert the existing code since it's possible that the feature might actually make it into the upstream linux kernel eventually.
Until it's upstream I don't think though we should add support (even if we document that it will not work) for stuff that is not upstream since the design of the upstream interface might then differ, which will make us carry two implementations.
This series merely addresses adding back the hostdev changes using the same processing as <disk> except for where the sgio bit is stored and handled for hostdev. The unpriv_sgio for <disk> is already in libvirt as of 1.0.2 and if I read between the lines correctly, that's not in the upstream kernel either, but I cannot state that for sure.
Correct, none of the unpriv_sgio stuff is there, not even for <disk>. That's the part Jan was thinking of reverting from upstream. My stance is that we could wait for a while before purging that stuff to see whether the patches possibly make it upstream. If there's no such incentive, we should revert the code to the point where it would not change the behavior but the parts that actually check for the sysfs interface can be removed since the file never existed in upstream kernels.
If unpriv_sgio doesn't exist upstream for <disk> and could cause us to carry two implementations if done differently, then I don't see how having hostdev have the same implementation as disk is problematic. Or am I misreading what you wrote? My assumption being that whatever is done upstream would use the same mechanism for disk and hostdev as it does "now" for the downstream implementation.
Well, if we accept this upstream it will decrease the motivation to get the kernel part merged upstream since it will work for the vendor that carries the downstream kernel patch and others will not care. If we want to make it useful, we should do it right.
Since there's already existing code that touches the kernel interface for unpriv_sgio, actually exposing the support will then require us to carry that part instead of changing it to the actual upstream impl.
If a "hypothetical" downstream distro would add kernel patches to add the feature, they might as well as carry the downstream libvirt patches too.
NACK series until upstream kernel support is present. (Some of the cleanup patches may be worth taking until the kernel issue gets settled though.)
Right w/r/t taking some of these patches... This is something I pointed out in the original series, but since this is a new series here's my thoughts...
Patch 1 changes to entirely remove the text about sgio in hostdev.
Agreed, 1 is mostly fine.
Patch 2 seems OK with a slight text adjustment on the commit message
Patches 3-8 adjust the shared hostdev logic regardless of unpriv sgio
Yes, I'll check them later whether they make sense if 9 and 10 is removed.
Patches 9 & 10 could then become downstream only, at least for now. Much smaller subset of changes than the original add/revert (and there's I think 2 less bugs from what was there, plus the extra one you found in patch 7 that would have been cleaned up by patch 9 - although I'll contend that's an ordering thing - I did 9 first and then thought that 7 & 8 would things more logical).
Making all the stuff upstream, including the kernel change is the preferred way.
Since there are ACK's for 2 and 8, I can assume to a degree you agree with my thoughts through patch 8. There's no explicit ACK's on other patches, although there is an explicit NACK to the whole series, so I'll wait before doing anything with this to see your and others thoughts regarding what is or should be applicable. I've already adjusted in my local branch patch 2, 7, & 8.
Peter
participants (2)
-
John Ferlan -
Peter Krempa