9:51 p.m.
https://bugzilla.redhat.com/show_bug.cgi?id=1072736
This series of patches unreverts the functionality from commit id 'ce346623'
which reverted the original functionality.
Since pure revert caused too many conflicts and because the code has
undergone a few changes since the prior reversion, I had to restore
the code by rote method. The reversion includes some refactorings to
make the final check much easier to handle. Of note patch 3 covers much
of what was adjusted in the reverted patch 'qemuCheckSharedDevice'.
Patch 4 expands the driver lock to cover the same space as the
similar disk code - I can take the other as well and shorten the
time the disk code has the lock. Keeping them similar is mostly a
sanity thing.
John Ferlan (7):
qemu: Introduce qemuIsSharedHostdev
qemu: Introduce qemuGetHostdevPath
qemu: Refactor qemuCheckSharedDisk to create virCheckUnprivSGIO
qemu: Refactor qemuAddSharedHostdev and qemuRemoveSharedHostdev
qemu: Extract qemuGetHostdevPath from qemuGetSharedHostdevKey
qemu: Add ability to set sgio values for hostdev
qemu: Add check for unpriv sgio for SCSI generic host device
src/qemu/qemu_conf.c | 205 ++++++++++++++++++++++++++++++++++-----------------
1 file changed, 138 insertions(+), 67 deletions(-)
--
2.1.0
9:51 p.m.
New subject: [libvirt] [PATCH 1/7] qemu: Introduce qemuIsSharedHostdev
Add a single boolean function to handle whether the hostdev is shared or not
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/qemu/qemu_conf.c | 23 +++++++++++++++--------
1 file changed, 15 insertions(+), 8 deletions(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 16ae6ab..8e9da0d 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -1196,6 +1196,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)
{
@@ -1233,10 +1246,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)))
@@ -1337,10 +1347,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)))
--
2.1.0
4:41 p.m.
New subject: [libvirt] [PATCH 1/7] qemu: Introduce qemuIsSharedHostdev
On 06/16/2015 03:51 PM, John Ferlan wrote:
Add a single boolean function to handle whether the hostdev is shared
or not
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/qemu/qemu_conf.c | 23 +++++++++++++++--------
1 file changed, 15 insertions(+), 8 deletions(-)
Given Jan's comments about the overall series, I'd like to propose
squashing in the attached patch into this patch. Essentially it's
making the same check and returning 0 if the shared bit wasn't set. The
current code checks for shared hostdev *and* sgio is set, then fail;
otherwise, return 0. This patch will check shared hostdev, return 0 if
not.. then check sgio singularly and fail; otherwise return 0.
John
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 16ae6ab..8e9da0d 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -1196,6 +1196,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)
{
@@ -1233,10 +1246,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)))
@@ -1337,10 +1347,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)))
9:51 p.m.
New subject: [libvirt] [PATCH 2/7] 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 8e9da0d..fd0ad3f 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -1208,15 +1208,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,
@@ -1225,14 +1223,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
9:51 p.m.
New subject: [libvirt] [PATCH 3/7] 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 fd0ad3f..0314707 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -1013,26 +1013,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. */
@@ -1041,7 +1036,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. */
@@ -1050,29 +1045,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;
@@ -1083,6 +1068,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
9:51 p.m.
New subject: [libvirt] [PATCH 4/7] 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 0314707..faa0b49 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -1286,13 +1286,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;
}
@@ -1382,18 +1387,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
9:51 p.m.
New subject: [libvirt] [PATCH 5/7] qemu: Extract qemuGetHostdevPath from qemuGetSharedHostdevKey
The device path will be necessary for upcoming 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 faa0b49..797e5f1 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -1258,20 +1258,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;
}
@@ -1280,6 +1274,7 @@ qemuAddSharedHostdev(virQEMUDriverPtr driver,
virDomainHostdevDefPtr hostdev,
const char *name)
{
+ char *dev_path = NULL;
char *key = NULL;
int ret = -1;
@@ -1288,7 +1283,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)
@@ -1298,6 +1296,7 @@ qemuAddSharedHostdev(virQEMUDriverPtr driver,
cleanup:
qemuDriverUnlock(driver);
+ VIR_FREE(dev_path);
VIR_FREE(key);
return ret;
}
@@ -1386,6 +1385,7 @@ qemuRemoveSharedHostdev(virQEMUDriverPtr driver,
virDomainHostdevDefPtr hostdev,
const char *name)
{
+ char *dev_path = NULL;
char *key = NULL;
int ret = -1;
@@ -1394,7 +1394,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)
@@ -1403,6 +1406,7 @@ qemuRemoveSharedHostdev(virQEMUDriverPtr driver,
ret = 0;
cleanup:
qemuDriverUnlock(driver);
+ VIR_FREE(dev_path);
VIR_FREE(key);
return ret;
}
--
2.1.0
9:51 p.m.
New subject: [libvirt] [PATCH 6/7] 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 | 22 +++++++++++-----------
1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 797e5f1..14ea4c8 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -1441,6 +1441,7 @@ qemuSetUnprivSGIO(virDomainDeviceDefPtr dev)
virDomainDiskDefPtr disk = NULL;
virDomainHostdevDefPtr hostdev = NULL;
char *sysfs_path = NULL;
+ char *hostdev_path = NULL;
const char *path = NULL;
int val = -1;
int ret = 0;
@@ -1459,19 +1460,13 @@ 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) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("'sgio' is not supported for SCSI "
- "generic device yet "));
- ret = -1;
+ if (!(hostdev_path = qemuGetHostdevPath(hostdev)))
goto cleanup;
- }
- return 0;
+ path = hostdev_path;
} else {
return 0;
}
@@ -1483,7 +1478,11 @@ qemuSetUnprivSGIO(virDomainDeviceDefPtr dev)
}
/* 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
@@ -1494,6 +1493,7 @@ qemuSetUnprivSGIO(virDomainDeviceDefPtr dev)
ret = -1;
cleanup:
+ VIR_FREE(hostdev_path);
VIR_FREE(sysfs_path);
return ret;
}
--
2.1.0
9:51 p.m.
New subject: [libvirt] [PATCH 7/7] qemu: Add check for unpriv sgio for SCSI generic host device
https://bugzilla.redhat.com/show_bug.cgi?id=1072736
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 the SCSI generic host device. Note that this patch fixes
a bug 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 sgio 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 14ea4c8..485f1cc 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -1276,6 +1276,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))
@@ -1286,6 +1288,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-%d-%d-%d'
"
+ "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:24 p.m.
On Tue, Jun 16, 2015 at 03:51:23PM -0400, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1072736
This series of patches unreverts the functionality from commit id 'ce346623'
which reverted the original functionality.
What is the motivation to revert the revert?
The commit message of that commit says:
The kernel didn't support the unprivileged SGIO for SCSI generic
device finally, and since it's unknow whether the way to support
unprivileged SGIO for SCSI generic device will be similar as for
SCSI block device or not, even it's simliar (I.e. via sysfs, for
SCSI block device, it's /sys/dev/block/8\:0/queue/unpriv_sgio,
for example), the file name might be different, So it's better not
guess what it should be like currently.
But 'git grep unpriv_sgio' gives me no results on linux-next,
I only see it in the source of the dowstream kernel of a certain
enterprise distro.
If unpriv_sgio is RHEL-only, maybe we should revert it from upstream
libvirt completely.
Jan
Since pure revert caused too many conflicts and because the code has
undergone a few changes since the prior reversion, I had to restore
the code by rote method. The reversion includes some refactorings to
make the final check much easier to handle. Of note patch 3 covers much
of what was adjusted in the reverted patch 'qemuCheckSharedDevice'.
Patch 4 expands the driver lock to cover the same space as the
similar disk code - I can take the other as well and shorten the
time the disk code has the lock. Keeping them similar is mostly a
sanity thing.
4:16 p.m.
On 06/24/2015 08:24 AM, Ján Tomko wrote:
On Tue, Jun 16, 2015 at 03:51:23PM -0400, John Ferlan wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1072736
>
> This series of patches unreverts the functionality from commit id 'ce346623'
> which reverted the original functionality.
>
What is the motivation to revert the revert?
The commit message of that commit says:
The kernel didn't support the unprivileged SGIO for SCSI generic
device finally, and since it's unknow whether the way to support
unprivileged SGIO for SCSI generic device will be similar as for
SCSI block device or not, even it's simliar (I.e. via sysfs, for
SCSI block device, it's /sys/dev/block/8\:0/queue/unpriv_sgio,
for example), the file name might be different, So it's better not
guess what it should be like currently.
But 'git grep unpriv_sgio' gives me no results on linux-next,
I only see it in the source of the dowstream kernel of a certain
enterprise distro.
If unpriv_sgio is RHEL-only, maybe we should revert it from upstream
libvirt completely.
Jan
I guess it wasn't perfectly clear from the original change that
unpriv_sgio was only for one downstream implementation and only for
shared hostdev (generic scsi), but now that I read the related cases
with this in mind, I see your point.
I suppose there could be reason to carry it only for that downstream
source pool, although I think the overhead of doing that could be made
easier by accepting these patches. At the very least a couple of the
patches are not necessarily sgio hostdev related. Perhaps only patch 6
would need to be reworked to remove the "hostdev_path" and patch 7 could
be removed.
It seems from my reading that sgio is OK for disk since that's not the
contention here, correct? Or does that need to be removed as well?
John
> Since pure revert caused too many conflicts and because the code
has
> undergone a few changes since the prior reversion, I had to restore
> the code by rote method. The reversion includes some refactorings to
> make the final check much easier to handle. Of note patch 3 covers much
> of what was adjusted in the reverted patch 'qemuCheckSharedDevice'.
> Patch 4 expands the driver lock to cover the same space as the
> similar disk code - I can take the other as well and shorten the
> time the disk code has the lock. Keeping them similar is mostly a
> sanity thing.
>
8:15 p.m.
New subject: [libvirt] [PATCH 0.5/7] docs: Remove sgio from hostdev docs
Since hostdev sgio is not supported, let's remove it from the docs
to remove confusion.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
Hopefully I got the in-reply-to magic correct! I figured keeping the
sgio buried in the scsisrc is "ok" for those down stream providers
that want or have the ability to support it. I could remove it as well
if that's desired.
docs/formatdomain.html.in | 23 +----------------------
1 file changed, 1 insertion(+), 22 deletions(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index f7b9f51..59b4e86 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -3171,23 +3171,6 @@
<pre>
...
<devices>
- <hostdev mode='subsystem' type='scsi' sgio='filtered'
rawio='yes'>
- <source>
- <adapter name='scsi_host0'/>
- <address bus='0' target='0' unit='0'/>
- </source>
- <readonly/>
- <address type='drive' controller='0' bus='0'
target='0' unit='0'/>
- </hostdev>
- </devices>
- ...</pre>
-
-
- <p>or:</p>
-
-<pre>
- ...
- <devices>
<hostdev mode='subsystem' type='scsi'>
<source protocol='iscsi'
name='iqn.2014-08.com.example:iscsi-nopool/1'>
<host name='example.com' port='3260'/>
@@ -3225,11 +3208,7 @@
</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>
- (<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>
+ is not used by host. 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:38 p.m.
On 06/16/2015 03:51 PM, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1072736
This series of patches unreverts the functionality from commit id 'ce346623'
which reverted the original functionality.
Since pure revert caused too many conflicts and because the code has
undergone a few changes since the prior reversion, I had to restore
the code by rote method. The reversion includes some refactorings to
make the final check much easier to handle. Of note patch 3 covers much
of what was adjusted in the reverted patch 'qemuCheckSharedDevice'.
Patch 4 expands the driver lock to cover the same space as the
similar disk code - I can take the other as well and shorten the
time the disk code has the lock. Keeping them similar is mostly a
sanity thing.
John Ferlan (7):
qemu: Introduce qemuIsSharedHostdev
qemu: Introduce qemuGetHostdevPath
qemu: Refactor qemuCheckSharedDisk to create virCheckUnprivSGIO
qemu: Refactor qemuAddSharedHostdev and qemuRemoveSharedHostdev
qemu: Extract qemuGetHostdevPath from qemuGetSharedHostdevKey
qemu: Add ability to set sgio values for hostdev
qemu: Add check for unpriv sgio for SCSI generic host device
src/qemu/qemu_conf.c | 205 ++++++++++++++++++++++++++++++++++-----------------
1 file changed, 138 insertions(+), 67 deletions(-)
ping... Any thoughts/comments on patches 0.5, 1->5 being "OK" for this
upstream release - including squashing part of patch 6 into 1 (thus
making patches 6 & 7 downstream only)
Tks,
John
3369
days inactive
3383
days old
12 comments
2 participants
participants (2)
-
John Ferlan -
Ján Tomko