On Wed, Sep 28, 2016 at 01:41:35PM -0400, Dawid Zamirski wrote:
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.
The allocation is not guarded by any lock, so there's still a race. I think there should be a global struct that has only some lock in it and whatever global data you need, the struct will be initialized on the first call to any function (check out VIR_ONCE_GLOBAL_INIT) and then the connection (or global data or how it's called) would be reference counted (just like you have). It's just that having the reference count in the object you will be reallocating over and over again for each connection is not really good. I don't understand how vbox works, but if initializing g_pVBoxGlobalData does not make any connection, and ISession does (which would make sense), we could keep the global data around and add ISession for each connection. I guess that's something you're talking about below.
* _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.
That's not true if there is a connection being made while it is being free()d or it's being allocated in two threads, etc. Unless I missed something, that is.
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list