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

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.

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

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

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

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