Re: [libvirt] PATCH: Thread-safe error reporting public API

On Fri, Dec 12, 2008 at 09:46:14AM +0100, Daniel Veillard wrote: > On Thu, Dec 11, 2008 at 08:16:36PM +0000, Daniel P. Berrange wrote: > > The current public API for error reporting consists of > > > > - A global error object > > - A per-connection error object > > > > Some functions always set errors on the global object. Other functions > > always set errors on the per-connection object, except when they set > > errors on the global object. Both have built-in race conditions if they > > are accessed from multiple threads because of the time between the API > > call raising the error, and the caller querying it. > > > > The solution to this is to do away with all of the existing error objects > > and replace them with a per-thread error object. Well, except we can't > > do away with existing objects because of ABI compatability. This turns > > out to not be such a bad problem after all.... > > > > So with this patch... > > > > > > virterror.c gets ability to track a per-thread virErrorPtr instance. This > > object is stored in a thread local variable via pthread_{get,set}specific. > > It is allocated when first required, and deleted when the thread exits > > > > Every single API will *always* set the virErrorPtr object associated with > > its current thread. > > > > In the public virterror.h we add > > > > virErrorPtr virThreadGetLastError (void); > > int virThreadCopyLastError (virErrorPtr to); > > void virThreadResetLastError (void); > > > > This provides a guarenteed thread-safe public API for fetching error > > information. All the other existing APIs have docs updated to recommend > > against their use. > > I understand the problem but I don't understand the way you expect to > fix it. Historically this copies the libxml2 error APIs, but in libxml2 > the error data are stored in thread safe local storage. Until now > basically libvirt could not be used in a threaded way or at least not > without lot of constraints. Now we are getting thread safe, I suggest to > just retrofit thread safety into the exiting API, since they have the > exact same signature I see no reason to annoy the application writer > with extra API and deprecated ones. > If the application is single threaded nothing changes for them in > practice. > If the application is multi-threaded the old behaviour wasn't making > sense, could not be trusted and the new code does basically "the > right thing" now. Yes, I guess that does atually make sense. So I'll make the global virGetLastError() thread-safe, and then there's no need for the new APIs, and also no need for anyone to call the virConnGetLastError(), though I'll make sure that still syncs to whatever error is stored in the global location anyway.

> I guess saying "from 0.6.0 the API becomes thread-safe and as a result > the error data become thread specific" sounds just fine to me, and that > probably works better for most applications (and bindings). Sounds like a good plan.

With this error stuff done and the other patches I've posted this week all the major thread-safety problems are addressed. Its now a case of tracking down all the loose-ends / minor faults. The Xen driver will be easy enough todo - its mostly stateless apart from the async events which is easy . Then there's the strerror_r() stuff and other related _r() functions. And whatever other edge-cases i discover.