2:46 a.m.
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(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/