[PATCH 3/4] vbox: change API (un)initialization logic.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]