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

[Daniel Veillard]

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.

  IMHO, just put the thread support in the old APIs and avoid expanding
the API set and exposing thread specific APIs to the programmers. A
single entry point which does the right thing in all situations is
better than a new set of thread specific APIs.
  The key is that the new APIs have exactly the same signature, and
basically the same behaviour except for the specific cases where the
old behaviour could not be trusted or didn't make sense. IMHO this
is a fix of the old APIs we should just view the patch this way.

> In libvirt.c, in any exit paths which result in an error code, we copy
> the per-thread virErrorPtr object into either the global error object
> or per-connection object as applicable for the scenario. This gives us
> 100% backwards compatability. NB, we hold a lock when doing this so
> that these are race-free when setting them.
> 
> At the start of every API call, we call virThreadResetLastError() and
> at any exit path with an error, if the error object is not set, then
> we set a generic error message. This means that if the internal driver
> code is broken and forgets to raise an error, the caller will still
> at least see a generic error report.
> 
> Finally, virCopyLastError and virConnCopyLastError were not correctly
> strdup'ing the char * fields. This meant that if the original error
> was cleared, you'd get a use-after-free error, shortly followed by
> a double-free error if the first didn't kill you. This patch also
> fixes those two methods to correctly strdup the char *.

  All those sounds excellent fixes,

> As for language bindings, they should *all* be updated to use the
> virThreadGetLastError() method, and *never* call virGetLastError()
> or the virConnGetLastError() calls.
> 
> With this patch applied, assuming all the per-hypervisor drivers are
> thread-safe (they are except for Xen which is still TODO), then the 
> public API for virConectPtr is also (almost[1]) guarenteed to be thread-
> safe.

  
[...]
> @@ -1050,8 +1045,16 @@ virConnectClose(virConnectPtr conn)
>  {
>      DEBUG("conn=%p", conn);
>  
> -    if (!VIR_IS_CONNECT(conn))
> -        return (-1);
> +    virThreadResetLastError();
> +
> +    virThreadResetLastError();

  Hum one should be sufficient :-) 

  In general though I guess we need to agree (one way or another) on the
above before reviewing patches though.
  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).

Daniel

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel at veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/