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