[libvirt] [PATCH 2/2] qemu: Don't fail if the SCSI host device is shareable between domains
Osier Yang
jyang at redhat.com
Thu Jan 16 13:31:53 UTC 2014
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
More information about the libvir-list
mailing list