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

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.

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