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;
+
+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.
+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...
+
+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,