[libvirt] [PATCH 3/4] vbox: change API (un)initialization logic.
Dawid Zamirski
dzamirski at datto.com
Wed Sep 28 17:41:35 UTC 2016
Since VBOX API initialization method (pfnComInitialize) is not
thread-safe and must be called from the primary thread, calling it in
every vboxConnectOpen (as we used to do) leads to segmentation
faults when multiple connections are in use. Therefore the initalization
and unitialization logic has been changed as the following:
* _registerGlobalData allocates g_pVBoxGlobalData (only when not
already allocated) and calls VBOX's pfnComInitialize to grab
references to ISession and IVirtualBox objects. This ensures that
pfnComInitialize is called when the first connection is established.
* _pfnInitialize sets up the virConnectPtr->privateData (aka
vboxPrivateData) for each connection by copying references to
ISession and IVirtualBox from g_pVBoxGlobalData so that the rest of
the driver API can use it without referencing the global. Each time
this happens, a conntionCount is incremented on g_pVBoxGlobalData to
keep track of when it's safe to call pfnComUnitialize. One of the
reasons for existence of per-connection vboxPrivateData rather than
completely relying on vboxGlobalData, is that more modern VBOX APIs
provide pfnClientInitialize (since 4.2.20 and 4.3.4) and
pfnClientThreadInitialize (5.0+) that allow to create multiple
instances of ISession so if the code ever gets ported to support
those newer APIs it should create much less diff noise as all API
implementations are already updated to assume per-connection
ISession/IVirutalBox instances.
* _pfnUnitilalize decrements connectionCount in g_pVBoxGlobalData and
once it is down to 0, it calls pfnComUnitialize and
g_pVBoxGlobalData if free'd. This ensures that pfnComInitialize and
pfnComUnitialize pair is called only once, even when multiple
concurrent connections are in use.
---
src/vbox/vbox_common.c | 32 ++++--------
src/vbox/vbox_tmpl.c | 113 ++++++++++++++++++++++++++++--------------
src/vbox/vbox_uniformed_api.h | 57 +++++++--------------
3 files changed, 106 insertions(+), 96 deletions(-)
diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
index be6ff2d..1728275 100644
--- a/src/vbox/vbox_common.c
+++ b/src/vbox/vbox_common.c
@@ -294,18 +294,6 @@ static int vboxInitialize(vboxPrivate *data)
if (gVBoxAPI.domainEventCallbacks && gVBoxAPI.initializeDomainEvent(data) != 0)
goto cleanup;
- if (data->vboxObj == NULL) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("IVirtualBox object is null"));
- goto cleanup;
- }
-
- if (data->vboxSession == NULL) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("ISession object is null"));
- goto cleanup;
- }
-
return 0;
cleanup:
@@ -382,12 +370,12 @@ static void vboxUninitialize(vboxPrivate *data)
if (!data)
return;
- gVBoxAPI.UPFN.Uninitialize(data);
-
- virObjectUnref(data->caps);
virObjectUnref(data->xmlopt);
if (gVBoxAPI.domainEventCallbacks)
virObjectEventStateFree(data->domainEvents);
+
+ gVBoxAPI.UPFN.Uninitialize(data);
+
VIR_FREE(data);
}
@@ -398,6 +386,8 @@ vboxConnectOpen(virConnectPtr conn,
unsigned int flags)
{
vboxPrivate *data = NULL;
+ vboxGlobalData *globalData = NULL;
+
uid_t uid = geteuid();
virCheckFlags(VIR_CONNECT_RO, VIR_DRV_OPEN_ERROR);
@@ -438,8 +428,12 @@ vboxConnectOpen(virConnectPtr conn,
if (VIR_ALLOC(data) < 0)
return VIR_DRV_OPEN_ERROR;
- if (!(data->caps = vboxCapsInit()) ||
- vboxInitialize(data) < 0 ||
+ globalData = gVBoxAPI.registerGlobalData();
+
+ if (!globalData || !(globalData->caps = vboxCapsInit()))
+ return VIR_DRV_OPEN_ERROR;
+
+ if (vboxInitialize(data) < 0 ||
vboxExtractVersion(data) < 0 ||
!(data->xmlopt = vboxXMLConfInit())) {
vboxUninitialize(data);
@@ -451,12 +445,8 @@ vboxConnectOpen(virConnectPtr conn,
vboxUninitialize(data);
return VIR_DRV_OPEN_ERROR;
}
-
- data->conn = conn;
}
- if (gVBoxAPI.hasStaticGlobalData)
- gVBoxAPI.registerGlobalData(data);
conn->privateData = data;
VIR_DEBUG("in vboxOpen");
diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c
index 13869eb..37dcd3e 100644
--- a/src/vbox/vbox_tmpl.c
+++ b/src/vbox/vbox_tmpl.c
@@ -102,6 +102,8 @@ typedef IMedium IHardDisk;
VIR_LOG_INIT("vbox.vbox_tmpl");
+static vboxGlobalData *g_pVBoxGlobalData;
+
#define vboxUnsupported() \
VIR_WARN("No %s in current vbox version %d.", __FUNCTION__, VBOX_API_VERSION);
@@ -167,22 +169,6 @@ if (strUtf16) {\
(unsigned)(iid)->m3[7]);\
}\
-#if VBOX_API_VERSION > 2002000
-
-/* g_pVBoxGlobalData has to be global variable,
- * there is no other way to make the callbacks
- * work other then having g_pVBoxGlobalData as
- * global, because the functions namely AddRef,
- * Release, etc consider it as global and you
- * can't change the function definition as it
- * is XPCOM nsISupport::* function and it expects
- * them that way
- */
-
-static vboxGlobalData *g_pVBoxGlobalData;
-
-#endif /* !(VBOX_API_VERSION == 2002000) */
-
#if VBOX_API_VERSION < 4000000
# define VBOX_SESSION_OPEN(/* in */ iid_value, /* unused */ machine) \
@@ -1976,19 +1962,6 @@ _registerDomainEvent(virHypervisorDriverPtr driver)
#endif /* !(VBOX_API_VERSION == 2002000 || VBOX_API_VERSION >= 4000000) */
-static int _pfnInitialize(vboxPrivate *data)
-{
- data->pFuncs = g_pfnGetFunctions(VBOX_XPCOMC_VERSION);
- if (data->pFuncs == NULL)
- return -1;
-#if VBOX_XPCOMC_VERSION == 0x00010000U
- data->pFuncs->pfnComInitialize(&data->vboxObj, &data->vboxSession);
-#else /* !(VBOX_XPCOMC_VERSION == 0x00010000U) */
- data->pFuncs->pfnComInitialize(IVIRTUALBOX_IID_STR, &data->vboxObj, ISESSION_IID_STR, &data->vboxSession);
-#endif /* !(VBOX_XPCOMC_VERSION == 0x00010000U) */
- return 0;
-}
-
static int
_initializeDomainEvent(vboxPrivate *data ATTRIBUTE_UNUSED)
{
@@ -2009,13 +1982,38 @@ _initializeDomainEvent(vboxPrivate *data ATTRIBUTE_UNUSED)
}
static
-void _registerGlobalData(vboxPrivate *data ATTRIBUTE_UNUSED)
+vboxGlobalData* _registerGlobalData(void)
{
-#if VBOX_API_VERSION == 2002000
- vboxUnsupported();
-#else /* VBOX_API_VERSION != 2002000 */
- g_pVBoxGlobalData = (vboxGlobalData *) data;
-#endif /* VBOX_API_VERSION != 2002000 */
+ if (g_pVBoxGlobalData == NULL && VIR_ALLOC(g_pVBoxGlobalData) == 0) {
+ g_pVBoxGlobalData->pFuncs = g_pfnGetFunctions(VBOX_XPCOMC_VERSION);
+ if (g_pVBoxGlobalData->pFuncs == NULL)
+ return NULL;
+
+#if VBOX_XPCOMC_VERSION == 0x00010000U
+ g_pVBoxGlobalData->pFuncs->pfnComInitialize(&g_pVBoxGlobalData->vboxObj,
+ &g_pVBoxGlobalData->vboxSession);
+#else /* !(VBOX_XPCOMC_VERSION == 0x00010000U) */
+ g_pVBoxGlobalData->pFuncs->pfnComInitialize(IVIRTUALBOX_IID_STR,
+ &g_pVBoxGlobalData->vboxObj,
+ ISESSION_IID_STR,
+ &g_pVBoxGlobalData->vboxSession);
+#endif /* !(VBOX_XPCOMC_VERSION == 0x00010000U) */
+ }
+
+
+ if (g_pVBoxGlobalData->vboxObj == NULL) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("IVirtualBox object is null"));
+ return NULL;
+ }
+
+ if (g_pVBoxGlobalData->vboxSession == NULL) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("ISession object is null"));
+ return NULL;
+ }
+
+ return g_pVBoxGlobalData;
}
#if VBOX_API_VERSION < 4000000
@@ -2629,10 +2627,51 @@ _detachFloppy(IMachine *machine ATTRIBUTE_UNUSED)
#endif /* VBOX_API_VERSION >= 3001000 */
+static int _pfnInitialize(vboxPrivate *data)
+{
+ int ret = -1;
+
+ virMutexLock(&g_pVBoxGlobalData->lock);
+
+ if (g_pVBoxGlobalData->vboxObj && g_pVBoxGlobalData->vboxSession) {
+ data->vboxObj = g_pVBoxGlobalData->vboxObj;
+ data->vboxSession = g_pVBoxGlobalData->vboxSession;
+
+ g_pVBoxGlobalData->vboxObj->vtbl->nsisupports.AddRef((nsISupports *) g_pVBoxGlobalData->vboxObj);
+ g_pVBoxGlobalData->vboxSession->vtbl->nsisupports.AddRef((nsISupports *) g_pVBoxGlobalData->vboxSession);
+
+ data->caps = virObjectRef(g_pVBoxGlobalData->caps);
+
+ g_pVBoxGlobalData->connectionCount++;
+
+ ret = 0;
+ }
+
+ virMutexUnlock(&g_pVBoxGlobalData->lock);
+
+ return ret;
+}
+
static void _pfnUninitialize(vboxPrivate *data)
{
- if (data->pFuncs)
- data->pFuncs->pfnComUninitialize();
+ if (!data || !g_pVBoxGlobalData)
+ return;
+
+ virMutexLock(&g_pVBoxGlobalData->lock);
+
+ g_pVBoxGlobalData->connectionCount--;
+
+ data->vboxObj->vtbl->nsisupports.Release((nsISupports *) data->vboxObj);
+ data->vboxSession->vtbl->nsisupports.Release((nsISupports *) data->vboxSession);
+ virObjectUnref(data->caps);
+
+ if (g_pVBoxGlobalData->connectionCount == 0) {
+ g_pVBoxGlobalData->pFuncs->pfnComUninitialize();
+ virMutexUnlock(&g_pVBoxGlobalData->lock);
+ VIR_FREE(g_pVBoxGlobalData);
+ } else {
+ virMutexUnlock(&g_pVBoxGlobalData->lock);
+ }
}
static void _pfnComUnallocMem(PCVBOXXPCOM pFuncs, void *pv)
diff --git a/src/vbox/vbox_uniformed_api.h b/src/vbox/vbox_uniformed_api.h
index 50041c0..dac44e6 100644
--- a/src/vbox/vbox_uniformed_api.h
+++ b/src/vbox/vbox_uniformed_api.h
@@ -96,24 +96,28 @@ typedef union {
PRInt32 resultCode;
} resultCodeUnion;
+/* "extends" IVirtualBoxCallback to provide members for context info */
+typedef struct {
+ struct IVirtualBoxCallback_vtbl *vtbl;
+ virConnectPtr conn;
+ int vboxCallBackRefCount;
+} VirtualBoxCallback;
+
typedef struct {
virMutex lock;
unsigned long version;
- virCapsPtr caps;
virDomainXMLOptionPtr xmlopt;
+ /* references to members of g_pVBoxGlobalData */
+ virCapsPtr caps;
IVirtualBox *vboxObj;
ISession *vboxSession;
- /** Our version specific API table pointer. */
- PCVBOXXPCOM pFuncs;
-
/* The next is used for domainEvent */
/* Async event handling */
virObjectEventStatePtr domainEvents;
int fdWatch;
- int volatile vboxCallBackRefCount;
# if defined(VBOX_API_VERSION) && VBOX_API_VERSION > 2002000 && VBOX_API_VERSION < 4000000
IVirtualBoxCallback *vboxCallback;
nsIEventQueue *vboxQueue;
@@ -121,49 +125,26 @@ typedef struct {
void *vboxCallback;
void *vboxQueue;
# endif /* VBOX_API_VERSION <= 2002000 || VBOX_API_VERSION >= 4000000 || VBOX_API_VERSION undefined */
-
- /* pointer back to the connection */
- virConnectPtr conn;
} vboxPrivate;
typedef struct {
virMutex lock;
- unsigned long version;
-
virCapsPtr caps;
- virDomainXMLOptionPtr xmlopt;
-
- IVirtualBox *vboxObj;
- ISession *vboxSession;
/** Our version specific API table pointer. */
PCVBOXXPCOM pFuncs;
- /* The next is used for domainEvent */
-# if defined(VBOX_API_VERSION) && VBOX_API_VERSION > 2002000 && VBOX_API_VERSION < 4000000
-
- /* Async event handling */
- virObjectEventStatePtr domainEvents;
- int fdWatch;
- IVirtualBoxCallback *vboxCallback;
- nsIEventQueue *vboxQueue;
-
- int volatile vboxCallBackRefCount;
-
- /* pointer back to the connection */
- virConnectPtr conn;
-
-# else /* VBOX_API_VERSION <= 2002000 || VBOX_API_VERSION >= 4000000 || VBOX_API_VERSION undefined */
-
- virObjectEventStatePtr domainEvents;
- int fdWatch;
- void *vboxCallback;
- void *vboxQueue;
- int volatile vboxCallBackRefCount;
- virConnectPtr conn;
-
-# endif /* VBOX_API_VERSION <= 2002000 || VBOX_API_VERSION >= 4000000 || VBOX_API_VERSION undefined */
+ /* vbox objects shared for all connections.
+ *
+ * The pfnComInitialize does not support multiple sessions
+ * per process, therefore IVirutalBox and ISession must
+ * be global.
+ */
+ IVirtualBox *vboxObj;
+ ISession *vboxSession;
+ /* keeps track of vbox connection count */
+ int volatile connectionCount;
} vboxGlobalData;
/* vboxUniformedAPI gives vbox_common.c a uniformed layer to see
--
2.7.4
More information about the libvir-list
mailing list