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);
>
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list