Re: [PATCH v3 04/10] util: Management routines for scsi_host devices

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 11/14/2016 05:06 PM, Eric Farman wrote:
> 
> 
> On 11/14/2016 04:40 PM, John Ferlan wrote:
>>
>> On 11/14/2016 03:38 PM, Eric Farman wrote:
>>>
>>> On 11/11/2016 04:44 PM, John Ferlan wrote:
>>>> While perhaps mostly obvious - you need some sort of commit message
>>>> here.
>>>>
>>>> On 11/08/2016 01:26 PM, Eric Farman wrote:
>>>>> Signed-off-by: Eric Farman <farman@xxxxxxxxxxxxxxxxxx>
>>>>> ---
>>>>>    po/POTFILES.in           |   1 +
>>>>>    src/Makefile.am          |   1 +
>>>>>    src/libvirt_private.syms |  19 +++
>>>>>    src/util/virhost.c       | 299
>>>>> +++++++++++++++++++++++++++++++++++++++++++++++
>>>>>    src/util/virhost.h       |  72 ++++++++++++
>>>>>    src/util/virhostdev.c    | 155 ++++++++++++++++++++++++
>>>>>    src/util/virhostdev.h    |  16 +++
>>>>>    tests/qemuxml2argvmock.c |   9 ++
>>>>>    8 files changed, 572 insertions(+)
>>>>>    create mode 100644 src/util/virhost.c
>>>>>    create mode 100644 src/util/virhost.h
>>>>>
>>>> I fear someone will equate "virhost" to host virtual functions as
>>>> opposed to what it is.  You'll note there's virhostcpu, virhostdev, and
>>>> virhostmem - each of which are utility API's for that subsystem.
>>>>
>>>> So I think this needs to become 'virscsihost.{c,h}' and of course the
>>>> API's are 'virSCSIHostDevice' prefixed instead of 'virHostDevice'.
>>>> Alternatively, the functions could go in virscsi.c, but I do prefer the
>>>> separation.
>>>>
>>>>> diff --git a/po/POTFILES.in b/po/POTFILES.in
>>>>> index 1469240..a7cc542 100644
>>>>> --- a/po/POTFILES.in
>>>>> +++ b/po/POTFILES.in
>>>>> @@ -199,6 +199,7 @@ src/util/virfirewall.c
>>>>>    src/util/virfirmware.c
>>>>>    src/util/virhash.c
>>>>>    src/util/virhook.c
>>>>> +src/util/virhost.c
>>>>>    src/util/virhostcpu.c
>>>>>    src/util/virhostdev.c
>>>>>    src/util/virhostmem.c
>>>>> diff --git a/src/Makefile.am b/src/Makefile.am
>>>>> index d417b6e..404c64e 100644
>>>>> --- a/src/Makefile.am
>>>>> +++ b/src/Makefile.am
>>>>> @@ -122,6 +122,7 @@ UTIL_SOURCES =                            \
>>>>>            util/virhash.c util/virhash.h            \
>>>>>            util/virhashcode.c util/virhashcode.h        \
>>>>>            util/virhook.c util/virhook.h            \
>>>>> +        util/virhost.c util/virhost.h            \
>>>>>            util/virhostcpu.c util/virhostcpu.h util/virhostcpupriv.h \
>>>>>            util/virhostdev.c util/virhostdev.h        \
>>>>>            util/virhostmem.c util/virhostmem.h        \
>>>>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>>>>> index 74dd527..ff535f9 100644
>>>>> --- a/src/libvirt_private.syms
>>>>> +++ b/src/libvirt_private.syms
>>>>> @@ -1675,6 +1675,23 @@ virHookInitialize;
>>>>>    virHookPresent;
>>>>>      +# util/virhost.h
>>>>> +virHostDeviceFileIterate;
>>>>> +virHostDeviceFree;
>>>>> +virHostDeviceGetName;
>>>>> +virHostDeviceListAdd;
>>>>> +virHostDeviceListCount;
>>>>> +virHostDeviceListDel;
>>>>> +virHostDeviceListFind;
>>>>> +virHostDeviceListFindIndex;
>>>> This one is not used externally, so it could just be static to
>>>> virhost.c
>>>>
>>>>> +virHostDeviceListGet;
>>>>> +virHostDeviceListNew;
>>>>> +virHostDeviceListSteal;
>>>>> +virHostDeviceNew;
>>>>> +virHostDeviceSetUsedBy;
>>>>> +virHostOpenVhostSCSI;
>>>>> +
>>>>> +
>>>>>    # util/virhostdev.h
>>>>>    virHostdevFindUSBDevice;
>>>>>    virHostdevManagerGetDefault;
>>>>> @@ -1682,10 +1699,12 @@ virHostdevPCINodeDeviceDetach;
>>>>>    virHostdevPCINodeDeviceReAttach;
>>>>>    virHostdevPCINodeDeviceReset;
>>>>>    virHostdevPrepareDomainDevices;
>>>>> +virHostdevPrepareHostDevices;
>>> As I alluded to in my response in patch 3, taking your s/Host/SCSIHost/
>>> comment to heart breaks down here because
>>> "virHostdevPrepareSCSIHostDevices" was introduced by commit 17bddc46.
>>
>> Ugh... So many close names is making my eyes hurt.
> 
> Mine too.  :)
> 
>>   I was afraid we may
>> end with this dilemma. Trust me I waffled a lot about saying anything on
>> this, but it eventually got to a point where there were virHostXXX API's
>> that I really had to say something.  My other thought along the way was
>> to use the "vHost" in some way (e.g. virSCSIHostvHost prefixes).
>>
>> I was hoping though that a generic solution would work, but felt it may
>> not work for everything and we could address those as we went along.
>>
>> For this particular one what you're adding I think would be:
>>
>>     virHostdevPrepareSCSIHostvHostDevices
>>
>> That would say to me as a reader that I'm modifying a scsi_host device
>> using the vhost protocol. Search around for "SCSIiSCSI" to see some
>> really ugly names.
> 
> Would "SCSIHostVHost" be acceptable?  Writing "SCSIHostvHost" looks like
> a typo to me.
> 

