[libvirt] [PATCH 3/4] vbox: change API (un)initialization logic.
Martin Kletzander
mkletzan at redhat.com
Tue Oct 11 15:24:53 UTC 2016
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 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?
Yes, that would do.
-------------- 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/0ae1d3a8/attachment-0001.sig>
More information about the libvir-list
mailing list