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. 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.
> 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.
>>> 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