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

[Osier Yang <jyang@xxxxxxxxxx>]

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

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list