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

[Martin Kletzander]

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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20161011/d480b3a9/attachment-0001.sig>