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