[libvirt] [PATCH v3] qemu: Don't fail if the SCSI host device is shareable between domains
John Ferlan
jferlan at redhat.com
Fri Jan 24 13:16:31 UTC 2014
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);
>
>
More information about the libvir-list
mailing list