On 01/23/2014 10:04 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.

Since prior to this patch, the "shareable" tag didn't work as expected, it actually work like "non-shareable". If there was a live domain using the SCSI device with "shareable" specified, then after upgrading (assuming the live domain keep running during upgrading) the older libvirt (without this patch) to newer libvirt (with this patch), it may cause some confusion when user tries to start later domains as "shareable". So this patch also added notes in formatdomain.html to declare the fact.

* 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". And since it's only used in one place, we can safely removing the code to find out the dev in the list first. - 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; And make more sensible error w.r.t the current "shareable" value in driver->activeScsiHostdevs. - qemuDomainReAttachHostScsiDevices: Change the logic according to the changes on helpers. --- docs/formatdomain.html.in | 6 ++++ src/libvirt_private.syms | 2 +- src/qemu/qemu_hostdev.c | 77 ++++++++++++++++++++++++++--------------------- src/util/virscsi.c | 45 +++++++++++++++++++++------ src/util/virscsi.h | 7 +++-- 5 files changed, 90 insertions(+), 47 deletions(-)

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index ff50214..18bfad1 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -2798,6 +2798,12 @@ between domains (assuming the hypervisor and OS support this). Only supported by SCSI host device. <span class="since">Since 1.0.6</span> + <p> + Note: though <code>shareable</shareable> was introduced

Beyond the </shareable> => </code> note you've already made s/though/Although/

+ <span class="since">since 1.0.6</span>, but it doesn't work as

s/since 1.0.6/in 1.0.6/ s/doesn't/did not/

+ as expected (actually works like non-shareable) until

I'm 50/50 on whether the text within the parentheses is necessary...

+ <span class="since">1.2.2</span>. + </p> </dd> </dl>

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 13caf93..0f30292 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1686,7 +1686,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..2b9d274 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,29 @@ 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); + bool scsi_shareable = virSCSIDeviceGetShareable(scsi); + bool tmp_shareable = virSCSIDeviceGetShareable(tmp);

- if (other_name) - virReportError(VIR_ERR_OPERATION_INVALID, - _("SCSI device %s is in use by domain %s"), - virSCSIDeviceGetName(tmp), other_name); - else + if (!(scsi_shareable && tmp_shareable)) { virReportError(VIR_ERR_OPERATION_INVALID, - _("SCSI device %s is already in use"), + _("SCSI device %s is already in use by " + "other domain(s) as '%s'"), + tmp_shareable ? "shareable" : "non-shareable", 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 +1393,8 @@ 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; + virSCSIDevicePtr tmp; virDomainDeviceDef dev;

dev.type = VIR_DOMAIN_DEVICE_HOSTDEV; @@ -1411,30 +1424,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 (!(tmp = 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, tmp, name); + virSCSIDeviceFree(scsi); } virObjectUnlock(driver->activeScsiHostdevs); } diff --git a/src/util/virscsi.c b/src/util/virscsi.c index 58d9e25..2c6d865 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,24 @@ 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; + + for (i = 0; i < ret->n_used_by; i++) {

^^^ Coverity: Event var_deref_op: Dereferencing null pointer "ret". Which leaves me wondering about testing... Is there an automated test that could be added? FWIW: v2 had the following code which has been removed from v3: + if (!(ret = virSCSIDeviceListFind(list, dev))) + return; + John

+ 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);