[libvirt] [PATCH 0/2 v2] Fixes on shareable SCSI host device
*** 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
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
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
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
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
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);
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
[ 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
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
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
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
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
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(-)
participants (2)
-
John Ferlan -
Osier Yang