On Tue, Oct 11, 2016 at 10:58:47AM -0400, Dawid Zamirski wrote:
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 v2I 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?
Yes, that would do.
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list