That's fine... It's funny reading HostvHost alone gets me to thinking of
the short hand for sports matches "verses".

John

>>
>>> So now I'm torn in how far to correlate the changes.  I currently have
>>> two (to-be-squashed) patches that does "s/virHost/virSCSIHost/" and
>>> "s/HOST/SCSIHOST/" within the context of this series, but going farther
>>> means qemuHostdevPrepareSCSIHostDevices calls
>>> virHostdevPrepareHostDevices, because qemuHostdevPrepareSCSIDevices
>>> calls virHostdevPrepareSCSIDevices, whihc may call
>>> virHostdevPrepareSCSIHostDevices
>>>
>> I recall hitting something else during review, but now I cannot remember
>> what it was... There was some name that should have use SCSI, but
>> didn't. I thought I flagged it, but I might not of. Now I cannot
>> remember what it was.  It was something I found while looking at the PCI
>> address code. <sigh> must've been interrupted and lost my train of
>> thought and didn't get back to it.
> 
> I'll keep an eye out.  As long as I'm mucking around in this code, if we
> can straighten some things out that would be a Good Thing.
> 
>  - Eric
> 
>>
>>>>>    virHostdevPreparePCIDevices;
>>>>>    virHostdevPrepareSCSIDevices;
>>>>>    virHostdevPrepareUSBDevices;
>>>>>    virHostdevReAttachDomainDevices;
>>>>> +virHostdevReAttachHostDevices;
>>>>>    virHostdevReAttachPCIDevices;
>>>>>    virHostdevReAttachSCSIDevices;
>>>>>    virHostdevReAttachUSBDevices;
>>>>> diff --git a/src/util/virhost.c b/src/util/virhost.c
>>>>> new file mode 100644
>>>>> index 0000000..9b5f524
>>>>> --- /dev/null
>>>>> +++ b/src/util/virhost.c
>>>>> @@ -0,0 +1,299 @@
>>>>> +/*
>>>>> + * virhost.c: helper APIs for managing scsi_host devices
>>>>> + *
>>>>> + * Copyright (C) 2016 IBM Corporation
>>>>> + *
>>>>> + * This library is free software; you can redistribute it and/or
>>>>> + * modify it under the terms of the GNU Lesser General Public
>>>>> + * License as published by the Free Software Foundation; either
>>>>> + * version 2.1 of the License, or (at your option) any later version.
>>>>> + *
>>>>> + * This library is distributed in the hope that it will be useful,
>>>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>>>>> + * Lesser General Public License for more details.
>>>>> + *
>>>>> + * You should have received a copy of the GNU Lesser General Public
>>>>> + * License along with this library.  If not, see
>>>>> + * <http://www.gnu.org/licenses/>.
>>>>> + *
>>>>> + * Authors:
>>>>> + *     Eric Farman <farman@xxxxxxxxxxxxxxxxxx>
>>>>> + */
>>>>> +
>>>>> +#include <config.h>
>>>>> +#include <fcntl.h>
>>>>> +
>>>>> +#include "virhost.h"
>>>>> +#include "virlog.h"
>>>>> +#include "viralloc.h"
>>>>> +#include "virerror.h"
>>>>> +#include "virfile.h"
>>>>> +#include "virstring.h"
>>>>> +
>>>>> +VIR_LOG_INIT("util.host");
>>>>> +
>>>>> +#define SYSFS_VHOST_SCSI_DEVICES "/sys/kernel/config/target/vhost/"
>>>>> +#define VHOST_SCSI_DEVICE "/dev/vhost-scsi"
>>>>> +
>>>>> +struct _virUsedByInfo {
>>>>> +    char *drvname; /* which driver */
>>>>> +    char *domname; /* which domain */
>>>>> +};
>>>>> +typedef struct _virUsedByInfo *virUsedByInfoPtr;
>>>> Hmm... seeing this makes me think the code should go in virscsi.c...
>>>> That would seem to then allow more code reuse...
>>>>
>>>> However, looking at your virHostdevPrepareHostDevices changes for how
>>>> this is implemented, I see no sharing allowed/going on. Thus,
>>>> there's no
>>>> need for a list of used_by - rather it's just a single
>>>> 'used_by_drvname'
>>>> and 'used_by_domname' in the virSCSIHostDevice structure (similar to
>>>> PCI
>>>> and USB).
>>>>
>>>> Unless of course you think you want to add sharing....  which I cannot
>>>> imagine is a good idea...
>>> No, this is a fine idea.  I cannot see a point in permitting this.
>>>
>>>>> +
>>>>> +struct _virHostDevice {
>>>> s/Host/SCSIHost/
>>>>
>>>>> +    char *name; /* naa.<wwn> */
>>>>> +    char *path;
>>>>> +    virUsedByInfoPtr *used_by; /* driver:domain(s) using this dev */
>>>>> +    size_t n_used_by; /* how many domains are using this dev */
>>>> Looks like the above 2 get replaced by
>>>>
>>>>       char *used_by_drvname;
>>>>       char *used_by_domname;
>>>>
>>>> Also - looking at QEMU and "forward" thinking - there are options on
>>>> the
>>>> qemu command line for things like boot_tpgt, num_queues, max_sectors,
>>>> cmd_per_lun, and bootindex... I can see the last one being asked for -
>>>> as in can we pass one of these to the guest to allow booting from a
>>>> specific LUN on the array...
>>>>
>>>>> +};
>>>>> +
>>>>> +struct _virHostDeviceList {
>>>> s/Host/SCSIHost/
>>>>
>>>>> +    virObjectLockable parent;
>>>>> +    size_t count;
>>>>> +    virHostDevicePtr *devs;
>>>>> +};
>>>>> +
>>>>> +static virClassPtr virHostDeviceListClass;
>>>> s/Host/SCSIHost/
>>>>
>>>> (ad nauseum ;-))
>>>>
>>>>> +
>>>>> +static void
>>>>> +virHostDeviceListDispose(void *obj)
>>>>> +{
>>>>> +    virHostDeviceListPtr list = obj;
>>>>> +    size_t i;
>>>>> +
>>>>> +    for (i = 0; i < list->count; i++)
>>>>> +        virHostDeviceFree(list->devs[i]);
>>>>> +
>>>>> +    VIR_FREE(list->devs);
>>>>> +}
>>>>> +
>>>>> +
>>>> You start w/ the newer convention of 2 spaces between functions, but it
>>>> ends here. After this there's always just 1 space between functions -
>>>> let's try to go w/ 2.
>>> OK.
>>>
>>>>> +static int
>>>>> +virHostOnceInit(void)
>>>>> +{
>>>>> +    if (!(virHostDeviceListClass =
>>>>> virClassNew(virClassForObjectLockable(),
>>>>> +                                               "virHostDeviceList",
>>>>> +
>>>>> sizeof(virHostDeviceList),
>>>>> +
>>>>> virHostDeviceListDispose)))
>>>>> +        return -1;
>>>>> +
>>>>> +    return 0;
>>>>> +}
>>>>> +
>>>>> +VIR_ONCE_GLOBAL_INIT(virHost)
>>>>> +
>>>>> +/* For virReportOOMError()  and virReportSystemError() */
>>>>> +#define VIR_FROM_THIS VIR_FROM_NONE
>>>>> +
>>>>> +int
>>>>> +virHostOpenVhostSCSI(int *vhostfd)
>>>>> +{
>>>>> +    if (!virFileExists(VHOST_SCSI_DEVICE))
>>>>> +        goto error;
>>>>> +
>>>>> +    *vhostfd = open(VHOST_SCSI_DEVICE, O_RDWR);
>>>>> +
>>>>> +    if (*vhostfd < 0) {
>>>>> +        virReportSystemError(errno, _("Failed to open %s"),
>>>>> VHOST_SCSI_DEVICE);
>>>>> +        goto error;
>>>>> +    }
>>>>> +
>>>>> +    return 0;
>>>>> +
>>>>> + error:
>>>>> +    VIR_FORCE_CLOSE(*vhostfd);
>>>>> +
>>>>> +    return -1;
>>>>> +}
>>>>> +
>>>>> +static void
>>>>> +virHostDeviceUsedByInfoFree(virUsedByInfoPtr used_by)
>>>>> +{
>>>> I think this ends up going away since things aren't shareable.
>>>>
>>>>> +    VIR_FREE(used_by->drvname);
>>>>> +    VIR_FREE(used_by->domname);
>>>>> +    VIR_FREE(used_by);
>>>>> +}
>>>>> +
>>>>> +void
>>>>> +virHostDeviceListDel(virHostDeviceListPtr list,
>>>>> +                     virHostDevicePtr dev,
>>>>> +                     const char *drvname,
>>>>> +                     const char *domname)
>>>>> +{
>>>>> +    virHostDevicePtr tmp = NULL;
>>>>> +    size_t i;
>>>>> +
>>>> This will need to be adjusted since (I assume) you don't want shareable
>>>> scsi_host devices
>>>>
>>>>> +    for (i = 0; i < dev->n_used_by; i++) {
>>>>> +        if (STREQ_NULLABLE(dev->used_by[i]->drvname, drvname) &&
>>>>> +            STREQ_NULLABLE(dev->used_by[i]->domname, domname)) {
>>>>> +            if (dev->n_used_by > 1) {
>>>>> +                virHostDeviceUsedByInfoFree(dev->used_by[i]);
>>>>> +                VIR_DELETE_ELEMENT(dev->used_by, i, dev->n_used_by);
>>>>> +            } else {
>>>>> +                tmp = virHostDeviceListSteal(list, dev);
>>>>> +                virHostDeviceFree(tmp);
>>>>> +            }
>>>>> +            break;
>>>>> +        }
>>>>> +    }
>>>>> +}
>>>>> +
>>>>> +int
>>>> s/int/static int/
>>>>
>>>>> +virHostDeviceListFindIndex(virHostDeviceListPtr list,
>>>>> virHostDevicePtr dev)
>>>>> +{
>>>>> +    size_t i;
>>>>> +
>>>>> +    for (i = 0; i < list->count; i++) {
>>>>> +        virHostDevicePtr other = list->devs[i];
>>>>> +        if (STREQ_NULLABLE(other->name, dev->name))
>>>>> +            return i;
>>>>> +    }
>>>>> +    return -1;
>>>>> +}
>>>>> +
>>>>> +virHostDevicePtr
>>>>> +virHostDeviceListGet(virHostDeviceListPtr list, int idx)
>>>>> +{
>>>>> +    if (idx >= list->count || idx < 0)
>>>>> +        return NULL;
>>>>> +
>>>>> +    return list->devs[idx];
>>>>> +}
>>>>> +
>>>>> +size_t
>>>>> +virHostDeviceListCount(virHostDeviceListPtr list)
>>>>> +{
>>>>> +    return list->count;
>>>>> +}
>>>>> +
>>>>> +virHostDevicePtr
>>>>> +virHostDeviceListSteal(virHostDeviceListPtr list,
>>>>> +                       virHostDevicePtr dev)
>>>>> +{
>>>>> +    virHostDevicePtr ret = NULL;
>>>>> +    size_t i;
>>>>> +
>>>>> +    for (i = 0; i < list->count; i++) {
>>>>> +        if (STREQ_NULLABLE(list->devs[i]->name, dev->name)) {
>>>>> +            ret = list->devs[i];
>>>>> +            VIR_DELETE_ELEMENT(list->devs, i, list->count);
>>>>> +            break;
>>>>> +        }
>>>>> +    }
>>>>> +
>>>>> +    return ret;
>>>>> +}
>>>>> +
>>>>> +virHostDevicePtr
>>>>> +virHostDeviceListFind(virHostDeviceListPtr list, virHostDevicePtr
>>>>> dev)
>>>>> +{
>>>>> +    int idx;
>>>>> +
>>>>> +    if ((idx = virHostDeviceListFindIndex(list, dev)) >= 0)
>>>>> +        return list->devs[idx];
>>>>> +    else
>>>>> +        return NULL;
>>>>> +}
>>>>> +
>>>>> +int
>>>>> +virHostDeviceListAdd(virHostDeviceListPtr list,
>>>>> +                     virHostDevicePtr dev)
>>>>> +{
>>>>> +    if (virHostDeviceListFind(list, dev)) {
>>>>> +        virReportError(VIR_ERR_INTERNAL_ERROR,
>>>>> +                       _("Device %s is already in use"), dev->name);
>>>>> +        return -1;
>>>>> +    }
>>>>> +    return VIR_APPEND_ELEMENT(list->devs, list->count, dev);
>>>>> +}
>>>>> +
>>>>> +virHostDeviceListPtr
>>>>> +virHostDeviceListNew(void)
>>>>> +{
>>>>> +    virHostDeviceListPtr list;
>>>>> +
>>>>> +    if (virHostInitialize() < 0)
>>>>> +        return NULL;
>>>>> +
>>>>> +    if (!(list = virObjectLockableNew(virHostDeviceListClass)))
>>>>> +        return NULL;
>>>>> +
>>>>> +    return list;
>>>> or simply return virObjectLockableNew(virSCSIHostDeviceListClass); and
>>>> then 'list' is not needed.
>>>>
>>>>> +}
>>>>> +
>>>>> +int
>>>>> +virHostDeviceSetUsedBy(virHostDevicePtr dev,
>>>>> +                       const char *drvname,
>>>>> +                       const char *domname)
>>>>> +{
>>>> Without sharing, this mimics PCI/USB instead...
>>>>
>>>>> +    virUsedByInfoPtr copy;
>>>>> +    if (VIR_ALLOC(copy) < 0)
>>>>> +        return -1;
>>>>> +    if (VIR_STRDUP(copy->drvname, drvname) < 0 ||
>>>>> +        VIR_STRDUP(copy->domname, domname) < 0)
>>>>> +        goto cleanup;
>>>>> +
>>>>> +    if (VIR_APPEND_ELEMENT(dev->used_by, dev->n_used_by, copy) < 0)
>>>>> +        goto cleanup;
>>>>> +
>>>>> +    return 0;
>>>>> +
>>>>> + cleanup:
>>>>> +    virHostDeviceUsedByInfoFree(copy);
>>>>> +    return -1;
>>>>> +}
>>>>> +
>>>>> +int
>>>>> +virHostDeviceFileIterate(virHostDevicePtr dev,
>>>>> +                         virHostDeviceFileActor actor,
>>>>> +                         void *opaque)
>>>>> +{
>>>>> +    return (actor)(dev, dev->path, opaque);
>>>>> +}
>>>>> +
>>>>> +const char *
>>>>> +virHostDeviceGetName(virHostDevicePtr dev)
>>>>> +{
>>>>> +    return dev->name;
>>>>> +}
>>>>> +
>>>>> +virHostDevicePtr
>>>>> +virHostDeviceNew(const char *name)
>>>>> +{
>>>>> +    virHostDevicePtr dev;
>>>>> +
>>>>> +    if (VIR_ALLOC(dev) < 0)
>>>>> +        return NULL;
>>>>> +
>>>>> +    if (VIR_STRDUP(dev->name, name) < 0) {
>>>>> +        virReportError(VIR_ERR_INTERNAL_ERROR,
>>>>> +                       _("dev->name buffer overflow: %s"),
>>>>> +                       name);
>>>>> +        goto error;
>>>>> +    }
>>>>> +
>>>>> +    if (virAsprintf(&dev->path, "%s/%s",
>>>>> +                    SYSFS_VHOST_SCSI_DEVICES, name) < 0)
>>>>> +        goto cleanup;
>>>>> +
>>>>> +    VIR_DEBUG("%s: initialized", dev->name);
>>>>> +
>>>>> + cleanup:
>>>>> +    return dev;
>>>>> +
>>>>> + error:
>>>>> +    virHostDeviceFree(dev);
>>>>> +    dev = NULL;
>>>>> +    goto cleanup;
>>>>> +}
>>>>> +
>>>>> +void
>>>>> +virHostDeviceFree(virHostDevicePtr dev)
>>>>> +{
>>>>       size_t i;
>>>>
>>>>> +    if (!dev)
>>>>> +        return;
>>>>> +    VIR_DEBUG("%s: freeing", dev->name);
>>>> Would be nice if it was freeing everything in 'dev' before freeing
>>>> dev...
>>> Oops!
>>>
>>>> Thus you have:
>>>>
>>>>       VIR_FREE(dev->name);
>>>>       VIR_FREE(dev->path);
>>>>       VIR_FREE(dev->used_by_drvname);
>>>>       VIR_FREE(dev->used_by_domname);
>>>>
>>>>> +    VIR_FREE(dev);
>>>>> +}
>>>>> diff --git a/src/util/virhost.h b/src/util/virhost.h
>>>>> new file mode 100644
>>>>> index 0000000..6d7b790
>>>>> --- /dev/null
>>>>> +++ b/src/util/virhost.h
>>>>> @@ -0,0 +1,72 @@
>>>>> +/*
>>>>> + * virhost.h: helper APIs for managing host scsi_host devices
>>>>> + *
>>>>> + * Copyright (C) 2016 IBM Corporation
>>>>> + *
>>>>> + * This library is free software; you can redistribute it and/or
>>>>> + * modify it under the terms of the GNU Lesser General Public
>>>>> + * License as published by the Free Software Foundation; either
>>>>> + * version 2.1 of the License, or (at your option) any later version.
>>>>> + *
>>>>> + * This library is distributed in the hope that it will be useful,
>>>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>>>>> + * Lesser General Public License for more details.
>>>>> + *
>>>>> + * You should have received a copy of the GNU Lesser General Public
>>>>> + * License along with this library.  If not, see
>>>>> + * <http://www.gnu.org/licenses/>.
>>>>> + *
>>>>> + * Authors:
>>>>> + *     Eric Farman <farman@xxxxxxxxxxxxxxxxxx>
>>>>> + */
>>>>> +
>>>>> +#ifndef __VIR_HOST_H__
>>>>> +# define __VIR_HOST_H__
>>>>> +
>>>>> +# include "internal.h"
>>>>> +# include "virobject.h"
>>>>> +# include "virutil.h"
>>>>> +
>>>>> +typedef struct _virHostDevice virHostDevice;
>>>>> +typedef virHostDevice *virHostDevicePtr;
>>>>> +typedef struct _virHostDeviceAddress virHostDeviceAddress;
>>>>> +typedef virHostDeviceAddress *virHostDeviceAddressPtr;
>>>>> +typedef struct _virHostDeviceList virHostDeviceList;
>>>>> +typedef virHostDeviceList *virHostDeviceListPtr;
>>>>> +
>>>>> +struct _virHostDeviceAddress {
>>>>> +    char *wwpn;
>>>>> +};
>>>> Hmmm.. this would seemingly duplicate name without the "naa.", but in
>>>> the long run it's not even used - so why is it necessary?
>>>>
>>>> Then again as it turns out a "vhost-scsi-pci" or "vhost-scsi-ccw"
>>>> device
>>>> object is created. Each of those would seemingly need a controller to
>>>> use wouldn't they?
>>> Are you referring to a <controller> tag?  Or something else?
>>>
>> Um.. oh yeah. I think at this point it wasn't totally clear what address
>> was going to be used in the guest.  This is probably where I started
>> down the rabbit hole of figuring out how PCI addresses would be added to
>> the XML.
>>
>>>>    So the address would seem to be important, but is not
>>>> handled.
>>> Will check how the PCI handling is broken.  Seemed okay for CCW, but
>>> maybe I've missed something there too.
>>>
>> I think you're letting QEMU pick which may not be a good idea based on
>> our recent experience.
>>
>> John
>>>   - Eric
>>>
>>>>> +
>>>>> +typedef int (*virHostDeviceFileActor)(virHostDevicePtr dev,
>>>>> +                                      const char *name, void
>>>>> *opaque);
>>>>> +
>>>>> +int virHostDeviceFileIterate(virHostDevicePtr dev,
>>>>> +                             virHostDeviceFileActor actor,
>>>>> +                             void *opaque);
>>>>> +const char *virHostDeviceGetName(virHostDevicePtr dev);
>>>>> +virHostDevicePtr virHostDeviceListGet(virHostDeviceListPtr list,
>>>>> +                                      int idx);
>>>>> +size_t virHostDeviceListCount(virHostDeviceListPtr list);
>>>>> +virHostDevicePtr virHostDeviceListSteal(virHostDeviceListPtr list,
>>>>> +                                        virHostDevicePtr dev);
>>>>> +int virHostDeviceListFindIndex(virHostDeviceListPtr list,
>>>>> +                               virHostDevicePtr dev);
>>>> Remove the def since it's local to virhost.c
>>>>
>>>>> +virHostDevicePtr virHostDeviceListFind(virHostDeviceListPtr list,
>>>>> +                                       virHostDevicePtr dev);
>>>>> +int  virHostDeviceListAdd(virHostDeviceListPtr list,
>>>>> +                          virHostDevicePtr dev);
>>>>> +void virHostDeviceListDel(virHostDeviceListPtr list,
>>>>> +                          virHostDevicePtr dev,
>>>>> +                          const char *drvname,
>>>>> +                          const char *domname);
>>>>> +virHostDeviceListPtr virHostDeviceListNew(void);
>>>>> +virHostDevicePtr virHostDeviceNew(const char *name);
>>>>> +int virHostDeviceSetUsedBy(virHostDevicePtr dev,
>>>>> +                           const char *drvname,
>>>>> +                           const char *domname);
>>>>> +void virHostDeviceFree(virHostDevicePtr dev);
>>>>> +int virHostOpenVhostSCSI(int *vhostfd);
>>>>> +
>>>>> +#endif /* __VIR_HOST_H__ */
>>>>> diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c
>>>>> index 9c2262e..b92e246 100644
>>>>> --- a/src/util/virhostdev.c
>>>>> +++ b/src/util/virhostdev.c
>>>>> @@ -146,6 +146,7 @@ virHostdevManagerDispose(void *obj)
>>>>>        virObjectUnref(hostdevMgr->inactivePCIHostdevs);
>>>>>        virObjectUnref(hostdevMgr->activeUSBHostdevs);
>>>>>        virObjectUnref(hostdevMgr->activeSCSIHostdevs);
>>>>> +    virObjectUnref(hostdevMgr->activeHostHostdevs);
>>>>>        VIR_FREE(hostdevMgr->stateDir);
>>>>>    }
>>>>>    @@ -170,6 +171,9 @@ virHostdevManagerNew(void)
>>>>>        if (!(hostdevMgr->activeSCSIHostdevs = virSCSIDeviceListNew()))
>>>>>            goto error;
>>>>>    +    if (!(hostdevMgr->activeHostHostdevs =
>>>>> virHostDeviceListNew()))
>>>>> +        goto error;
>>>>> +
>>>>>        if (privileged) {
>>>>>            if (VIR_STRDUP(hostdevMgr->stateDir, HOSTDEV_STATE_DIR)
>>>>> < 0)
>>>>>                goto error;
>>>>> @@ -1472,6 +1476,102 @@
>>>>> virHostdevPrepareSCSIDevices(virHostdevManagerPtr mgr,
>>>>>        return -1;
>>>>>    }
>>>>>    +int
>>>>> +virHostdevPrepareHostDevices(virHostdevManagerPtr mgr,
>>>>> +                             const char *drv_name,
>>>>> +                             const char *dom_name,
>>>>> +                             virDomainHostdevDefPtr *hostdevs,
>>>>> +                             int nhostdevs)
>>>>> +{
>>>>> +    size_t i, j;
>>>>> +    int count;
>>>>> +    virHostDeviceListPtr list;
>>>>> +    virHostDevicePtr tmp;
>>>>> +
>>>>> +    if (!nhostdevs)
>>>>> +        return 0;
>>>>> +
>>>>> +    /* To prevent situation where scsi_host device is assigned to
>>>>> two domains
>>>>> +     * we need to keep a list of currently assigned scsi_host
>>>>> devices.
>>>>> +     * This is done in several loops which cannot be joined into one
>>>>> big
>>>>> +     * loop. See virHostdevPreparePCIDevices()
>>>>> +     */
>>>>> +    if (!(list = virHostDeviceListNew()))
>>>>> +        goto cleanup;
>>>>> +
>>>>> +    /* Loop 1: build temporary list */
>>>>> +    for (i = 0; i < nhostdevs; i++) {
>>>>> +        virDomainHostdevDefPtr hostdev = hostdevs[i];
>>>>> +        virDomainHostdevSubsysHostPtr hostsrc =
>>>>> &hostdev->source.subsys.u.host;
>>>>> +
>>>>> +        if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS ||
>>>>> +            hostdev->source.subsys.type !=
>>>>> VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_HOST)
>>>>> +            continue;
>>>>> +
>>>>> +        if (hostsrc->protocol !=
>>>>> VIR_DOMAIN_HOSTDEV_SUBSYS_HOST_PROTOCOL_TYPE_VHOST) {
>>>>> +            continue;  /* Not supported */
>>>>> +        } else {
>>>> The else would be unnecessary - just move the virSCSIHostDevicePtr
>>>> up to
>>>> the beginning of the for loop.
>>>>
>>>>> +            virHostDevicePtr host;
>>>>> +            if (!(host = virHostDeviceNew(hostsrc->wwpn)))
>>>>> +                goto cleanup;
>>>>> +
>>>>> +            if (virHostDeviceListAdd(list, host) < 0) {
>>>>> +                virHostDeviceFree(host);
>>>>> +                goto cleanup;
>>>>> +            }
>>>>> +        }
>>>>> +    }
>>>>> +
>>>>> +    /* Loop 2: Mark devices in temporary list as used by @name
>>>>> +     * and add them to driver list. However, if something goes
>>>>> +     * wrong, perform rollback.
>>>>> +     */
>>>>> +    virObjectLock(mgr->activeHostHostdevs);
>>>>> +    count = virHostDeviceListCount(list);
>>>>> +
>>>>> +    for (i = 0; i < count; i++) {
>>>>> +        virHostDevicePtr host = virHostDeviceListGet(list, i);
>>>> See this code doesn't do any sort of sharing - so no need for a list,
>>>> just a single entry...
>>>>
>>>>> +        if ((tmp = virHostDeviceListFind(mgr->activeHostHostdevs,
>>>>> host))) {
>>>>> +            virReportError(VIR_ERR_OPERATION_INVALID,
>>>>> +                           _("SCSI_host device %s is already in use
>>>>> by "
>>>>> +                             "another domain"),
>>>>> +                           virHostDeviceGetName(tmp));
>>>>> +            goto error;
>>>>> +        } else {
>>>>> +            if (virHostDeviceSetUsedBy(host, drv_name, dom_name) < 0)
>>>>> +                goto error;
>>>>> +
>>>>> +            VIR_DEBUG("Adding %s to activeHostHostdevs",
>>>>> virHostDeviceGetName(host));
>>>>> +
>>>>> +            if (virHostDeviceListAdd(mgr->activeHostHostdevs, host)
>>>>> < 0)
>>>>> +                goto error;
>>>>> +        }
>>>>> +    }
>>>>> +
>>>>> +    virObjectUnlock(mgr->activeHostHostdevs);
>>>>> +
>>>>> +    /* Loop 3: Temporary list was successfully merged with
>>>>> +     * driver list, so steal all items to avoid freeing them
>>>>> +     * when freeing temporary list.
>>>>> +     */
>>>>> +    while (virHostDeviceListCount(list) > 0) {
>>>>> +        tmp = virHostDeviceListGet(list, 0);
>>>>> +        virHostDeviceListSteal(list, tmp);
>>>>> +    }
>>>>> +
>>>>> +    virObjectUnref(list);
>>>>> +    return 0;
>>>>> + error:
>>>>> +    for (j = 0; j < i; j++) {
>>>>> +        tmp = virHostDeviceListGet(list, i);
>>>>> +        virHostDeviceListSteal(mgr->activeHostHostdevs, tmp);
>>>>> +    }
>>>>> +    virObjectUnlock(mgr->activeHostHostdevs);
>>>>> + cleanup:
>>>>> +    virObjectUnref(list);
>>>>> +    return -1;
>>>>> +}
>>>>> +
>>>>>    void
>>>>>    virHostdevReAttachUSBDevices(virHostdevManagerPtr mgr,
>>>>>                                 const char *drv_name,
>>>>> @@ -1604,6 +1704,61 @@
>>>>> virHostdevReAttachSCSIDevices(virHostdevManagerPtr mgr,
>>>>>        virObjectUnlock(mgr->activeSCSIHostdevs);
>>>>>    }
>>>>>    +void
>>>>> +virHostdevReAttachHostDevices(virHostdevManagerPtr mgr,
>>>>> +                              const char *drv_name,
>>>>> +                              const char *dom_name,
>>>>> +                              virDomainHostdevDefPtr *hostdevs,
>>>>> +                              int nhostdevs)
>>>>> +{
>>>>> +    size_t i;
>>>>> +    virHostDevicePtr host, tmp;
>>>>> +
>>>>> +
>>>>> +    if (!nhostdevs)
>>>>> +        return;
>>>>> +
>>>>> +    virObjectLock(mgr->activeHostHostdevs);
>>>>> +    for (i = 0; i < nhostdevs; i++) {
>>>>> +        virDomainHostdevDefPtr hostdev = hostdevs[i];
>>>>> +        virDomainHostdevSubsysHostPtr hostsrc =
>>>>> &hostdev->source.subsys.u.host;
>>>>> +
>>>>> +        if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS ||
>>>>> +            hostdev->source.subsys.type !=
>>>>> VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_HOST)
>>>>> +            continue;
>>>>> +
>>>>> +        if (hostsrc->protocol !=
>>>>> VIR_DOMAIN_HOSTDEV_SUBSYS_HOST_PROTOCOL_TYPE_VHOST)
>>>>> +            continue; /* Not supported */
>>>>> +
>>>>> +        if (!(host = virHostDeviceNew(hostsrc->wwpn))) {
>>>>> +            VIR_WARN("Unable to reattach SCSI_host device %s on
>>>>> domain %s",
>>>>> +                     hostsrc->wwpn, dom_name);
>>>>> +            virObjectUnlock(mgr->activeHostHostdevs);
>>>>> +            return;
>>>>> +        }
>>>>> +
>>>> I assume the following changes to match PCI/USB more closely since no
>>>> sharing is allowed.
>>>>
>>>>> +        /* Only delete the devices which are marked as being used by
>>>>> @name,
>>>>> +         * because qemuProcessStart could fail half way through. */
>>>>> +
>>>>> +        if (!(tmp = virHostDeviceListFind(mgr->activeHostHostdevs,
>>>>> host))) {
>>>>> +            VIR_WARN("Unable to find device %s "
>>>>> +                     "in list of active SCSI_host devices",
>>>>> +                     hostsrc->wwpn);
>>>>> +            virHostDeviceFree(host);
>>>>> +            virObjectUnlock(mgr->activeHostHostdevs);
>>>>> +            return;
>>>>> +        }
>>>>> +
>>>>> +        VIR_DEBUG("Removing %s dom=%s from activeHostHostdevs",
>>>>> +                   hostsrc->wwpn, dom_name);
>>>>> +
>>>>> +        virHostDeviceListDel(mgr->activeHostHostdevs, tmp,
>>>>> +                             drv_name, dom_name);
>>>>> +        virHostDeviceFree(host);
>>>>> +    }
>>>>> +    virObjectUnlock(mgr->activeHostHostdevs);
>>>>> +}
>>>>> +
>>>>>    int
>>>>>    virHostdevPCINodeDeviceDetach(virHostdevManagerPtr mgr,
>>>>>                                  virPCIDevicePtr pci)
>>>>> diff --git a/src/util/virhostdev.h b/src/util/virhostdev.h
>>>>> index f2f51bd..19cef7e 100644
>>>>> --- a/src/util/virhostdev.h
>>>>> +++ b/src/util/virhostdev.h
>>>>> @@ -30,6 +30,7 @@
>>>>>    # include "virpci.h"
>>>>>    # include "virusb.h"
>>>>>    # include "virscsi.h"
>>>>> +# include "virhost.h"
>>>>>    # include "domain_conf.h"
>>>>>      typedef enum {
>>>>> @@ -53,6 +54,7 @@ struct _virHostdevManager {
>>>>>        virPCIDeviceListPtr inactivePCIHostdevs;
>>>>>        virUSBDeviceListPtr activeUSBHostdevs;
>>>>>        virSCSIDeviceListPtr activeSCSIHostdevs;
>>>>> +    virHostDeviceListPtr activeHostHostdevs;
>>>> s/activeHostHostdevs/activeSCSIHostHostdevs/
>>>>
>>>> John
>>>>
>>>>>    };
>>>>>      virHostdevManagerPtr virHostdevManagerGetDefault(void);
>>>>> @@ -87,6 +89,13 @@ virHostdevPrepareSCSIDevices(virHostdevManagerPtr
>>>>> hostdev_mgr,
>>>>>                                 virDomainHostdevDefPtr *hostdevs,
>>>>>                                 int nhostdevs)
>>>>>        ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3);
>>>>> +int
>>>>> +virHostdevPrepareHostDevices(virHostdevManagerPtr hostdev_mgr,
>>>>> +                             const char *drv_name,
>>>>> +                             const char *dom_name,
>>>>> +                             virDomainHostdevDefPtr *hostdevs,
>>>>> +                             int nhostdevs)
>>>>> +    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3);
>>>>>    void
>>>>>    virHostdevReAttachPCIDevices(virHostdevManagerPtr hostdev_mgr,
>>>>>                                 const char *drv_name,
>>>>> @@ -109,6 +118,13 @@
>>>>> virHostdevReAttachSCSIDevices(virHostdevManagerPtr hostdev_mgr,
>>>>>                                  virDomainHostdevDefPtr *hostdevs,
>>>>>                                  int nhostdevs)
>>>>>        ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3);
>>>>> +void
>>>>> +virHostdevReAttachHostDevices(virHostdevManagerPtr hostdev_mgr,
>>>>> +                              const char *drv_name,
>>>>> +                              const char *dom_name,
>>>>> +                              virDomainHostdevDefPtr *hostdevs,
>>>>> +                              int nhostdevs)
>>>>> +    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3);
>>>>>    int
>>>>>    virHostdevUpdateActivePCIDevices(virHostdevManagerPtr mgr,
>>>>>                                     virDomainHostdevDefPtr *hostdevs,
>>>>> diff --git a/tests/qemuxml2argvmock.c b/tests/qemuxml2argvmock.c
>>>>> index 78a224b..2c85140 100644
>>>>> --- a/tests/qemuxml2argvmock.c
>>>>> +++ b/tests/qemuxml2argvmock.c
>>>>> @@ -24,6 +24,7 @@
>>>>>    #include "viralloc.h"
>>>>>    #include "vircommand.h"
>>>>>    #include "vircrypto.h"
>>>>> +#include "virhost.h"
>>>>>    #include "virmock.h"
>>>>>    #include "virnetdev.h"
>>>>>    #include "virnetdevip.h"
>>>>> @@ -107,6 +108,14 @@ virSCSIDeviceGetSgName(const char *sysfs_prefix
>>>>> ATTRIBUTE_UNUSED,
>>>>>    }
>>>>>      int
>>>>> +virHostOpenVhostSCSI(int *vhostfd)
>>>>> +{
>>>>> +    *vhostfd = STDERR_FILENO + 1;
>>>>> +
>>>>> +    return 0;
>>>>> +}
>>>>> +
>>>>> +int
>>>>>    virNetDevTapCreate(char **ifname,
>>>>>                       const char *tunpath ATTRIBUTE_UNUSED,
>>>>>                       int *tapfd,
>>>>>
> 

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



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]