Re: [Xen-devel] [PATCH 07/10] libxl: New API for providing OS events to libxl

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Thu, 12 Jan 2012, Ian Campbell wrote:
> > I still prefer the approach prototyped in
> > http://marc.info/?l=xen-devel&m=132371797519877
> 
> I pointed out some issues with this in
> http://marc.info/?l=xen-devel&m=132378496608354&w=2 which have gone
> unanswered.

I was asked by Ian to wait for his next version and so I did.


> In particular error handling for the pthread_setspecific in
> libxl__free_all seems pretty nasty to me since you would be introducing
> a new failure path at the end of pretty much every libxl function. Worse
> it is after the actual action of each function is otherwise complete so
> you may end up having to undo stuff. I think it is going to be a lot of
> work to fix all of those up properly.
> 
> Having frequently used pro/epilogue code (especially epilogue) which
> cannot fail seems much simpler in general to me.

The epilogue is not a problem because pthread_setspecific could only
fail if the key is invalid so we don't need to handle that error.
However the prologue still needs error handling.
Personally I don't think there is anything wrong with a GC_INIT that can
fail. We can decide whether to handle the error within the macro (like
it is done now in the new updated patch) or outside the macro. Both are
fine by me.


diff -r 5b2676ac1321 tools/libxl/libxl.c
--- a/tools/libxl/libxl.c       Mon Jan 09 16:01:44 2012 +0100
+++ b/tools/libxl/libxl.c       Thu Jan 12 13:44:16 2012 +0000
@@ -60,6 +60,12 @@ int libxl_ctx_alloc(libxl_ctx **pctx, in
      * only as an initialiser, not as an expression. */
     memcpy(&ctx->lock, &mutex_value, sizeof(ctx->lock));
 
+    if(pthread_key_create(&ctx->tls_key, NULL) < 0) {
+        LIBXL__LOG_ERRNOVAL(ctx, LIBXL__LOG_ERROR, errno,
+            "cannot create tls key");
+        return ERROR_FAIL;
+    }
+
     if ( stat(XENSTORE_PID_FILE, &stat_buf) != 0 ) {
         LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "Is xenstore daemon running?\n"
                      "failed to stat %s", XENSTORE_PID_FILE);
@@ -93,6 +99,8 @@ int libxl_ctx_free(libxl_ctx *ctx)
     if (ctx->xch) xc_interface_close(ctx->xch);
     libxl_version_info_dispose(&ctx->version_info);
     if (ctx->xsh) xs_daemon_close(ctx->xsh);
+    if (pthread_key_delete(ctx->tls_key) < 0)
+        return ERROR_FAIL;
     free(ctx);
     return 0;
 }
diff -r 5b2676ac1321 tools/libxl/libxl_internal.c
--- a/tools/libxl/libxl_internal.c      Mon Jan 09 16:01:44 2012 +0100
+++ b/tools/libxl/libxl_internal.c      Thu Jan 12 13:44:16 2012 +0000
@@ -17,6 +17,7 @@
 
 #include <stdio.h>
 
+#include <pthread.h>
 #include <sys/types.h>
 #include <sys/stat.h>
 #include <fcntl.h>
@@ -61,11 +62,38 @@ int libxl__ptr_add(libxl__gc *gc, void *
     return 0;
 }
 
+libxl__gc *libxl__init_gc(libxl_ctx *ctx)
+{
+    libxl__gc *gc = (libxl__gc *) pthread_getspecific(ctx->tls_key);
+    if (gc == NULL) {
+        gc = (libxl__gc *) malloc(sizeof(libxl__gc));
+        if (gc == NULL)
+            return NULL;
+        gc->alloc_maxsize = 0;
+        gc->alloc_ptrs = 0;
+        gc->nested = 0;
+        gc->owner = ctx;
+        gc->nested = 1;
+        if (pthread_setspecific(ctx->tls_key, gc) < 0) {
+            free(gc);
+            return NULL;
+        }
+    } else {
+        gc->nested++;
+    }
+    return gc;
+}
+
+
 void libxl__free_all(libxl__gc *gc)
 {
     void *ptr;
     int i;
 
+    gc->nested--;
+    if (gc->nested > 0)
+        return;
+
     for (i = 0; i < gc->alloc_maxsize; i++) {
         ptr = gc->alloc_ptrs[i];
         gc->alloc_ptrs[i] = NULL;
@@ -74,6 +102,8 @@ void libxl__free_all(libxl__gc *gc)
     free(gc->alloc_ptrs);
     gc->alloc_ptrs = 0;
     gc->alloc_maxsize = 0;
+    pthread_setspecific(CTX->tls_key, NULL);
+    free(gc);
 }
 
 void *libxl__zalloc(libxl__gc *gc, int bytes)
diff -r 5b2676ac1321 tools/libxl/libxl_internal.h
--- a/tools/libxl/libxl_internal.h      Mon Jan 09 16:01:44 2012 +0100
+++ b/tools/libxl/libxl_internal.h      Thu Jan 12 13:44:16 2012 +0000
@@ -98,6 +98,8 @@ struct libxl__ctx {
     xc_interface *xch;
     struct xs_handle *xsh;
 
+    pthread_key_t tls_key;
+
     pthread_mutex_t lock; /* protects data structures hanging off the ctx */
       /* Always use CTX_LOCK and CTX_UNLOCK to manipulate this.
        *
@@ -145,9 +147,9 @@ typedef struct {
     int alloc_maxsize;
     void **alloc_ptrs;
     libxl_ctx *owner;
+    int nested;
 } libxl__gc;
 
-#define LIBXL_INIT_GC(ctx) (libxl__gc){ .alloc_maxsize = 0, .alloc_ptrs = 0, 
.owner = ctx }
 static inline libxl_ctx *libxl__gc_owner(libxl__gc *gc)
 {
     return gc->owner;
@@ -721,7 +723,8 @@ libxl__device_model_version_running(libx
  * as a local variable.
  */
 
-#define GC_INIT(ctx)  libxl__gc gc[1] = { LIBXL_INIT_GC(ctx) }
+_hidden libxl__gc *libxl__init_gc(struct libxl__ctx *ctx);
+#define GC_INIT(ctx)  libxl__gc *gc = libxl__init_gc(ctx); if (gc == NULL) 
return ERROR_NOMEM;
 #define GC_FREE       libxl__free_all(gc)
 #define CTX           libxl__gc_owner(gc)
 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel