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

[Dawid Zamirski]

On Tue, 2016-10-11 at 16:22 +0200, Martin Kletzander wrote:
> On Wed, Sep 28, 2016 at 01:41:35PM -0400, Dawid Zamirski wrote:
> 
> 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.
> 

Thanks, I see, I'll address this in v2

> 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.

Neither I'm super familiar with vbox internals, but your suggestion
sounds reasonable, so I'll dive into that code in vbox source to find
out.

> 
> > 
> > * _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.

Understood, though I figure that your initial suggestion to guard
allocation of g_pVBoxGlobalData should take care of this?