Re: [libvirt] [PATCH 2/2] qemu: Don't fail if the SCSI host device is shareable between domains

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