Thread local error reporting

ChangeLog

@@ -1,3 +1,20 @@
+Tue Jan 20 12:01:53 GMT 2009 Daniel P. Berrange <berrange@redhat.com>
+
+	Thread local error handling
+	* src/datatypes.c: Don't reference global error object directly
+	now that it is thread-local. Avoid passing 'conn' arg to error
+	routines if we just determined that the pointer is invalid
+	* src/datatypes.h: Add note about rules for locking when using
+	virConnectPtr members
+	* src/libvirt.c: Initialize error handling routines at startup.
+	Adapt driver API methods to reset last error upon entry, and
+	copy the global thread local error to the per-connection error
+	upon exit
+	* src/virterror.c, src/virterror_internal.h: Store the global
+	error object in a thread local variable. Provide a API to copy
+	the global error into a per-connection error object. Add an
+	initialization routine to setup the thread local
+
 Tue Jan 20 11:43:53 GMT 2009 Daniel P. Berrange <berrange@redhat.com>
 
 	* src/remote_internal.c: Disable event watch when doing an

src/datatypes.c

@@ -195,8 +195,6 @@ virReleaseConnect(virConnectPtr conn) {
         virHashFree(conn->nodeDevices, (virHashDeallocator) virNodeDeviceFree);
 
     virResetError(&conn->err);
-    if (virLastErr.conn == conn)
-        virLastErr.conn = NULL;
 
     xmlFreeURI(conn->uri);
 
@@ -219,7 +217,7 @@ virUnrefConnect(virConnectPtr conn) {
     int refs;
 
     if ((!VIR_IS_CONNECT(conn))) {
-        virLibConnError(conn, VIR_ERR_INVALID_ARG, __FUNCTION__);
+        virLibConnError(NULL, VIR_ERR_INVALID_ARG, __FUNCTION__);
         return(-1);
     }
     virMutexLock(&conn->lock);
@@ -253,7 +251,7 @@ virGetDomain(virConnectPtr conn, const char *name, const unsigned char *uuid) {
     virDomainPtr ret = NULL;
 
     if ((!VIR_IS_CONNECT(conn)) || (name == NULL) || (uuid == NULL)) {
-        virLibConnError(conn, VIR_ERR_INVALID_ARG, __FUNCTION__);
+        virLibConnError(NULL, VIR_ERR_INVALID_ARG, __FUNCTION__);
         return(NULL);
     }
     virMutexLock(&conn->lock);
@@ -323,10 +321,6 @@ virReleaseDomain(virDomainPtr domain) {
         virLibConnError(conn, VIR_ERR_INTERNAL_ERROR,
                         _("domain missing from connection hash table"));
 
-    if (conn->err.dom == domain)
-        conn->err.dom = NULL;
-    if (virLastErr.dom == domain)
-        virLastErr.dom = NULL;
     domain->magic = -1;
     domain->id = -1;
     VIR_FREE(domain->name);
@@ -358,7 +352,7 @@ virUnrefDomain(virDomainPtr domain) {
     int refs;
 
     if (!VIR_IS_CONNECTED_DOMAIN(domain)) {
-        virLibConnError(domain->conn, VIR_ERR_INVALID_ARG, __FUNCTION__);
+        virLibConnError(NULL, VIR_ERR_INVALID_ARG, __FUNCTION__);
         return(-1);
     }
     virMutexLock(&domain->conn->lock);
@@ -393,7 +387,7 @@ virGetNetwork(virConnectPtr conn, const char *name, const unsigned char *uuid) {
     virNetworkPtr ret = NULL;
 
     if ((!VIR_IS_CONNECT(conn)) || (name == NULL) || (uuid == NULL)) {
-        virLibConnError(conn, VIR_ERR_INVALID_ARG, __FUNCTION__);
+        virLibConnError(NULL, VIR_ERR_INVALID_ARG, __FUNCTION__);
         return(NULL);
     }
     virMutexLock(&conn->lock);
@@ -459,11 +453,6 @@ virReleaseNetwork(virNetworkPtr network) {
         virLibConnError(conn, VIR_ERR_INTERNAL_ERROR,
                         _("network missing from connection hash table"));
 
-    if (conn->err.net == network)
-        conn->err.net = NULL;
-    if (virLastErr.net == network)
-        virLastErr.net = NULL;
-
     network->magic = -1;
     VIR_FREE(network->name);
     VIR_FREE(network);
@@ -494,7 +483,7 @@ virUnrefNetwork(virNetworkPtr network) {
     int refs;
 
     if (!VIR_IS_CONNECTED_NETWORK(network)) {
-        virLibConnError(network->conn, VIR_ERR_INVALID_ARG, __FUNCTION__);
+        virLibConnError(NULL, VIR_ERR_INVALID_ARG, __FUNCTION__);
         return(-1);
     }
     virMutexLock(&network->conn->lock);
@@ -530,7 +519,7 @@ virGetStoragePool(virConnectPtr conn, const char *name, const unsigned char *uui
     virStoragePoolPtr ret = NULL;
 
     if ((!VIR_IS_CONNECT(conn)) || (name == NULL) || (uuid == NULL)) {
-        virLibConnError(conn, VIR_ERR_INVALID_ARG, __FUNCTION__);
+        virLibConnError(NULL, VIR_ERR_INVALID_ARG, __FUNCTION__);
         return(NULL);
     }
     virMutexLock(&conn->lock);
@@ -627,7 +616,7 @@ virUnrefStoragePool(virStoragePoolPtr pool) {
     int refs;
 
     if (!VIR_IS_CONNECTED_STORAGE_POOL(pool)) {
-        virLibConnError(pool->conn, VIR_ERR_INVALID_ARG, __FUNCTION__);
+        virLibConnError(NULL, VIR_ERR_INVALID_ARG, __FUNCTION__);
         return(-1);
     }
     virMutexLock(&pool->conn->lock);
@@ -664,7 +653,7 @@ virGetStorageVol(virConnectPtr conn, const char *pool, const char *name, const c
     virStorageVolPtr ret = NULL;
 
     if ((!VIR_IS_CONNECT(conn)) || (name == NULL) || (key == NULL)) {
-        virLibConnError(conn, VIR_ERR_INVALID_ARG, __FUNCTION__);
+        virLibConnError(NULL, VIR_ERR_INVALID_ARG, __FUNCTION__);
         return(NULL);
     }
     virMutexLock(&conn->lock);
@@ -765,7 +754,7 @@ virUnrefStorageVol(virStorageVolPtr vol) {
     int refs;
 
     if (!VIR_IS_CONNECTED_STORAGE_VOL(vol)) {
-        virLibConnError(vol->conn, VIR_ERR_INVALID_ARG, __FUNCTION__);
+        virLibConnError(NULL, VIR_ERR_INVALID_ARG, __FUNCTION__);
         return(-1);
     }
     virMutexLock(&vol->conn->lock);
@@ -801,7 +790,7 @@ virGetNodeDevice(virConnectPtr conn, const char *name)
     virNodeDevicePtr ret = NULL;
 
     if ((!VIR_IS_CONNECT(conn)) || (name == NULL)) {
-        virLibConnError(conn, VIR_ERR_INVALID_ARG, __FUNCTION__);
+        virLibConnError(NULL, VIR_ERR_INVALID_ARG, __FUNCTION__);
         return(NULL);
     }
     virMutexLock(&conn->lock);

src/datatypes.h

@@ -95,6 +95,10 @@
  * Internal structure associated to a connection
  */
 struct _virConnect {
+    /* All the variables from here, until the 'lock' declaration
+     * are setup at time of connection open, and never changed
+     * since. Thus no need to lock when accessing them
+     */
     unsigned int magic;     /* specific value to check */
     int flags;              /* a set of connection flags */
     xmlURIPtr uri;          /* connection URI */
@@ -114,11 +118,6 @@ struct _virConnect {
     void *            storagePrivateData;
     void *            devMonPrivateData;
 
-    /* Per-connection error. */
-    virError err;           /* the last error */
-    virErrorFunc handler;   /* associated handlet */
-    void *userData;         /* the user data */
-
     /*
      * The lock mutex must be acquired before accessing/changing
      * any of members following this point, or changing the ref
@@ -126,6 +125,12 @@ struct _virConnect {
      * this connection
      */
     virMutex lock;
+
+    /* Per-connection error. */
+    virError err;           /* the last error */
+    virErrorFunc handler;   /* associated handlet */
+    void *userData;         /* the user data */
+
     virHashTablePtr domains;  /* hash table for known domains */
     virHashTablePtr networks; /* hash table for known domains */
     virHashTablePtr storagePools;/* hash table for known storage pools */

src/virterror.c

@@ -18,11 +18,11 @@
 #include "virterror_internal.h"
 #include "datatypes.h"
 #include "logging.h"
+#include "memory.h"
+#include "threads.h"
+
+virThreadLocal virLastErr;
 
-virError virLastErr =       /* the last error */
-  { .code = 0, .domain = 0, .message = NULL, .level = VIR_ERR_NONE,
-    .conn = NULL, .dom = NULL, .str1 = NULL, .str2 = NULL, .str3 = NULL,
-    .int1 = 0, .int2 = 0, .net = NULL };
 virErrorFunc virErrorHandler = NULL;     /* global error handler */
 void *virUserData = NULL;        /* associated data */
 
@@ -154,28 +154,118 @@ static const char *virErrorDomainName(virErrorDomain domain) {
     return(dom);
 }
 
+
 /*
+ * Internal helper that is called when a thread exits, to
+ * release the error object stored in the thread local
+ */
+static void
+virLastErrFreeData(void *data)
+{
+    virErrorPtr err = data;
+    if (!err)
+        return;
+    virResetError(err);
+    VIR_FREE(err);
+}
+
+
+int
+virErrorInitialize(void)
+{
+    return virThreadLocalInit(&virLastErr, virLastErrFreeData);
+}
+
+
+/*
+ * Internal helper to ensure a generic error code is stored
+ * in case where API returns failure, but forgot to set an
+ * error
+ */
+static void
+virErrorGenericFailure(virErrorPtr err)
+{
+    err->code = VIR_ERR_INTERNAL_ERROR;
+    err->domain = VIR_FROM_NONE;
+    err->level = VIR_ERR_ERROR;
+    err->message = strdup(_("Unknown failure"));
+}
+
+
+/*
+ * Internal helper to perform a deep copy of the an error
+ */
+static int
+virCopyError(virErrorPtr from,
+             virErrorPtr to)
+{
+    int ret = 0;
+    if (!to)
+        return 0;
+    virResetError(to);
+    if (!from)
+        return 0;
+    to->code = from->code;
+    to->domain = from->domain;
+    to->level = from->level;
+    if (from->message && !(to->message = strdup(from->message)))
+        ret = -1;
+    if (from->str1 && !(to->str1 = strdup(from->str1)))
+        ret = -1;
+    if (from->str2 && !(to->str2 = strdup(from->str2)))
+        ret = -1;
+    if (from->str3 && !(to->str3 = strdup(from->str3)))
+        ret = -1;
+    to->int1 = from->int1;
+    to->int2 = from->int2;
+    /*
+     * Delibrately not setting 'conn', 'dom', 'net' references
+     */
+    return ret;
+}
+
+static virErrorPtr
+virLastErrorObject(void)
+{
+    virErrorPtr err;
+    err = virThreadLocalGet(&virLastErr);
+    if (!err) {
+        if (VIR_ALLOC(err) < 0)
+            return NULL;
+        virThreadLocalSet(&virLastErr, err);
+    }
+    return err;
+}
+
+
+/**
  * virGetLastError:
  *
  * Provide a pointer to the last error caught at the library level
- * Simpler but may not be suitable for multithreaded accesses, in which
+ *
- * case use virCopyLastError()
+ * The error object is kept in thread local storage, so separate
+ * threads can safely access this concurrently.
  *
  * Returns a pointer to the last error or NULL if none occurred.
  */
 virErrorPtr
 virGetLastError(void)
 {
-    if (virLastErr.code == VIR_ERR_OK)
+    virErrorPtr err = virLastErrorObject();
-        return (NULL);
+    if (!err || err->code == VIR_ERR_OK)
-    return (&virLastErr);
+        return NULL;
+    return err;
 }
 
-/*
+/**
  * virCopyLastError:
  * @to: target to receive the copy
  *
  * Copy the content of the last error caught at the library level
+ *
+ * The error object is kept in thread local storage, so separate
+ * threads can safely access this concurrently.
+ *
  * One will need to free the result with virResetError()
  *
  * Returns 0 if no error was found and the error code otherwise and -1 in case
@@ -184,12 +274,14 @@ virGetLastError(void)
 int
 virCopyLastError(virErrorPtr to)
 {
-    if (to == NULL)
+    virErrorPtr err = virLastErrorObject();
-        return (-1);
+    /* We can't guarentee caller has initialized it to zero */
-    if (virLastErr.code == VIR_ERR_OK)
+    memset(to, 0, sizeof(*to));
-        return (0);
+    if (err)
-    memcpy(to, &virLastErr, sizeof(virError));
+        virCopyError(err, to);
-    return (virLastErr.code);
+    else
+        virResetError(to);
+    return to->code;
 }
 
 /**
@@ -210,15 +302,22 @@ virResetError(virErrorPtr err)
     memset(err, 0, sizeof(virError));
 }
 
+
 /**
  * virResetLastError:
  *
  * Reset the last error caught at the library level.
+ *
+ * The error object is kept in thread local storage, so separate
+ * threads can safely access this concurrently, only resetting
+ * their own error object.
  */
 void
 virResetLastError(void)
 {
-    virResetError(&virLastErr);
+    virErrorPtr err = virLastErrorObject();
+    if (err)
+        virResetError(err);
 }
 
 /**
@@ -226,8 +325,20 @@ virResetLastError(void)
  * @conn: pointer to the hypervisor connection
  *
  * Provide a pointer to the last error caught on that connection
- * Simpler but may not be suitable for multithreaded accesses, in which
+ *
- * case use virConnCopyLastError()
+ * This method is not protected against access from multiple
+ * threads. In a multi-threaded application, always use the
+ * global virGetLastError() API which is backed by thread
+ * local storage.
+ *
+ * If the connection object was discovered to be invalid by
+ * an API call, then the error will be reported against the
+ * global error object.
+ * 
+ * Since 0.6.0, all errors reported in the per-connection object
+ * are also duplicated in the global error object. As such an
+ * application can always use virGetLastError(). This method
+ * remains for backwards compatability.
  *
  * Returns a pointer to the last error or NULL if none occurred.
  */
@@ -235,8 +346,8 @@ virErrorPtr
 virConnGetLastError(virConnectPtr conn)
 {
     if (conn == NULL)
-        return (NULL);
+        return NULL;
-    return (&conn->err);
+    return &conn->err;
 }
 
 /**
@@ -245,6 +356,21 @@ virConnGetLastError(virConnectPtr conn)
  * @to: target to receive the copy
  *
  * Copy the content of the last error caught on that connection
+ *
+ * This method is not protected against access from multiple
+ * threads. In a multi-threaded application, always use the
+ * global virGetLastError() API which is backed by thread
+ * local storage.
+ *
+ * If the connection object was discovered to be invalid by
+ * an API call, then the error will be reported against the
+ * global error object.
+ * 
+ * Since 0.6.0, all errors reported in the per-connection object
+ * are also duplicated in the global error object. As such an
+ * application can always use virGetLastError(). This method
+ * remains for backwards compatability.
+ *
  * One will need to free the result with virResetError()
  *
  * Returns 0 if no error was found and the error code otherwise and -1 in case
@@ -253,20 +379,27 @@ virConnGetLastError(virConnectPtr conn)
 int
 virConnCopyLastError(virConnectPtr conn, virErrorPtr to)
 {
+    /* We can't guarentee caller has initialized it to zero */
+    memset(to, 0, sizeof(*to));
+
     if (conn == NULL)
-        return (-1);
+        return -1;
-    if (to == NULL)
+    virMutexLock(&conn->lock);
-        return (-1);
     if (conn->err.code == VIR_ERR_OK)
-        return (0);
+        virResetError(to);
-    memcpy(to, &conn->err, sizeof(virError));
+    else
-    return (conn->err.code);
+        virCopyError(&conn->err, to);
+    virMutexUnlock(&conn->lock);
+    return to->code;
 }
 
 /**
  * virConnResetLastError:
  * @conn: pointer to the hypervisor connection
  *
+ * The error object is kept in thread local storage, so separate
+ * threads can safely access this concurrently.
+ *
  * Reset the last error caught on that connection
  */
 void
@@ -274,7 +407,9 @@ virConnResetLastError(virConnectPtr conn)
 {
     if (conn == NULL)
         return;
+    virMutexLock(&conn->lock);
     virResetError(&conn->err);
+    virMutexUnlock(&conn->lock);
 }
 
 /**
@@ -309,8 +444,10 @@ virConnSetErrorFunc(virConnectPtr conn, void *userData,
 {
     if (conn == NULL)
         return;
+    virMutexLock(&conn->lock);
     conn->handler = handler;
     conn->userData = userData;
+    virMutexUnlock(&conn->lock);
 }
 
 /**
@@ -357,6 +494,44 @@ virDefaultErrorFunc(virErrorPtr err)
                 dom, lvl, domain, network, err->message);
 }
 
+/*
+ * Internal helper to ensure the global error object
+ * is initialized with a generic message if not already
+ * set.
+ */
+void
+virSetGlobalError(void)
+{
+    virErrorPtr err = virLastErrorObject();
+
+    if (err && err->code == VIR_ERR_OK)
+        virErrorGenericFailure(err);
+}
+
+/*
+ * Internal helper to ensure the connection error object
+ * is initialized from the global object.
+ */
+void
+virSetConnError(virConnectPtr conn)
+{
+    virErrorPtr err = virLastErrorObject();
+
+    if (err && err->code == VIR_ERR_OK)
+        virErrorGenericFailure(err);
+
+    if (conn) {
+        virMutexLock(&conn->lock);
+        if (err)
+            virCopyError(err, &conn->err);
+        else
+            virErrorGenericFailure(&conn->err);
+        virMutexUnlock(&conn->lock);
+    }
+}
+
+
+
 /**
  * virRaiseError:
  * @conn: the connection to the hypervisor if available
@@ -377,16 +552,29 @@ virDefaultErrorFunc(virErrorPtr err)
  * immediately if a callback is found and store it for later handling.
  */
 void
-virRaiseError(virConnectPtr conn, virDomainPtr dom, virNetworkPtr net,
+virRaiseError(virConnectPtr conn,
+              virDomainPtr dom ATTRIBUTE_UNUSED,
+              virNetworkPtr net ATTRIBUTE_UNUSED,
               int domain, int code, virErrorLevel level,
               const char *str1, const char *str2, const char *str3,
               int int1, int int2, const char *msg, ...)
 {
-    virErrorPtr to = &virLastErr;
+    virErrorPtr to;
     void *userData = virUserData;
     virErrorFunc handler = virErrorHandler;
     char *str;
 
+    /*
+     * All errors are recorded in thread local storage
+     * For compatability, public API calls will copy them
+     * to the per-connection error object when neccessary
+     */
+    to = virLastErrorObject();
+    if (!to)
+        return; /* Hit OOM allocating thread error object, sod all we can do now */
+
+    virResetError(to);
+
     if (code == VIR_ERR_OK)
         return;
 
@@ -394,11 +582,12 @@ virRaiseError(virConnectPtr conn, virDomainPtr dom, virNetworkPtr net,
      * try to find the best place to save and report the error
      */
     if (conn != NULL) {
-        to = &conn->err;
+        virMutexLock(&conn->lock);
         if (conn->handler != NULL) {
             handler = conn->handler;
             userData = conn->userData;
         }
+        virMutexUnlock(&conn->lock);
     }
 
     /*
@@ -421,9 +610,10 @@ virRaiseError(virConnectPtr conn, virDomainPtr dom, virNetworkPtr net,
      * Save the information about the error
      */
     virResetError(to);
-    to->conn = conn;
+    /*
-    to->dom = dom;
+     * Delibrately not setting conn, dom & net fields since
-    to->net = net;
+     * they're utterly unsafe
+     */
     to->domain = domain;
     to->code = code;
     to->message = str;
@@ -813,3 +1003,5 @@ void virReportErrorHelper(virConnectPtr conn, int domcode, int errcode,
                   virerr, errorMessage, NULL, -1, -1, virerr, errorMessage);
 
 }
+
+

src/virterror_internal.h

@@ -24,7 +24,6 @@
 
 #include "internal.h"
 
-extern virError virLastErr;
 extern virErrorFunc virErrorHandler;
 extern void *virUserData;
 
@@ -33,6 +32,7 @@ extern void *virUserData;
  *		API for error handling					*
  *									*
  ************************************************************************/
+int virErrorInitialize(void);
 void virRaiseError(virConnectPtr conn,
                    virDomainPtr dom,
                    virNetworkPtr net,
@@ -53,4 +53,7 @@ void virReportErrorHelper(virConnectPtr conn, int domcode, int errcode,
   ATTRIBUTE_FORMAT(printf, 7, 8);
 
 
+void virSetGlobalError(void);
+void virSetConnError(virConnectPtr conn);
+
 #endif