8:51 a.m.
*** BLURB HERE ***
Osier Yang (2):
util: Add "shareable" field for virSCSIDevice struct
qemu: Don't fail if the SCSI host device is shareable between domains
src/libvirt_private.syms | 3 +-
src/qemu/qemu_cgroup.c | 3 +-
src/qemu/qemu_hostdev.c | 84 ++++++++++++++++++++++------------------
src/security/security_apparmor.c | 3 +-
src/security/security_dac.c | 6 ++-
src/security/security_selinux.c | 6 ++-
src/util/virscsi.c | 59 +++++++++++++++++++++++-----
src/util/virscsi.h | 11 ++++--
8 files changed, 116 insertions(+), 59 deletions(-)
--
1.8.1.4
8:51 a.m.
New subject: [libvirt] [PATCH 1/2] util: Add "shareable" field for virSCSIDevice struct
Unlike the host devices of other types, SCSI host device XML supports
"shareable" tag. This patch introduces it for the virSCSIDevice struct
for a later patch use (to detect if the SCSI device is shareable when
preparing the SCSI host device in QEMU driver).
---
src/libvirt_private.syms | 1 +
src/qemu/qemu_cgroup.c | 3 ++-
src/qemu/qemu_hostdev.c | 9 ++++++---
src/security/security_apparmor.c | 3 ++-
src/security/security_dac.c | 6 ++++--
src/security/security_selinux.c | 6 ++++--
src/util/virscsi.c | 11 ++++++++++-
src/util/virscsi.h | 4 +++-
8 files changed, 32 insertions(+), 11 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index fbc9e11..65d1bde 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1674,6 +1674,7 @@ virSCSIDeviceGetDevName;
virSCSIDeviceGetName;
virSCSIDeviceGetReadonly;
virSCSIDeviceGetSgName;
+virSCSIDeviceGetShareable;
virSCSIDeviceGetTarget;
virSCSIDeviceGetUnit;
virSCSIDeviceGetUsedBy;
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index a18955e..10b1131 100644
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -295,7 +295,8 @@ qemuSetupHostdevCGroup(virDomainObjPtr vm,
dev->source.subsys.u.scsi.bus,
dev->source.subsys.u.scsi.target,
dev->source.subsys.u.scsi.unit,
- dev->readonly)) == NULL)
+ dev->readonly,
+ dev->shareable)) == NULL)
goto cleanup;
if (virSCSIDeviceFileIterate(scsi,
diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c
index dee61e7..86a463a 100644
--- a/src/qemu/qemu_hostdev.c
+++ b/src/qemu/qemu_hostdev.c
@@ -267,7 +267,8 @@ qemuUpdateActiveScsiHostdevs(virQEMUDriverPtr driver,
hostdev->source.subsys.u.scsi.bus,
hostdev->source.subsys.u.scsi.target,
hostdev->source.subsys.u.scsi.unit,
- hostdev->readonly)))
+ hostdev->readonly,
+ hostdev->shareable)))
goto cleanup;
virSCSIDeviceSetUsedBy(scsi, def->name);
@@ -1097,7 +1098,8 @@ qemuPrepareHostdevSCSIDevices(virQEMUDriverPtr driver,
hostdev->source.subsys.u.scsi.bus,
hostdev->source.subsys.u.scsi.target,
hostdev->source.subsys.u.scsi.unit,
- hostdev->readonly)))
+ hostdev->readonly,
+ hostdev->shareable)))
goto cleanup;
if (scsi && virSCSIDeviceListAdd(list, scsi) < 0) {
@@ -1395,7 +1397,8 @@ qemuDomainReAttachHostScsiDevices(virQEMUDriverPtr driver,
hostdev->source.subsys.u.scsi.bus,
hostdev->source.subsys.u.scsi.target,
hostdev->source.subsys.u.scsi.unit,
- hostdev->readonly))) {
+ hostdev->readonly,
+ hostdev->shareable))) {
VIR_WARN("Unable to reattach SCSI device %s:%d:%d:%d on domain
%s",
hostdev->source.subsys.u.scsi.adapter,
hostdev->source.subsys.u.scsi.bus,
diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c
index a9f04d2..86a033f 100644
--- a/src/security/security_apparmor.c
+++ b/src/security/security_apparmor.c
@@ -833,7 +833,8 @@ AppArmorSetSecurityHostdevLabel(virSecurityManagerPtr mgr,
dev->source.subsys.u.scsi.bus,
dev->source.subsys.u.scsi.target,
dev->source.subsys.u.scsi.unit,
- dev->readonly);
+ dev->readonly,
+ dev->shareable);
if (!scsi)
goto done;
diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index cb7d322..0952df9 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -536,7 +536,8 @@ virSecurityDACSetSecurityHostdevLabel(virSecurityManagerPtr mgr,
dev->source.subsys.u.scsi.bus,
dev->source.subsys.u.scsi.target,
dev->source.subsys.u.scsi.unit,
- dev->readonly);
+ dev->readonly,
+ dev->shareable);
if (!scsi)
goto done;
@@ -653,7 +654,8 @@ virSecurityDACRestoreSecurityHostdevLabel(virSecurityManagerPtr mgr,
dev->source.subsys.u.scsi.bus,
dev->source.subsys.u.scsi.target,
dev->source.subsys.u.scsi.unit,
- dev->readonly);
+ dev->readonly,
+ dev->shareable);
if (!scsi)
goto done;
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index 11c0c3b..37f755c 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -1353,7 +1353,8 @@ virSecuritySELinuxSetSecurityHostdevSubsysLabel(virDomainDefPtr
def,
dev->source.subsys.u.scsi.bus,
dev->source.subsys.u.scsi.target,
dev->source.subsys.u.scsi.unit,
- dev->readonly);
+ dev->readonly,
+ dev->shareable);
if (!scsi)
goto done;
@@ -1544,7 +1545,8 @@
virSecuritySELinuxRestoreSecurityHostdevSubsysLabel(virSecurityManagerPtr mgr,
dev->source.subsys.u.scsi.bus,
dev->source.subsys.u.scsi.target,
dev->source.subsys.u.scsi.unit,
- dev->readonly);
+ dev->readonly,
+ dev->shareable);
if (!scsi)
goto done;
diff --git a/src/util/virscsi.c b/src/util/virscsi.c
index 751eaf0..3998c3a 100644
--- a/src/util/virscsi.c
+++ b/src/util/virscsi.c
@@ -58,6 +58,7 @@ struct _virSCSIDevice {
const char *used_by; /* name of the domain using this dev */
bool readonly;
+ bool shareable;
};
struct _virSCSIDeviceList {
@@ -185,7 +186,8 @@ virSCSIDeviceNew(const char *adapter,
unsigned int bus,
unsigned int target,
unsigned int unit,
- bool readonly)
+ bool readonly,
+ bool shareable)
{
virSCSIDevicePtr dev, ret = NULL;
char *sg = NULL;
@@ -201,6 +203,7 @@ virSCSIDeviceNew(const char *adapter,
dev->target = target;
dev->unit = unit;
dev->readonly = readonly;
+ dev->shareable= shareable;
if (!(sg = virSCSIDeviceGetSgName(adapter, bus, target, unit)))
goto cleanup;
@@ -311,6 +314,12 @@ virSCSIDeviceGetReadonly(virSCSIDevicePtr dev)
return dev->readonly;
}
+bool
+virSCSIDeviceGetShareable(virSCSIDevicePtr dev)
+{
+ return dev->shareable;
+}
+
int
virSCSIDeviceFileIterate(virSCSIDevicePtr dev,
virSCSIDeviceFileActor actor,
diff --git a/src/util/virscsi.h b/src/util/virscsi.h
index 4c461f8..b2e98ca 100644
--- a/src/util/virscsi.h
+++ b/src/util/virscsi.h
@@ -46,7 +46,8 @@ virSCSIDevicePtr virSCSIDeviceNew(const char *adapter,
unsigned int bus,
unsigned int target,
unsigned int unit,
- bool readonly);
+ bool readonly,
+ bool shareable);
void virSCSIDeviceFree(virSCSIDevicePtr dev);
void virSCSIDeviceSetUsedBy(virSCSIDevicePtr dev, const char *name);
@@ -57,6 +58,7 @@ unsigned int virSCSIDeviceGetBus(virSCSIDevicePtr dev);
unsigned int virSCSIDeviceGetTarget(virSCSIDevicePtr dev);
unsigned int virSCSIDeviceGetUnit(virSCSIDevicePtr dev);
bool virSCSIDeviceGetReadonly(virSCSIDevicePtr dev);
+bool virSCSIDeviceGetShareable(virSCSIDevicePtr dev);
/*
* Callback that will be invoked once for each file
--
1.8.1.4
6:51 p.m.
New subject: [libvirt] [PATCH 1/2] util: Add "shareable" field for virSCSIDevice struct
On 01/08/2014 09:51 AM, Osier Yang wrote:
Unlike the host devices of other types, SCSI host device XML
supports
"shareable" tag. This patch introduces it for the virSCSIDevice struct
for a later patch use (to detect if the SCSI device is shareable when
preparing the SCSI host device in QEMU driver).
---
src/libvirt_private.syms | 1 +
src/qemu/qemu_cgroup.c | 3 ++-
src/qemu/qemu_hostdev.c | 9 ++++++---
src/security/security_apparmor.c | 3 ++-
src/security/security_dac.c | 6 ++++--
src/security/security_selinux.c | 6 ++++--
src/util/virscsi.c | 11 ++++++++++-
src/util/virscsi.h | 4 +++-
8 files changed, 32 insertions(+), 11 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index fbc9e11..65d1bde 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1674,6 +1674,7 @@ virSCSIDeviceGetDevName;
virSCSIDeviceGetName;
virSCSIDeviceGetReadonly;
virSCSIDeviceGetSgName;
+virSCSIDeviceGetShareable;
virSCSIDeviceGetTarget;
virSCSIDeviceGetUnit;
virSCSIDeviceGetUsedBy;
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index a18955e..10b1131 100644
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -295,7 +295,8 @@ qemuSetupHostdevCGroup(virDomainObjPtr vm,
dev->source.subsys.u.scsi.bus,
dev->source.subsys.u.scsi.target,
dev->source.subsys.u.scsi.unit,
- dev->readonly)) == NULL)
+ dev->readonly,
+ dev->shareable)) == NULL)
goto cleanup;
if (virSCSIDeviceFileIterate(scsi,
diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c
index dee61e7..86a463a 100644
--- a/src/qemu/qemu_hostdev.c
+++ b/src/qemu/qemu_hostdev.c
@@ -267,7 +267,8 @@ qemuUpdateActiveScsiHostdevs(virQEMUDriverPtr driver,
hostdev->source.subsys.u.scsi.bus,
hostdev->source.subsys.u.scsi.target,
hostdev->source.subsys.u.scsi.unit,
- hostdev->readonly)))
+ hostdev->readonly,
+ hostdev->shareable)))
goto cleanup;
virSCSIDeviceSetUsedBy(scsi, def->name);
@@ -1097,7 +1098,8 @@ qemuPrepareHostdevSCSIDevices(virQEMUDriverPtr driver,
hostdev->source.subsys.u.scsi.bus,
hostdev->source.subsys.u.scsi.target,
hostdev->source.subsys.u.scsi.unit,
- hostdev->readonly)))
+ hostdev->readonly,
+ hostdev->shareable)))
goto cleanup;
if (scsi && virSCSIDeviceListAdd(list, scsi) < 0) {
@@ -1395,7 +1397,8 @@ qemuDomainReAttachHostScsiDevices(virQEMUDriverPtr driver,
hostdev->source.subsys.u.scsi.bus,
hostdev->source.subsys.u.scsi.target,
hostdev->source.subsys.u.scsi.unit,
- hostdev->readonly))) {
+ hostdev->readonly,
+ hostdev->shareable))) {
VIR_WARN("Unable to reattach SCSI device %s:%d:%d:%d on domain
%s",
hostdev->source.subsys.u.scsi.adapter,
hostdev->source.subsys.u.scsi.bus,
diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c
index a9f04d2..86a033f 100644
--- a/src/security/security_apparmor.c
+++ b/src/security/security_apparmor.c
@@ -833,7 +833,8 @@ AppArmorSetSecurityHostdevLabel(virSecurityManagerPtr mgr,
dev->source.subsys.u.scsi.bus,
dev->source.subsys.u.scsi.target,
dev->source.subsys.u.scsi.unit,
- dev->readonly);
+ dev->readonly,
+ dev->shareable);
if (!scsi)
goto done;
diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index cb7d322..0952df9 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -536,7 +536,8 @@ virSecurityDACSetSecurityHostdevLabel(virSecurityManagerPtr mgr,
dev->source.subsys.u.scsi.bus,
dev->source.subsys.u.scsi.target,
dev->source.subsys.u.scsi.unit,
- dev->readonly);
+ dev->readonly,
+ dev->shareable);
if (!scsi)
goto done;
@@ -653,7 +654,8 @@ virSecurityDACRestoreSecurityHostdevLabel(virSecurityManagerPtr mgr,
dev->source.subsys.u.scsi.bus,
dev->source.subsys.u.scsi.target,
dev->source.subsys.u.scsi.unit,
- dev->readonly);
+ dev->readonly,
+ dev->shareable);
if (!scsi)
goto done;
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index 11c0c3b..37f755c 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -1353,7 +1353,8 @@ virSecuritySELinuxSetSecurityHostdevSubsysLabel(virDomainDefPtr
def,
dev->source.subsys.u.scsi.bus,
dev->source.subsys.u.scsi.target,
dev->source.subsys.u.scsi.unit,
- dev->readonly);
+ dev->readonly,
+ dev->shareable);
if (!scsi)
goto done;
@@ -1544,7 +1545,8 @@
virSecuritySELinuxRestoreSecurityHostdevSubsysLabel(virSecurityManagerPtr mgr,
dev->source.subsys.u.scsi.bus,
dev->source.subsys.u.scsi.target,
dev->source.subsys.u.scsi.unit,
- dev->readonly);
+ dev->readonly,
+ dev->shareable);
if (!scsi)
goto done;
diff --git a/src/util/virscsi.c b/src/util/virscsi.c
index 751eaf0..3998c3a 100644
--- a/src/util/virscsi.c
+++ b/src/util/virscsi.c
@@ -58,6 +58,7 @@ struct _virSCSIDevice {
const char *used_by; /* name of the domain using this dev */
bool readonly;
+ bool shareable;
};
struct _virSCSIDeviceList {
@@ -185,7 +186,8 @@ virSCSIDeviceNew(const char *adapter,
unsigned int bus,
unsigned int target,
unsigned int unit,
- bool readonly)
+ bool readonly,
+ bool shareable)
{
virSCSIDevicePtr dev, ret = NULL;
char *sg = NULL;
@@ -201,6 +203,7 @@ virSCSIDeviceNew(const char *adapter,
dev->target = target;
dev->unit = unit;
dev->readonly = readonly;
+ dev->shareable= shareable;
You still didn't add the space here before the "="
ACK if you do. I don't believe this is 1.2.1 material.
John
if (!(sg = virSCSIDeviceGetSgName(adapter, bus, target, unit)))
goto cleanup;
@@ -311,6 +314,12 @@ virSCSIDeviceGetReadonly(virSCSIDevicePtr dev)
return dev->readonly;
}
+bool
+virSCSIDeviceGetShareable(virSCSIDevicePtr dev)
+{
+ return dev->shareable;
+}
+
int
virSCSIDeviceFileIterate(virSCSIDevicePtr dev,
virSCSIDeviceFileActor actor,
diff --git a/src/util/virscsi.h b/src/util/virscsi.h
index 4c461f8..b2e98ca 100644
--- a/src/util/virscsi.h
+++ b/src/util/virscsi.h
@@ -46,7 +46,8 @@ virSCSIDevicePtr virSCSIDeviceNew(const char *adapter,
unsigned int bus,
unsigned int target,
unsigned int unit,
- bool readonly);
+ bool readonly,
+ bool shareable);
void virSCSIDeviceFree(virSCSIDevicePtr dev);
void virSCSIDeviceSetUsedBy(virSCSIDevicePtr dev, const char *name);
@@ -57,6 +58,7 @@ unsigned int virSCSIDeviceGetBus(virSCSIDevicePtr dev);
unsigned int virSCSIDeviceGetTarget(virSCSIDevicePtr dev);
unsigned int virSCSIDeviceGetUnit(virSCSIDevicePtr dev);
bool virSCSIDeviceGetReadonly(virSCSIDevicePtr dev);
+bool virSCSIDeviceGetShareable(virSCSIDevicePtr dev);
/*
* Callback that will be invoked once for each file
3:56 a.m.
New subject: [libvirt] [PATCH 1/2] util: Add "shareable" field for virSCSIDevice struct
On 16/01/14 08:51, John Ferlan wrote:
On 01/08/2014 09:51 AM, Osier Yang wrote:
<...>
diff --git a/src/util/virscsi.c b/src/util/virscsi.c
index 751eaf0..3998c3a 100644
--- a/src/util/virscsi.c
+++ b/src/util/virscsi.c
@@ -58,6 +58,7 @@ struct _virSCSIDevice {
const char *used_by; /* name of the domain using this dev */
bool readonly;
+ bool shareable;
};
struct _virSCSIDeviceList {
@@ -185,7 +186,8 @@ virSCSIDeviceNew(const char *adapter,
unsigned int bus,
unsigned int target,
unsigned int unit,
- bool readonly)
+ bool readonly,
+ bool shareable)
{
virSCSIDevicePtr dev, ret = NULL;
char *sg = NULL;
@@ -201,6 +203,7 @@ virSCSIDeviceNew(const char *adapter,
dev->target = target;
dev->unit = unit;
dev->readonly = readonly;
+ dev->shareable= shareable;
You still didn't add the space here before the "="
ACK if you do. I don't believe this is 1.2.1 material.
This patch is standalone. Pushed with the indention fixed.
Osier
8:51 a.m.
New subject: [libvirt] [PATCH 2/2] qemu: Don't fail if the SCSI host device is shareable between domains
It doesn't make sense to fail if the SCSI host device is specified
as "shareable" explicitly between domains (NB, it works if and only
if the device is specified as "shareable" for *all* domains,
otherwise it fails).
To fix the problem, this patch introduces an array for virSCSIDevice
struct, which records all the names of domain which are using the
device (note that the recorded domains must specifiy the device as
shareable). And the change on the data struct brings on many
subsequent changes in the code.
* src/util/virscsi.h:
- Remove virSCSIDeviceGetUsedBy
- Change definition of virSCSIDeviceGetUsedBy and virSCSIDeviceListDel
- Add virSCSIDeviceIsAvailable
* src/util/virscsi.c:
- struct virSCSIDevice: Change "used_by" to be an array; Add
"n_used_by" as the array count
- virSCSIDeviceGetUsedBy: Removed
- virSCSIDeviceFree: frees the "used_by" array
- virSCSIDeviceSetUsedBy: Copy the domain name to avoid potential
memory corruption
- virSCSIDeviceIsAvailable: New
- virSCSIDeviceListDel: Change the logic, for device which is already
in the list, just remove the corresponding entry in "used_by"
- Copyright updating
* src/libvirt_private.sys:
- virSCSIDeviceGetUsedBy: Remove
- virSCSIDeviceIsAvailable: New
* src/qemu/qemu_hostdev.c:
- qemuUpdateActiveScsiHostdevs: Check if the device existing before
adding it to the list;
- qemuPrepareHostdevSCSIDevices: Error out if the not all domains
use the device as "shareable"; Also don't try to add the device
to the activeScsiHostdevs list if it already there.
- qemuDomainReAttachHostScsiDevices: Change the logic according
to the changes on helpers.
---
src/libvirt_private.syms | 2 +-
src/qemu/qemu_hostdev.c | 75 ++++++++++++++++++++++++++----------------------
src/util/virscsi.c | 48 +++++++++++++++++++++++++------
src/util/virscsi.h | 7 +++--
4 files changed, 84 insertions(+), 48 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 65d1bde..bd5f466 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1677,7 +1677,7 @@ virSCSIDeviceGetSgName;
virSCSIDeviceGetShareable;
virSCSIDeviceGetTarget;
virSCSIDeviceGetUnit;
-virSCSIDeviceGetUsedBy;
+virSCSIDeviceIsAvailable;
virSCSIDeviceListAdd;
virSCSIDeviceListCount;
virSCSIDeviceListDel;
diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c
index 86a463a..9d81e94 100644
--- a/src/qemu/qemu_hostdev.c
+++ b/src/qemu/qemu_hostdev.c
@@ -250,13 +250,14 @@ qemuUpdateActiveScsiHostdevs(virQEMUDriverPtr driver,
virDomainHostdevDefPtr hostdev = NULL;
size_t i;
int ret = -1;
+ virSCSIDevicePtr scsi = NULL;
+ virSCSIDevicePtr tmp = NULL;
if (!def->nhostdevs)
return 0;
virObjectLock(driver->activeScsiHostdevs);
for (i = 0; i < def->nhostdevs; i++) {
- virSCSIDevicePtr scsi = NULL;
hostdev = def->hostdevs[i];
if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS ||
@@ -271,11 +272,18 @@ qemuUpdateActiveScsiHostdevs(virQEMUDriverPtr driver,
hostdev->shareable)))
goto cleanup;
- virSCSIDeviceSetUsedBy(scsi, def->name);
-
- if (virSCSIDeviceListAdd(driver->activeScsiHostdevs, scsi) < 0) {
+ if ((tmp = virSCSIDeviceListFind(driver->activeScsiHostdevs, scsi))) {
+ if (virSCSIDeviceSetUsedBy(tmp, def->name) < 0) {
+ virSCSIDeviceFree(scsi);
+ goto cleanup;
+ }
virSCSIDeviceFree(scsi);
- goto cleanup;
+ } else {
+ if (virSCSIDeviceSetUsedBy(scsi, def->name) < 0 ||
+ virSCSIDeviceListAdd(driver->activeScsiHostdevs, scsi) < 0) {
+ virSCSIDeviceFree(scsi);
+ goto cleanup;
+ }
}
}
ret = 0;
@@ -1118,24 +1126,26 @@ qemuPrepareHostdevSCSIDevices(virQEMUDriverPtr driver,
for (i = 0; i < count; i++) {
virSCSIDevicePtr scsi = virSCSIDeviceListGet(list, i);
if ((tmp = virSCSIDeviceListFind(driver->activeScsiHostdevs, scsi))) {
- const char *other_name = virSCSIDeviceGetUsedBy(tmp);
-
- if (other_name)
- virReportError(VIR_ERR_OPERATION_INVALID,
- _("SCSI device %s is in use by domain %s"),
- virSCSIDeviceGetName(tmp), other_name);
- else
+ if (!(virSCSIDeviceGetShareable(scsi) &&
+ virSCSIDeviceGetShareable(tmp))) {
virReportError(VIR_ERR_OPERATION_INVALID,
- _("SCSI device %s is already in use"),
+ _("SCSI device %s is already in use "
+ "by other domain(s)"),
virSCSIDeviceGetName(tmp));
- goto error;
- }
+ goto error;
+ }
- virSCSIDeviceSetUsedBy(scsi, name);
- VIR_DEBUG("Adding %s to activeScsiHostdevs",
virSCSIDeviceGetName(scsi));
+ if (virSCSIDeviceSetUsedBy(tmp, name) < 0)
+ goto error;
+ } else {
+ if (virSCSIDeviceSetUsedBy(scsi, name) < 0)
+ goto error;
- if (virSCSIDeviceListAdd(driver->activeScsiHostdevs, scsi) < 0)
- goto error;
+ VIR_DEBUG("Adding %s to activeScsiHostdevs",
virSCSIDeviceGetName(scsi));
+
+ if (virSCSIDeviceListAdd(driver->activeScsiHostdevs, scsi) < 0)
+ goto error;
+ }
}
virObjectUnlock(driver->activeScsiHostdevs);
@@ -1380,8 +1390,7 @@ qemuDomainReAttachHostScsiDevices(virQEMUDriverPtr driver,
virObjectLock(driver->activeScsiHostdevs);
for (i = 0; i < nhostdevs; i++) {
virDomainHostdevDefPtr hostdev = hostdevs[i];
- virSCSIDevicePtr scsi, tmp;
- const char *used_by = NULL;
+ virSCSIDevicePtr scsi;
virDomainDeviceDef dev;
dev.type = VIR_DOMAIN_DEVICE_HOSTDEV;
@@ -1411,30 +1420,26 @@ qemuDomainReAttachHostScsiDevices(virQEMUDriverPtr driver,
/* Only delete the devices which are marked as being used by @name,
* because qemuProcessStart could fail on the half way. */
- tmp = virSCSIDeviceListFind(driver->activeScsiHostdevs, scsi);
- virSCSIDeviceFree(scsi);
-
- if (!tmp) {
+ if (!virSCSIDeviceListFind(driver->activeScsiHostdevs, scsi)) {
VIR_WARN("Unable to find device %s:%d:%d:%d "
"in list of active SCSI devices",
hostdev->source.subsys.u.scsi.adapter,
hostdev->source.subsys.u.scsi.bus,
hostdev->source.subsys.u.scsi.target,
hostdev->source.subsys.u.scsi.unit);
+ virSCSIDeviceFree(scsi);
continue;
}
- used_by = virSCSIDeviceGetUsedBy(tmp);
- if (STREQ_NULLABLE(used_by, name)) {
- VIR_DEBUG("Removing %s:%d:%d:%d dom=%s from activeScsiHostdevs",
- hostdev->source.subsys.u.scsi.adapter,
- hostdev->source.subsys.u.scsi.bus,
- hostdev->source.subsys.u.scsi.target,
- hostdev->source.subsys.u.scsi.unit,
- name);
+ VIR_DEBUG("Removing %s:%d:%d:%d dom=%s from activeScsiHostdevs",
+ hostdev->source.subsys.u.scsi.adapter,
+ hostdev->source.subsys.u.scsi.bus,
+ hostdev->source.subsys.u.scsi.target,
+ hostdev->source.subsys.u.scsi.unit,
+ name);
- virSCSIDeviceListDel(driver->activeScsiHostdevs, tmp);
- }
+ virSCSIDeviceListDel(driver->activeScsiHostdevs, scsi, name);
+ virSCSIDeviceFree(scsi);
}
virObjectUnlock(driver->activeScsiHostdevs);
}
diff --git a/src/util/virscsi.c b/src/util/virscsi.c
index 3998c3a..42030c5 100644
--- a/src/util/virscsi.c
+++ b/src/util/virscsi.c
@@ -1,6 +1,7 @@
/*
* virscsi.c: helper APIs for managing host SCSI devices
*
+ * Copyright (C) 2013 - 2014 Red Hat, Inc.
* Copyright (C) 2013 Fujitsu, Inc.
*
* This library is free software; you can redistribute it and/or
@@ -55,7 +56,8 @@ struct _virSCSIDevice {
char *name; /* adapter:bus:target:unit */
char *id; /* model:vendor */
char *sg_path; /* e.g. /dev/sg2 */
- const char *used_by; /* name of the domain using this dev */
+ char **used_by; /* name of the domains using this dev */
+ size_t n_used_by; /* how many domains are using this dev */
bool readonly;
bool shareable;
@@ -256,26 +258,37 @@ cleanup:
void
virSCSIDeviceFree(virSCSIDevicePtr dev)
{
+ size_t i;
+
if (!dev)
return;
VIR_FREE(dev->id);
VIR_FREE(dev->name);
VIR_FREE(dev->sg_path);
+ for (i = 0; i < dev->n_used_by; i++) {
+ VIR_FREE(dev->used_by[i]);
+ }
+ VIR_FREE(dev->used_by);
VIR_FREE(dev);
}
-void
+int
virSCSIDeviceSetUsedBy(virSCSIDevicePtr dev,
const char *name)
{
- dev->used_by = name;
+ char *copy = NULL;
+
+ if (VIR_STRDUP(copy, name) < 0)
+ return -1;
+
+ return VIR_APPEND_ELEMENT(dev->used_by, dev->n_used_by, copy);
}
-const char *
-virSCSIDeviceGetUsedBy(virSCSIDevicePtr dev)
+bool
+virSCSIDeviceIsAvailable(virSCSIDevicePtr dev)
{
- return dev->used_by;
+ return dev->n_used_by == 0;
}
const char *
@@ -406,10 +419,27 @@ virSCSIDeviceListSteal(virSCSIDeviceListPtr list,
void
virSCSIDeviceListDel(virSCSIDeviceListPtr list,
- virSCSIDevicePtr dev)
+ virSCSIDevicePtr dev,
+ const char *name)
{
- virSCSIDevicePtr ret = virSCSIDeviceListSteal(list, dev);
- virSCSIDeviceFree(ret);
+ virSCSIDevicePtr ret = NULL;
+ size_t i;
+
+ if (!(ret = virSCSIDeviceListFind(list, dev)))
+ return;
+
+ for (i = 0; i < ret->n_used_by; i++) {
+ if (STREQ_NULLABLE(ret->used_by[i], name)) {
+ if (ret->n_used_by > 1) {
+ VIR_DELETE_ELEMENT(ret->used_by, i, ret->n_used_by);
+ } else {
+ ret = virSCSIDeviceListSteal(list, dev);
+ virSCSIDeviceFree(ret);
+ }
+
+ break;
+ }
+ }
}
virSCSIDevicePtr
diff --git a/src/util/virscsi.h b/src/util/virscsi.h
index b2e98ca..aff7e5a 100644
--- a/src/util/virscsi.h
+++ b/src/util/virscsi.h
@@ -50,8 +50,8 @@ virSCSIDevicePtr virSCSIDeviceNew(const char *adapter,
bool shareable);
void virSCSIDeviceFree(virSCSIDevicePtr dev);
-void virSCSIDeviceSetUsedBy(virSCSIDevicePtr dev, const char *name);
-const char *virSCSIDeviceGetUsedBy(virSCSIDevicePtr dev);
+int virSCSIDeviceSetUsedBy(virSCSIDevicePtr dev, const char *name);
+bool virSCSIDeviceIsAvailable(virSCSIDevicePtr dev);
const char *virSCSIDeviceGetName(virSCSIDevicePtr dev);
unsigned int virSCSIDeviceGetAdapter(virSCSIDevicePtr dev);
unsigned int virSCSIDeviceGetBus(virSCSIDevicePtr dev);
@@ -83,7 +83,8 @@ size_t virSCSIDeviceListCount(virSCSIDeviceListPtr list);
virSCSIDevicePtr virSCSIDeviceListSteal(virSCSIDeviceListPtr list,
virSCSIDevicePtr dev);
void virSCSIDeviceListDel(virSCSIDeviceListPtr list,
- virSCSIDevicePtr dev);
+ virSCSIDevicePtr dev,
+ const char *name);
virSCSIDevicePtr virSCSIDeviceListFind(virSCSIDeviceListPtr list,
virSCSIDevicePtr dev);
--
1.8.1.4
6:51 p.m.
New subject: [libvirt] [PATCH 2/2] qemu: Don't fail if the SCSI host device is shareable between domains
On 01/08/2014 09:51 AM, Osier Yang wrote:
It doesn't make sense to fail if the SCSI host device is
specified
as "shareable" explicitly between domains (NB, it works if and only
if the device is specified as "shareable" for *all* domains,
otherwise it fails).
To fix the problem, this patch introduces an array for virSCSIDevice
struct, which records all the names of domain which are using the
device (note that the recorded domains must specifiy the device as
shareable). And the change on the data struct brings on many
subsequent changes in the code.
* src/util/virscsi.h:
- Remove virSCSIDeviceGetUsedBy
- Change definition of virSCSIDeviceGetUsedBy and virSCSIDeviceListDel
- Add virSCSIDeviceIsAvailable
* src/util/virscsi.c:
- struct virSCSIDevice: Change "used_by" to be an array; Add
"n_used_by" as the array count
- virSCSIDeviceGetUsedBy: Removed
- virSCSIDeviceFree: frees the "used_by" array
- virSCSIDeviceSetUsedBy: Copy the domain name to avoid potential
memory corruption
- virSCSIDeviceIsAvailable: New
- virSCSIDeviceListDel: Change the logic, for device which is already
in the list, just remove the corresponding entry in "used_by"
- Copyright updating
* src/libvirt_private.sys:
- virSCSIDeviceGetUsedBy: Remove
- virSCSIDeviceIsAvailable: New
* src/qemu/qemu_hostdev.c:
- qemuUpdateActiveScsiHostdevs: Check if the device existing before
adding it to the list;
- qemuPrepareHostdevSCSIDevices: Error out if the not all domains
use the device as "shareable"; Also don't try to add the device
to the activeScsiHostdevs list if it already there.
- qemuDomainReAttachHostScsiDevices: Change the logic according
to the changes on helpers.
---
src/libvirt_private.syms | 2 +-
src/qemu/qemu_hostdev.c | 75 ++++++++++++++++++++++++++----------------------
src/util/virscsi.c | 48 +++++++++++++++++++++++++------
src/util/virscsi.h | 7 +++--
4 files changed, 84 insertions(+), 48 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 65d1bde..bd5f466 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1677,7 +1677,7 @@ virSCSIDeviceGetSgName;
virSCSIDeviceGetShareable;
virSCSIDeviceGetTarget;
virSCSIDeviceGetUnit;
-virSCSIDeviceGetUsedBy;
+virSCSIDeviceIsAvailable;
virSCSIDeviceListAdd;
virSCSIDeviceListCount;
virSCSIDeviceListDel;
diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c
index 86a463a..9d81e94 100644
--- a/src/qemu/qemu_hostdev.c
+++ b/src/qemu/qemu_hostdev.c
@@ -250,13 +250,14 @@ qemuUpdateActiveScsiHostdevs(virQEMUDriverPtr driver,
virDomainHostdevDefPtr hostdev = NULL;
size_t i;
int ret = -1;
+ virSCSIDevicePtr scsi = NULL;
+ virSCSIDevicePtr tmp = NULL;
if (!def->nhostdevs)
return 0;
virObjectLock(driver->activeScsiHostdevs);
for (i = 0; i < def->nhostdevs; i++) {
- virSCSIDevicePtr scsi = NULL;
hostdev = def->hostdevs[i];
if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS ||
@@ -271,11 +272,18 @@ qemuUpdateActiveScsiHostdevs(virQEMUDriverPtr driver,
hostdev->shareable)))
goto cleanup;
- virSCSIDeviceSetUsedBy(scsi, def->name);
-
- if (virSCSIDeviceListAdd(driver->activeScsiHostdevs, scsi) < 0) {
+ if ((tmp = virSCSIDeviceListFind(driver->activeScsiHostdevs, scsi))) {
+ if (virSCSIDeviceSetUsedBy(tmp, def->name) < 0) {
+ virSCSIDeviceFree(scsi);
+ goto cleanup;
+ }
virSCSIDeviceFree(scsi);
- goto cleanup;
+ } else {
+ if (virSCSIDeviceSetUsedBy(scsi, def->name) < 0 ||
+ virSCSIDeviceListAdd(driver->activeScsiHostdevs, scsi) < 0) {
+ virSCSIDeviceFree(scsi);
+ goto cleanup;
+ }
It took some thinking, but I convinced myself that this path doesn't
need the shared check since it's only called from qemuProcessReconnect;
however, if something else did call it some day then that check may be
necessary. It may be wise to add it anyway... I have no strong opinion
about it being required for this change.
}
}
ret = 0;
@@ -1118,24 +1126,26 @@ qemuPrepareHostdevSCSIDevices(virQEMUDriverPtr driver,
for (i = 0; i < count; i++) {
virSCSIDevicePtr scsi = virSCSIDeviceListGet(list, i);
if ((tmp = virSCSIDeviceListFind(driver->activeScsiHostdevs, scsi))) {
- const char *other_name = virSCSIDeviceGetUsedBy(tmp);
-
- if (other_name)
- virReportError(VIR_ERR_OPERATION_INVALID,
- _("SCSI device %s is in use by domain %s"),
- virSCSIDeviceGetName(tmp), other_name);
- else
+ if (!(virSCSIDeviceGetShareable(scsi) &&
+ virSCSIDeviceGetShareable(tmp))) {
virReportError(VIR_ERR_OPERATION_INVALID,
- _("SCSI device %s is already in use"),
+ _("SCSI device %s is already in use "
+ "by other domain(s)"),
virSCSIDeviceGetName(tmp));
- goto error;
- }
+ goto error;
+ }
- virSCSIDeviceSetUsedBy(scsi, name);
- VIR_DEBUG("Adding %s to activeScsiHostdevs",
virSCSIDeviceGetName(scsi));
+ if (virSCSIDeviceSetUsedBy(tmp, name) < 0)
+ goto error;
+ } else {
+ if (virSCSIDeviceSetUsedBy(scsi, name) < 0)
+ goto error;
- if (virSCSIDeviceListAdd(driver->activeScsiHostdevs, scsi) < 0)
- goto error;
+ VIR_DEBUG("Adding %s to activeScsiHostdevs",
virSCSIDeviceGetName(scsi));
+
+ if (virSCSIDeviceListAdd(driver->activeScsiHostdevs, scsi) < 0)
+ goto error;
+ }
}
virObjectUnlock(driver->activeScsiHostdevs);
@@ -1380,8 +1390,7 @@ qemuDomainReAttachHostScsiDevices(virQEMUDriverPtr driver,
virObjectLock(driver->activeScsiHostdevs);
for (i = 0; i < nhostdevs; i++) {
virDomainHostdevDefPtr hostdev = hostdevs[i];
- virSCSIDevicePtr scsi, tmp;
- const char *used_by = NULL;
+ virSCSIDevicePtr scsi;
virDomainDeviceDef dev;
dev.type = VIR_DOMAIN_DEVICE_HOSTDEV;
@@ -1411,30 +1420,26 @@ qemuDomainReAttachHostScsiDevices(virQEMUDriverPtr driver,
/* Only delete the devices which are marked as being used by @name,
* because qemuProcessStart could fail on the half way. */
- tmp = virSCSIDeviceListFind(driver->activeScsiHostdevs, scsi);
- virSCSIDeviceFree(scsi);
-
- if (!tmp) {
+ if (!virSCSIDeviceListFind(driver->activeScsiHostdevs, scsi)) {
I think you should keep (and perhaps rename) the tmp pointer and then
pass it to virSCSIDeviceListDel() since that function will call the the
find again anyway
VIR_WARN("Unable to find device %s:%d:%d:%d "
"in list of active SCSI devices",
hostdev->source.subsys.u.scsi.adapter,
hostdev->source.subsys.u.scsi.bus,
hostdev->source.subsys.u.scsi.target,
hostdev->source.subsys.u.scsi.unit);
+ virSCSIDeviceFree(scsi);
continue;
}
- used_by = virSCSIDeviceGetUsedBy(tmp);
- if (STREQ_NULLABLE(used_by, name)) {
- VIR_DEBUG("Removing %s:%d:%d:%d dom=%s from activeScsiHostdevs",
- hostdev->source.subsys.u.scsi.adapter,
- hostdev->source.subsys.u.scsi.bus,
- hostdev->source.subsys.u.scsi.target,
- hostdev->source.subsys.u.scsi.unit,
- name);
+ VIR_DEBUG("Removing %s:%d:%d:%d dom=%s from activeScsiHostdevs",
+ hostdev->source.subsys.u.scsi.adapter,
+ hostdev->source.subsys.u.scsi.bus,
+ hostdev->source.subsys.u.scsi.target,
+ hostdev->source.subsys.u.scsi.unit,
+ name);
- virSCSIDeviceListDel(driver->activeScsiHostdevs, tmp);
- }
+ virSCSIDeviceListDel(driver->activeScsiHostdevs, scsi, name);
+ virSCSIDeviceFree(scsi);
}
virObjectUnlock(driver->activeScsiHostdevs);
}
diff --git a/src/util/virscsi.c b/src/util/virscsi.c
index 3998c3a..42030c5 100644
--- a/src/util/virscsi.c
+++ b/src/util/virscsi.c
@@ -1,6 +1,7 @@
/*
* virscsi.c: helper APIs for managing host SCSI devices
*
+ * Copyright (C) 2013 - 2014 Red Hat, Inc.
* Copyright (C) 2013 Fujitsu, Inc.
*
* This library is free software; you can redistribute it and/or
@@ -55,7 +56,8 @@ struct _virSCSIDevice {
char *name; /* adapter:bus:target:unit */
char *id; /* model:vendor */
char *sg_path; /* e.g. /dev/sg2 */
- const char *used_by; /* name of the domain using this dev */
+ char **used_by; /* name of the domains using this dev */
+ size_t n_used_by; /* how many domains are using this dev */
bool readonly;
bool shareable;
@@ -256,26 +258,37 @@ cleanup:
void
virSCSIDeviceFree(virSCSIDevicePtr dev)
{
+ size_t i;
+
if (!dev)
return;
VIR_FREE(dev->id);
VIR_FREE(dev->name);
VIR_FREE(dev->sg_path);
+ for (i = 0; i < dev->n_used_by; i++) {
+ VIR_FREE(dev->used_by[i]);
+ }
+ VIR_FREE(dev->used_by);
VIR_FREE(dev);
}
-void
+int
virSCSIDeviceSetUsedBy(virSCSIDevicePtr dev,
const char *name)
{
- dev->used_by = name;
+ char *copy = NULL;
+
+ if (VIR_STRDUP(copy, name) < 0)
+ return -1;
+
+ return VIR_APPEND_ELEMENT(dev->used_by, dev->n_used_by, copy);
}
-const char *
-virSCSIDeviceGetUsedBy(virSCSIDevicePtr dev)
+bool
+virSCSIDeviceIsAvailable(virSCSIDevicePtr dev)
{
- return dev->used_by;
+ return dev->n_used_by == 0;
}
const char *
@@ -406,10 +419,27 @@ virSCSIDeviceListSteal(virSCSIDeviceListPtr list,
void
virSCSIDeviceListDel(virSCSIDeviceListPtr list,
- virSCSIDevicePtr dev)
+ virSCSIDevicePtr dev,
+ const char *name)
{
- virSCSIDevicePtr ret = virSCSIDeviceListSteal(list, dev);
- virSCSIDeviceFree(ret);
+ virSCSIDevicePtr ret = NULL;
+ size_t i;
+
+ if (!(ret = virSCSIDeviceListFind(list, dev)))
+ return;
since there's only one caller and it already did a
virSCSIDeviceListFind() (but threw away the results) - couldn't the
caller save and pass the results rather than making the same call twice?
I don't think a V3 would be necessary based on your thoughts. Again -
this is something for post 1.2.1
John
+
+ for (i = 0; i < ret->n_used_by; i++) {
+ if (STREQ_NULLABLE(ret->used_by[i], name)) {
+ if (ret->n_used_by > 1) {
+ VIR_DELETE_ELEMENT(ret->used_by, i, ret->n_used_by);
+ } else {
+ ret = virSCSIDeviceListSteal(list, dev);
+ virSCSIDeviceFree(ret);
+ }
+
+ break;
+ }
+ }
}
virSCSIDevicePtr
diff --git a/src/util/virscsi.h b/src/util/virscsi.h
index b2e98ca..aff7e5a 100644
--- a/src/util/virscsi.h
+++ b/src/util/virscsi.h
@@ -50,8 +50,8 @@ virSCSIDevicePtr virSCSIDeviceNew(const char *adapter,
bool shareable);
void virSCSIDeviceFree(virSCSIDevicePtr dev);
-void virSCSIDeviceSetUsedBy(virSCSIDevicePtr dev, const char *name);
-const char *virSCSIDeviceGetUsedBy(virSCSIDevicePtr dev);
+int virSCSIDeviceSetUsedBy(virSCSIDevicePtr dev, const char *name);
+bool virSCSIDeviceIsAvailable(virSCSIDevicePtr dev);
const char *virSCSIDeviceGetName(virSCSIDevicePtr dev);
unsigned int virSCSIDeviceGetAdapter(virSCSIDevicePtr dev);
unsigned int virSCSIDeviceGetBus(virSCSIDevicePtr dev);
@@ -83,7 +83,8 @@ size_t virSCSIDeviceListCount(virSCSIDeviceListPtr list);
virSCSIDevicePtr virSCSIDeviceListSteal(virSCSIDeviceListPtr list,
virSCSIDevicePtr dev);
void virSCSIDeviceListDel(virSCSIDeviceListPtr list,
- virSCSIDevicePtr dev);
+ virSCSIDevicePtr dev,
+ const char *name);
virSCSIDevicePtr virSCSIDeviceListFind(virSCSIDeviceListPtr list,
virSCSIDevicePtr dev);
7:31 a.m.
New subject: [libvirt] [PATCH 2/2] qemu: Don't fail if the SCSI host device is shareable between domains
On 16/01/14 08:51, John Ferlan wrote:
On 01/08/2014 09:51 AM, Osier Yang wrote:
> It doesn't make sense to fail if the SCSI host device is specified
> as "shareable" explicitly between domains (NB, it works if and only
> if the device is specified as "shareable" for *all* domains,
> otherwise it fails).
>
> To fix the problem, this patch introduces an array for virSCSIDevice
> struct, which records all the names of domain which are using the
> device (note that the recorded domains must specifiy the device as
> shareable). And the change on the data struct brings on many
> subsequent changes in the code.
>
> * src/util/virscsi.h:
> - Remove virSCSIDeviceGetUsedBy
> - Change definition of virSCSIDeviceGetUsedBy and virSCSIDeviceListDel
> - Add virSCSIDeviceIsAvailable
>
> * src/util/virscsi.c:
> - struct virSCSIDevice: Change "used_by" to be an array; Add
> "n_used_by" as the array count
> - virSCSIDeviceGetUsedBy: Removed
> - virSCSIDeviceFree: frees the "used_by" array
> - virSCSIDeviceSetUsedBy: Copy the domain name to avoid potential
> memory corruption
> - virSCSIDeviceIsAvailable: New
> - virSCSIDeviceListDel: Change the logic, for device which is already
> in the list, just remove the corresponding entry in "used_by"
> - Copyright updating
>
> * src/libvirt_private.sys:
> - virSCSIDeviceGetUsedBy: Remove
> - virSCSIDeviceIsAvailable: New
>
> * src/qemu/qemu_hostdev.c:
> - qemuUpdateActiveScsiHostdevs: Check if the device existing before
> adding it to the list;
> - qemuPrepareHostdevSCSIDevices: Error out if the not all domains
> use the device as "shareable"; Also don't try to add the device
> to the activeScsiHostdevs list if it already there.
> - qemuDomainReAttachHostScsiDevices: Change the logic according
> to the changes on helpers.
> ---
> src/libvirt_private.syms | 2 +-
> src/qemu/qemu_hostdev.c | 75 ++++++++++++++++++++++++++----------------------
> src/util/virscsi.c | 48 +++++++++++++++++++++++++------
> src/util/virscsi.h | 7 +++--
> 4 files changed, 84 insertions(+), 48 deletions(-)
......
> virObjectUnlock(driver->activeScsiHostdevs);
> @@ -1380,8 +1390,7 @@ qemuDomainReAttachHostScsiDevices(virQEMUDriverPtr driver,
> virObjectLock(driver->activeScsiHostdevs);
> for (i = 0; i < nhostdevs; i++) {
> virDomainHostdevDefPtr hostdev = hostdevs[i];
> - virSCSIDevicePtr scsi, tmp;
> - const char *used_by = NULL;
> + virSCSIDevicePtr scsi;
> virDomainDeviceDef dev;
>
> dev.type = VIR_DOMAIN_DEVICE_HOSTDEV;
> @@ -1411,30 +1420,26 @@ qemuDomainReAttachHostScsiDevices(virQEMUDriverPtr driver,
> /* Only delete the devices which are marked as being used by @name,
> * because qemuProcessStart could fail on the half way. */
>
> - tmp = virSCSIDeviceListFind(driver->activeScsiHostdevs, scsi);
> - virSCSIDeviceFree(scsi);
> -
> - if (!tmp) {
> + if (!virSCSIDeviceListFind(driver->activeScsiHostdevs, scsi)) {
I think you should keep (and perhaps rename) the tmp pointer and then
pass it to virSCSIDeviceListDel() since that function will call the the
find again anyway
> VIR_WARN("Unable to find device %s:%d:%d:%d "
> "in list of active SCSI devices",
> hostdev->source.subsys.u.scsi.adapter,
> hostdev->source.subsys.u.scsi.bus,
> hostdev->source.subsys.u.scsi.target,
> hostdev->source.subsys.u.scsi.unit);
> + virSCSIDeviceFree(scsi);
> continue;
> }
>
> - used_by = virSCSIDeviceGetUsedBy(tmp);
> - if (STREQ_NULLABLE(used_by, name)) {
> - VIR_DEBUG("Removing %s:%d:%d:%d dom=%s from
activeScsiHostdevs",
> - hostdev->source.subsys.u.scsi.adapter,
> - hostdev->source.subsys.u.scsi.bus,
> - hostdev->source.subsys.u.scsi.target,
> - hostdev->source.subsys.u.scsi.unit,
> - name);
> + VIR_DEBUG("Removing %s:%d:%d:%d dom=%s from activeScsiHostdevs",
> + hostdev->source.subsys.u.scsi.adapter,
> + hostdev->source.subsys.u.scsi.bus,
> + hostdev->source.subsys.u.scsi.target,
> + hostdev->source.subsys.u.scsi.unit,
> + name);
>
> - virSCSIDeviceListDel(driver->activeScsiHostdevs, tmp);
> - }
> + virSCSIDeviceListDel(driver->activeScsiHostdevs, scsi, name);
> + virSCSIDeviceFree(scsi);
> }
> virObjectUnlock(driver->activeScsiHostdevs);
> }
> diff --git a/src/util/virscsi.c b/src/util/virscsi.c
> index 3998c3a..42030c5 100644
> --- a/src/util/virscsi.c
> +++ b/src/util/virscsi.c
> @@ -1,6 +1,7 @@
> /*
> * virscsi.c: helper APIs for managing host SCSI devices
> *
> + * Copyright (C) 2013 - 2014 Red Hat, Inc.
> * Copyright (C) 2013 Fujitsu, Inc.
> *
> * This library is free software; you can redistribute it and/or
> @@ -55,7 +56,8 @@ struct _virSCSIDevice {
> char *name; /* adapter:bus:target:unit */
> char *id; /* model:vendor */
> char *sg_path; /* e.g. /dev/sg2 */
> - const char *used_by; /* name of the domain using this dev */
> + char **used_by; /* name of the domains using this dev */
> + size_t n_used_by; /* how many domains are using this dev */
>
> bool readonly;
> bool shareable;
> @@ -256,26 +258,37 @@ cleanup:
> void
> virSCSIDeviceFree(virSCSIDevicePtr dev)
> {
> + size_t i;
> +
> if (!dev)
> return;
>
> VIR_FREE(dev->id);
> VIR_FREE(dev->name);
> VIR_FREE(dev->sg_path);
> + for (i = 0; i < dev->n_used_by; i++) {
> + VIR_FREE(dev->used_by[i]);
> + }
> + VIR_FREE(dev->used_by);
> VIR_FREE(dev);
> }
>
> -void
> +int
> virSCSIDeviceSetUsedBy(virSCSIDevicePtr dev,
> const char *name)
> {
> - dev->used_by = name;
> + char *copy = NULL;
> +
> + if (VIR_STRDUP(copy, name) < 0)
> + return -1;
> +
> + return VIR_APPEND_ELEMENT(dev->used_by, dev->n_used_by, copy);
> }
>
> -const char *
> -virSCSIDeviceGetUsedBy(virSCSIDevicePtr dev)
> +bool
> +virSCSIDeviceIsAvailable(virSCSIDevicePtr dev)
> {
> - return dev->used_by;
> + return dev->n_used_by == 0;
> }
>
> const char *
> @@ -406,10 +419,27 @@ virSCSIDeviceListSteal(virSCSIDeviceListPtr list,
>
> void
> virSCSIDeviceListDel(virSCSIDeviceListPtr list,
> - virSCSIDevicePtr dev)
> + virSCSIDevicePtr dev,
> + const char *name)
> {
> - virSCSIDevicePtr ret = virSCSIDeviceListSteal(list, dev);
> - virSCSIDeviceFree(ret);
> + virSCSIDevicePtr ret = NULL;
> + size_t i;
> +
> + if (!(ret = virSCSIDeviceListFind(list, dev)))
> + return;
since there's only one caller and it already did a
virSCSIDeviceListFind() (but threw away the results) - couldn't the
caller save and pass the results rather than making the same call twice?
I don't think a V3 would be necessary based on your thoughts. Again -
this is something for post 1.2.1
Agreed, I will change when pushing later. Thanks for the reviewing.
Osier
7:53 a.m.
New subject: [libvirt] [PATCH 2/2] qemu: Don't fail if the SCSI host device is shareable between domains
[ shrinked ]
---
src/libvirt_private.syms | 2 +-
src/qemu/qemu_hostdev.c | 75 ++++++++++++++++++++++++++----------------------
src/util/virscsi.c | 48 +++++++++++++++++++++++++------
src/util/virscsi.h | 7 +++--
4 files changed, 84 insertions(+), 48 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 65d1bde..bd5f466 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1677,7 +1677,7 @@ virSCSIDeviceGetSgName;
virSCSIDeviceGetShareable;
virSCSIDeviceGetTarget;
virSCSIDeviceGetUnit;
-virSCSIDeviceGetUsedBy;
+virSCSIDeviceIsAvailable;
virSCSIDeviceListAdd;
virSCSIDeviceListCount;
virSCSIDeviceListDel;
diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c
index 86a463a..9d81e94 100644
--- a/src/qemu/qemu_hostdev.c
+++ b/src/qemu/qemu_hostdev.c
@@ -250,13 +250,14 @@ qemuUpdateActiveScsiHostdevs(virQEMUDriverPtr driver,
virDomainHostdevDefPtr hostdev = NULL;
size_t i;
int ret = -1;
+ virSCSIDevicePtr scsi = NULL;
+ virSCSIDevicePtr tmp = NULL;
if (!def->nhostdevs)
return 0;
virObjectLock(driver->activeScsiHostdevs);
for (i = 0; i < def->nhostdevs; i++) {
- virSCSIDevicePtr scsi = NULL;
hostdev = def->hostdevs[i];
if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS ||
@@ -271,11 +272,18 @@ qemuUpdateActiveScsiHostdevs(virQEMUDriverPtr driver,
hostdev->shareable)))
goto cleanup;
- virSCSIDeviceSetUsedBy(scsi, def->name);
-
- if (virSCSIDeviceListAdd(driver->activeScsiHostdevs, scsi) < 0) {
+ if ((tmp = virSCSIDeviceListFind(driver->activeScsiHostdevs, scsi))) {
+ if (virSCSIDeviceSetUsedBy(tmp, def->name) < 0) {
+ virSCSIDeviceFree(scsi);
+ goto cleanup;
+ }
virSCSIDeviceFree(scsi);
- goto cleanup;
+ } else {
+ if (virSCSIDeviceSetUsedBy(scsi, def->name) < 0 ||
+ virSCSIDeviceListAdd(driver->activeScsiHostdevs, scsi) < 0) {
+ virSCSIDeviceFree(scsi);
+ goto cleanup;
+ }
It took some thinking, but I convinced myself that this path
doesn't
need the shared check since it's only called from qemuProcessReconnect;
however, if something else did call it some day then that check may be
necessary. It may be wise to add it anyway... I have no strong opinion
about it being required for this change.
Missed reply for this one.
Actually it needs the checking. Assuming there are 2 live domains, and
they are using the same SCSI generic device. It's possible since the
device could be shared among domains. And in that case, we should
update the "dev->used_by" array instead of adding the device to the list
("driver->activeScsiHostdevs") directly, during the reconnecting sequence.
I.E it needs to restore the internal data correctly to the state just as the
one before libvirtd getting shutdown.
Regards,
Osier
9:39 a.m.
New subject: [libvirt] [PATCH 2/2] qemu: Don't fail if the SCSI host device is shareable between domains
On 01/16/2014 08:53 AM, Osier Yang wrote:
<...snip...>
> It took some thinking, but I convinced myself that this path doesn't
> need the shared check since it's only called from qemuProcessReconnect;
> however, if something else did call it some day then that check may be
> necessary. It may be wise to add it anyway... I have no strong opinion
> about it being required for this change.
>
Missed reply for this one.
Actually it needs the checking. Assuming there are 2 live domains, and
they are using the same SCSI generic device. It's possible since the
device could be shared among domains. And in that case, we should
update the "dev->used_by" array instead of adding the device to the list
("driver->activeScsiHostdevs") directly, during the reconnecting sequence.
I.E it needs to restore the internal data correctly to the state just as the
one before libvirtd getting shutdown.
The activeScsiHostdevs is listed off the driver and not the domain, so
it's not like each domain has to update which other domains is sharing.
Maybe I'm misreading your thoughts regarding needing to update instead
of add.
If there were 2..n already running with the share attribute already set,
would the reconnect threads run/finish before the new libvirtd accepts
new connections/domain starts? If so, then this code will restore life
as it was before libvirtd was stopped. The only way those domains would
have been able to start previously and share the device is if they ran
through the qemuPrepareHostdevSCSIDevices() which does check shareable.
My assumption was after some amount of code reading - those restart
threads would finish so that it's not possible for some domain to be
started that isn't sharing, but wanting to use the same device.
Since prior code doesn't allow sharing of these devices there doesn't
need to be a consideration made for an already running domain when this
code first appears. Although, you may run into situations where
qemuPrepareHostdevSCSIDevices() fails due to a running domain that was
running before the shareable attribute was known. That domain would have
to be restarted - something that could be documentable. The error
message could be more helpful indicating it's in use without the
shareable attribute by some other domain. In that case, only 1 domain
would be using it thus in theory used_by[0] would be that domain. If
there were more than 1 domain (n_used_by), then you've got other
problems in the code!
John
1:06 a.m.
New subject: [libvirt] [PATCH 2/2] qemu: Don't fail if the SCSI host device is shareable between domains
On 16/01/14 23:39, John Ferlan wrote:
On 01/16/2014 08:53 AM, Osier Yang wrote:
<...snip...>
>> It took some thinking, but I convinced myself that this path doesn't
>> need the shared check since it's only called from qemuProcessReconnect;
>> however, if something else did call it some day then that check may be
>> necessary. It may be wise to add it anyway... I have no strong opinion
>> about it being required for this change.
>>
> Missed reply for this one.
>
> Actually it needs the checking. Assuming there are 2 live domains, and
> they are using the same SCSI generic device. It's possible since the
> device could be shared among domains. And in that case, we should
> update the "dev->used_by" array instead of adding the device to the
list
> ("driver->activeScsiHostdevs") directly, during the reconnecting
sequence.
> I.E it needs to restore the internal data correctly to the state just as the
> one before libvirtd getting shutdown.
>
The activeScsiHostdevs is listed off the driver and not the domain,
Yes. It's of the driver.
so
it's not like each domain has to update which other domains is sharing.
But the list is filled while each domain is starting or *reconnecting*
Maybe I'm misreading your thoughts regarding needing to update
instead
of add.
If there were 2..n already running with the share attribute already set,
would the reconnect threads run/finish before the new libvirtd accepts
new connections/domain starts?
Yes. See daemonRunStateInit.
If so, then this code will restore life
as it was before libvirtd was stopped.
Yes. That's what this patch tries to do.
The only way those domains would
have been able to start previously and share the device is if they ran
through the qemuPrepareHostdevSCSIDevices() which does check shareable.
Yes, that's why I didn't do the checking about "shareable" in
qemuUpdateActivePciHostdevs.
My assumption was after some amount of code reading - those restart
threads would finish so that it's not possible for some domain to be
started that isn't sharing, but wanting to use the same device.
Exactly right.
Since prior code doesn't allow sharing of these devices there doesn't
need to be a consideration made for an already running domain when this
code first appears. Although, you may run into situations where
qemuPrepareHostdevSCSIDevices() fails due to a running domain that was
running before the shareable attribute was known.
Indeed, but we have no way to avoid this problem for the domains
which was running against the old libvirt, and the libvirt is upgraded
after that.
That domain would have
to be restarted - something that could be documentable. The error
message could be more helpful indicating it's in use without the
shareable attribute by some other domain.
It's more complicate than this, when a domain is trying to start,
the situations could be (with new code):
1) No domain is using the device
2) 1 domain is using the device
3) More than 1 domain are using the device.
For 1), it's just fine, let's ignore it.
For 2), the domain which is using it could use the device as either
"shareable" or "non-shareable". If it's "shareable",
the later domain
could be started successfully only if it uses the "device" as
"shareable"
too; If it's "non-shareable", the later domain can't use the device
anyway.
For 3), the domains must use the device as "shareable". And the later
domain must use the device as "shareable" too to be started successfully.
So, as you seen, we can't simply say with error "the device is in use
without shareable attribute by some other domain". We can use if...else
branches to construct the more sensible errors, but I'm doubting about
if we should make things more complicate.
Though for old code, simply saying "the device is in use without
the shareable attribute by some other domain" is fine, but if you think
about the situations in new code, it will be a mess.
In that case, only 1 domain
would be using it thus in theory used_by[0] would be that domain. If
there were more than 1 domain (n_used_by), then you've got other
problems in the code!
I guess here you still mean the domain(s) which were running with
the old code. And since with the old code, there could be only 1
domain using the same device, regardless of the device is specified
as "shareable" or not. So yet, the "n_used_by" must be 1.
As a conclusion, I think the only concern from you is about the problem
on the running domain of an old libvirt (without these 2 patches). Right?
If so, my thought is to add document somewhere, though I have not
much idea about where to put the document, and how to write the
document to explain the problem which is such complicated.
Osier
7:44 a.m.
New subject: [libvirt] [PATCH 2/2] qemu: Don't fail if the SCSI host device is shareable between domains
On 01/17/2014 02:06 AM, Osier Yang wrote:
<...snip...>
As a conclusion, I think the only concern from you is about the problem
on the running domain of an old libvirt (without these 2 patches). Right?
If so, my thought is to add document somewhere, though I have not
much idea about where to put the document, and how to write the
document to explain the problem which is such complicated.
Just trying to think and reason through the possibilities to help make
sure we're on the same page and reduce the chance for possible future
corner case issues...
With regard to the error message - just a way to indicate that the
failure was due to the device not being set shareable should be fine.
Whether that's "this" domain hasn't set it shareable or
"another" domain
hasn't set it shareable is a "nice" addition. Just saying already in use
doesn't give enough of a hint that perhaps there is a way or for what
reason we failed. Of course reading the code it's easy, but if you're a
customer without code...
Finally - I think a note in the Device section of formatdomain.html to
indicate when support was really added. Sure the tag was added in 1.0.6,
but it's not really functional until 1.2.2. Not sure how best to say
that other than perhaps changing the since value... In the same area it
should be noted that all domains using/defining the device need to have
the shareable tag; otherwise, depending on order of domain startup one
or more domains will fail to start.
John
3:38 a.m.
New subject: [libvirt] [PATCH 2/2] qemu: Don't fail if the SCSI host device is shareable between domains
On 17/01/14 21:44, John Ferlan wrote:
On 01/17/2014 02:06 AM, Osier Yang wrote:
<...snip...>
> As a conclusion, I think the only concern from you is about the problem
> on the running domain of an old libvirt (without these 2 patches). Right?
> If so, my thought is to add document somewhere, though I have not
> much idea about where to put the document, and how to write the
> document to explain the problem which is such complicated.
>
Just trying to think and reason through the possibilities to help make
sure we're on the same page and reduce the chance for possible future
corner case issues...
With regard to the error message - just a way to indicate that the
failure was due to the device not being set shareable should be fine.
Whether that's "this" domain hasn't set it shareable or
"another" domain
hasn't set it shareable is a "nice" addition. Just saying already in use
doesn't give enough of a hint that perhaps there is a way or for what
reason we failed. Of course reading the code it's easy, but if you're a
customer without code...
Finally - I think a note in the Device section of formatdomain.html to
indicate when support was really added. Sure the tag was added in 1.0.6,
but it's not really functional until 1.2.2. Not sure how best to say
that other than perhaps changing the since value... In the same area it
should be noted that all domains using/defining the device need to have
the shareable tag; otherwise, depending on order of domain startup one
or more domains will fail to start.
Agreed. formatdomain.html is a good place to do that. I will post
a v3 series including the document together.
Osier
9:05 a.m.
On 08/01/14 22:51, Osier Yang wrote:
Forgot to mention that the lacked tests for scsi utils will come
soon.
*** BLURB HERE ***
Osier Yang (2):
util: Add "shareable" field for virSCSIDevice struct
qemu: Don't fail if the SCSI host device is shareable between domains
src/libvirt_private.syms | 3 +-
src/qemu/qemu_cgroup.c | 3 +-
src/qemu/qemu_hostdev.c | 84 ++++++++++++++++++++++------------------
src/security/security_apparmor.c | 3 +-
src/security/security_dac.c | 6 ++-
src/security/security_selinux.c | 6 ++-
src/util/virscsi.c | 59 +++++++++++++++++++++++-----
src/util/virscsi.h | 11 ++++--
8 files changed, 116 insertions(+), 59 deletions(-)
3928
days inactive
3943
days old
12 comments
2 participants
participants (2)
-
John Ferlan -
Osier Yang