7:08 p.m.
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
7:08 p.m.
New subject: [libvirt] [PATCH v2 01/10] docs: Clarify unprivileged sgio feature for host devices
Not all kernels support SG_IO for host devices, so let's indicate so
Signed-off-by: John Ferlan <jferlan(a)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
2:19 p.m.
New subject: [libvirt] [PATCH v2 01/10] docs: Clarify unprivileged sgio feature for host devices
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(a)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
2:56 p.m.
New subject: [libvirt] [PATCH v2 01/10] docs: Clarify unprivileged sgio feature for host devices
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(a)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
3:04 p.m.
New subject: [libvirt] [PATCH v2 01/10] docs: Clarify unprivileged sgio feature for host devices
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
7:08 p.m.
New subject: [libvirt] [PATCH v2 02/10] qemu: Introduce qemuIsSharedHostdev
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(a)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
2:28 p.m.
New subject: [libvirt] [PATCH v2 02/10] qemu: Introduce qemuIsSharedHostdev
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(a)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
7:08 p.m.
New subject: [libvirt] [PATCH v2 03/10] qemu: Introduce qemuGetHostdevPath
Introduce a convenience function to handle formulating the hostdev path
Signed-off-by: John Ferlan <jferlan(a)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
3 p.m.
New subject: [libvirt] [PATCH v2 03/10] qemu: Introduce qemuGetHostdevPath
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(a)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
7:08 p.m.
New subject: [libvirt] [PATCH v2 04/10] qemu: Refactor qemuCheckSharedDisk to create virCheckUnprivSGIO
Split out the SGIO check for sharing with hostdev in future patches
Signed-off-by: John Ferlan <jferlan(a)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
2:57 p.m.
New subject: [libvirt] [PATCH v2 04/10] qemu: Refactor qemuCheckSharedDisk to create virCheckUnprivSGIO
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(a)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
7:08 p.m.
New subject: [libvirt] [PATCH v2 05/10] qemu: Refactor qemuAddSharedHostdev and qemuRemoveSharedHostdev
Refactor the functions to follow logic from qemuAddSharedDisk and
qemuRemoveSharedDisk with respect to locking driver.
Signed-off-by: John Ferlan <jferlan(a)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
2:48 p.m.
New subject: [libvirt] [PATCH v2 05/10] qemu: Refactor qemuAddSharedHostdev and qemuRemoveSharedHostdev
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(a)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
7:08 p.m.
New subject: [libvirt] [PATCH v2 06/10] qemu: Extract qemuGetHostdevPath from qemuGetSharedHostdevKey
The device path will be necessary for any future patch to allow sgio
settings on a hostdev
Signed-off-by: John Ferlan <jferlan(a)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
2:46 p.m.
New subject: [libvirt] [PATCH v2 06/10] qemu: Extract qemuGetHostdevPath from qemuGetSharedHostdevKey
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(a)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
7:08 p.m.
New subject: [libvirt] [PATCH v2 07/10] qemu: Refactor qemuSetUnprivSGIO return values
Set to ret = -1 and prove otherwise, like usual
Signed-off-by: John Ferlan <jferlan(a)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
3:11 p.m.
New subject: [libvirt] [PATCH v2 07/10] qemu: Refactor qemuSetUnprivSGIO return values
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(a)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;
}
7:08 p.m.
New subject: [libvirt] [PATCH v2 08/10] qemu: Fix integer/boolean logic in qemuSetUnprivSGIO
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(a)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
3:13 p.m.
New subject: [libvirt] [PATCH v2 08/10] qemu: Fix integer/boolean logic in qemuSetUnprivSGIO
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(a)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
7:08 p.m.
New subject: [libvirt] [PATCH v2 09/10] qemu: Add ability to set sgio values for hostdev
Add necessary checks in order to allow setting sgio values for a scsi
host device
Signed-off-by: John Ferlan <jferlan(a)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
7:08 p.m.
New subject: [libvirt] [PATCH v2 10/10] qemu: Add check for unpriv sgio for SCSI generic host device
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(a)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
2:19 p.m.
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
5:29 p.m.
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
1:56 p.m.
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
3361
days inactive
3363
days old
23 comments
2 participants
participants (2)
-
John Ferlan -
Peter Krempa