On Wednesday 04 April 2012 17:56:12 Jean-Baptiste Rouault wrote: > On Monday 16 January 2012 11:34:53 Matthias Bolte wrote: > > Okay, without looking deeper into this here are some ideas: > > > > The XPCOM API libvirt uses might not be threadsafe, or needs to be > > initialized for every thread that wants to use it. Currently its only > > initialized for the thread that opens the driver. I know that this is > > the case on Windows were VirtualBox uses MSCOM for its API and you > > need to call CoInitialize on every thread. This is currently not done > > for the MSCOM glue in libvirt, so I know that on Windows the > > VirtualBox driver is not threadsafe currently. Also I didn't look into > > a solution for this yet. Maybe we need a thread local variable that > > holds whether (MS/XP)COM was already initialized for this thread and > > add a check to every driver function to initialize it when needed. > > > > Did you try to open a connection for each thread instead of trying to > > share one? If that works reliable it might indicate that there is an > > VirtualBox API initialization problem. > > I tried today with one connection for each thread and it works. > > I changed the vbox driver so that the pfnComInitialize function is called > only when the first connection is opened : it breaks the test, even with > one connection per thread. > > I guess we'll have to use a thread local variable as you suggested, unless > someone has a better idea to handle this problem. Hi, I looked deeper into these thread-safety issues, once a new connection is opened for each thread, everything works well. However, opening and closing connections isn't thread-safe at all for two reasons : - VirtualBox C bindings initialization and uninitialization functions aren't thread-safe. I talked about it with upstream on IRC and they are probably not going to fix it, but would accept a patch fixing the issue. I'm going to contact upstream again to get some advices so I can write a patch. - In the libvirt vbox driver, for each new connection, modification of the global variable g_pVBoxGlobalData isn't protected (see line 1040 of vbox_tmpl.c). First of all, is it really necessary to replace it on each new connection, or would it be ok to initialize it only when the first connection is opened ? A global mutex is needed in the vbox driver to protect access to g_pVBoxGlobalData, the vboxRegister() function seems to be the best place to initialize such a mutex unless there is another entry point to do this ?

Am 19. April 2012 12:51 schrieb Jean-Baptiste Rouault > > A global mutex is needed in the vbox driver to protect access to > g_pVBoxGlobalData, the vboxRegister() function seems to be the best place > to initialize such a mutex unless there is another entry point to do > this ? Such a mutex doesn't help. Looking at g_pVBoxGlobalData tells me that we need to get rid of it in its current form for different reasons. The major reason is that it contains connection specific data such as the conn and domainEvents members. This means when you open a new connection you break all other open connections because connection specific data is overwritten. We need to redesign this. A major reason for the existence of g_pVBoxGlobalData is given in line 209 of vbox_tmpl.c: g_pVBoxGlobalData needs to be global because event callbacks won't work otherwise. Actually that's not true. Who ever did the original implementation of this was not aware of how COM (MS and XP variants) works. There is a vboxAllocCallbackObj function that creates an IVirtualBoxCallback COM object. The first member in a COM objects is a pointer to its vtbl that contains pointers to all the methods that can be called on it. After this vtbl the COM object contains its private data members, this aspect is not used in the current implementation. So we could just allocate a bigger object and put the data that belongs to the IVirtualBoxCallback implementation into this additional memory. This includes at least vboxCallBackRefCount and domainEvents that are currently located in g_pVBoxGlobalData. The only two members of g_pVBoxGlobalData that can stay global are caps and pFuncs, all other members are specific to a connection and cannot be global.

Are you referring to data->pFuncs->pfnComInitialize and data->pFuncs->pfnComUninitialize when you say "VirtualBox C bindings initialization and uninitialization functions"? As those are used to create connection specific objects we cannot move them out of vboxOpen/vboxClose. If they are not thread-safe than we need to put a global mutex around these calls.