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

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(a)linux.vnet.ibm.com&gt; >>>> --- >>>> 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.

> >> 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(a)linux.vnet.ibm.com&gt; >>>> + */ >>>> + >>>> +#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(a)linux.vnet.ibm.com&gt; >>>> + */ >>>> + >>>> +#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, >>>>