Re: [PATCH] libxenlight: fix multiple xenstore watches problem

Re: [PATCH] libxenlight: fix multiple xenstore watches problem

[Andres Lagar-Cavilla]

mmm,
most likely the first two patches in my latest resend clash against 
this. Yikes
Andres
> Message: 3
> Date: Wed, 2 Dec 2009 15:01:32 +0000
> From: Stefano Stabellini<stefano.stabellini@eu.citrix.com>
> Subject: [Xen-devel] [PATCH] libxenlight: fix multiple xenstore
> 	watches	problem
> To: xen-devel@lists.xensource.com
> Message-ID:<alpine.DEB.2.00.0912021455450.26846@kaball-desktop>
> Content-Type: text/plain; charset="US-ASCII"
>
> Hi all,
> this patch fixes the multiple xenstore watches problem in libxenlight
> opening a new xenstore connection to set and read temporary watches on
> the device state nodes.
> This way they don't interfere with other long running watches.
>
> Signed-off-by: Stefano Stabellini<stefano.stabellini@eu.citrix.com>
>
> ---
>
> diff -r 96a9740f4f33 tools/libxl/libxl_device.c
> --- a/tools/libxl/libxl_device.c	Wed Dec 02 14:29:24 2009 +0000
> +++ b/tools/libxl/libxl_device.c	Wed Dec 02 14:43:06 2009 +0000
> @@ -212,47 +212,50 @@
>       fd_set rfds;
>       struct timeval tv;
>       flexarray_t *toremove;
> +    struct libxl_ctx clone = *ctx;
>
> +    clone.xsh = xs_daemon_open();
>       toremove = flexarray_make(16, 1);
> -    path = libxl_sprintf(ctx, "/local/domain/%d/device", domid);
> -    l1 = libxl_xs_directory(ctx, XBT_NULL, path,&num1);
> +    path = libxl_sprintf(&clone, "/local/domain/%d/device", domid);
> +    l1 = libxl_xs_directory(&clone, XBT_NULL, path,&num1);
>       if (!l1) {
> -        XL_LOG(ctx, XL_LOG_ERROR, "%s is empty", path);
> +        XL_LOG(&clone, XL_LOG_ERROR, "%s is empty", path);
> +        xs_daemon_close(clone.xsh);
>           return -1;
>       }
>       for (i = 0; i<  num1; i++) {
> -        path = libxl_sprintf(ctx, "/local/domain/%d/device/%s", domid, l1[i]);
> -        l2 = libxl_xs_directory(ctx, XBT_NULL, path,&num2);
> +        path = libxl_sprintf(&clone, "/local/domain/%d/device/%s", domid, l1[i]);
> +        l2 = libxl_xs_directory(&clone, XBT_NULL, path,&num2);
>           if (!l2)
>               continue;
>           for (j = 0; j<  num2; j++) {
> -            fe_path = libxl_sprintf(ctx, "/local/domain/%d/device/%s/%s", domid, l1[i], l2[j]);
> -            be_path = libxl_xs_read(ctx, XBT_NULL, libxl_sprintf(ctx, "%s/backend", fe_path));
> +            fe_path = libxl_sprintf(&clone, "/local/domain/%d/device/%s/%s", domid, l1[i], l2[j]);
> +            be_path = libxl_xs_read(&clone, XBT_NULL, libxl_sprintf(&clone, "%s/backend", fe_path));
>               if (be_path != NULL) {
> -                if (libxl_device_destroy(ctx, be_path, force)>  0)
> +                if (libxl_device_destroy(&clone, be_path, force)>  0)
>                       n_watches++;
> -                flexarray_set(toremove, n++, libxl_dirname(ctx, be_path));
> +                flexarray_set(toremove, n++, libxl_dirname(&clone, be_path));
>               } else {
> -                xs_rm(ctx->xsh, XBT_NULL, path);
> +                xs_rm(clone.xsh, XBT_NULL, path);
>               }
>           }
>       }
>       if (!force) {
> -        nfds = xs_fileno(ctx->xsh) + 1;
> +        nfds = xs_fileno(clone.xsh) + 1;
>           /* Linux-ism */
>           tv.tv_sec = LIBXL_DESTROY_TIMEOUT;
>           tv.tv_usec = 0;
>           while (n_watches>  0&&  tv.tv_sec>  0) {
>               FD_ZERO(&rfds);
> -            FD_SET(xs_fileno(ctx->xsh),&rfds);
> +            FD_SET(xs_fileno(clone.xsh),&rfds);
>               if (select(nfds,&rfds, NULL, NULL,&tv)>  0) {
> -                l1 = xs_read_watch(ctx->xsh,&num1);
> +                l1 = xs_read_watch(clone.xsh,&num1);
>                   if (l1 != NULL) {
> -                    char *state = libxl_xs_read(ctx, XBT_NULL, l1[0]);
> +                    char *state = libxl_xs_read(&clone, XBT_NULL, l1[0]);
>                       if (!state || atoi(state) == 6) {
> -                        xs_unwatch(ctx->xsh, l1[0], l1[1]);
> -                        xs_rm(ctx->xsh, XBT_NULL, l1[1]);
> -                        XL_LOG(ctx, XL_LOG_DEBUG, "Destroyed device backend at %s", l1[1]);
> +                        xs_unwatch(clone.xsh, l1[0], l1[1]);
> +                        xs_rm(clone.xsh, XBT_NULL, l1[1]);
> +                        XL_LOG(&clone, XL_LOG_DEBUG, "Destroyed device backend at %s", l1[1]);
>                           n_watches--;
>                       }
>                       free(l1);
> @@ -263,9 +266,10 @@
>       }
>       for (i = 0; i<  n; i++) {
>           flexarray_get(toremove, i, (void**)&path);
> -        xs_rm(ctx->xsh, XBT_NULL, path);
> +        xs_rm(clone.xsh, XBT_NULL, path);
>       }
>       flexarray_free(toremove);
> +    xs_daemon_close(clone.xsh);
>       return 0;
>   }
>
>    

Re: Re: [PATCH] libxenlight: fix multiple xenstore watches problem

[Keir Fraser]

I will apply Stefano's patches now, then you can see what bits you need to
re-send. Stefano or Vincent will want to ack/nack your patches before I try
to apply them anyway.

 -- Keir

On 02/12/2009 18:08, "Andres Lagar-Cavilla" <andres@lagarcavilla.com> wrote:

> mmm,
> most likely the first two patches in my latest resend clash against
> this. Yikes
> Andres
>> Message: 3
>> Date: Wed, 2 Dec 2009 15:01:32 +0000
>> From: Stefano Stabellini<stefano.stabellini@eu.citrix.com>
>> Subject: [Xen-devel] [PATCH] libxenlight: fix multiple xenstore
>> watches problem
>> To: xen-devel@lists.xensource.com
>> Message-ID:<alpine.DEB.2.00.0912021455450.26846@kaball-desktop>
>> Content-Type: text/plain; charset="US-ASCII"
>> 
>> Hi all,
>> this patch fixes the multiple xenstore watches problem in libxenlight
>> opening a new xenstore connection to set and read temporary watches on
>> the device state nodes.
>> This way they don't interfere with other long running watches.
>> 
>> Signed-off-by: Stefano Stabellini<stefano.stabellini@eu.citrix.com>
>> 
>> ---
>> 
>> diff -r 96a9740f4f33 tools/libxl/libxl_device.c
>> --- a/tools/libxl/libxl_device.c Wed Dec 02 14:29:24 2009 +0000
>> +++ b/tools/libxl/libxl_device.c Wed Dec 02 14:43:06 2009 +0000
>> @@ -212,47 +212,50 @@
>>       fd_set rfds;
>>       struct timeval tv;
>>       flexarray_t *toremove;
>> +    struct libxl_ctx clone = *ctx;
>> 
>> +    clone.xsh = xs_daemon_open();
>>       toremove = flexarray_make(16, 1);
>> -    path = libxl_sprintf(ctx, "/local/domain/%d/device", domid);
>> -    l1 = libxl_xs_directory(ctx, XBT_NULL, path,&num1);
>> +    path = libxl_sprintf(&clone, "/local/domain/%d/device", domid);
>> +    l1 = libxl_xs_directory(&clone, XBT_NULL, path,&num1);
>>       if (!l1) {
>> -        XL_LOG(ctx, XL_LOG_ERROR, "%s is empty", path);
>> +        XL_LOG(&clone, XL_LOG_ERROR, "%s is empty", path);
>> +        xs_daemon_close(clone.xsh);
>>           return -1;
>>       }
>>       for (i = 0; i<  num1; i++) {
>> -        path = libxl_sprintf(ctx, "/local/domain/%d/device/%s", domid,
>> l1[i]);
>> -        l2 = libxl_xs_directory(ctx, XBT_NULL, path,&num2);
>> +        path = libxl_sprintf(&clone, "/local/domain/%d/device/%s", domid,
>> l1[i]);
>> +        l2 = libxl_xs_directory(&clone, XBT_NULL, path,&num2);
>>           if (!l2)
>>               continue;
>>           for (j = 0; j<  num2; j++) {
>> -            fe_path = libxl_sprintf(ctx, "/local/domain/%d/device/%s/%s",
>> domid, l1[i], l2[j]);
>> -            be_path = libxl_xs_read(ctx, XBT_NULL, libxl_sprintf(ctx,
>> "%s/backend", fe_path));
>> +            fe_path = libxl_sprintf(&clone, "/local/domain/%d/device/%s/%s",
>> domid, l1[i], l2[j]);
>> +            be_path = libxl_xs_read(&clone, XBT_NULL, libxl_sprintf(&clone,
>> "%s/backend", fe_path));
>>               if (be_path != NULL) {
>> -                if (libxl_device_destroy(ctx, be_path, force)>  0)
>> +                if (libxl_device_destroy(&clone, be_path, force)>  0)
>>                       n_watches++;
>> -                flexarray_set(toremove, n++, libxl_dirname(ctx, be_path));
>> +                flexarray_set(toremove, n++, libxl_dirname(&clone,
>> be_path));
>>               } else {
>> -                xs_rm(ctx->xsh, XBT_NULL, path);
>> +                xs_rm(clone.xsh, XBT_NULL, path);
>>               }
>>           }
>>       }
>>       if (!force) {
>> -        nfds = xs_fileno(ctx->xsh) + 1;
>> +        nfds = xs_fileno(clone.xsh) + 1;
>>           /* Linux-ism */
>>           tv.tv_sec = LIBXL_DESTROY_TIMEOUT;
>>           tv.tv_usec = 0;
>>           while (n_watches>  0&&  tv.tv_sec>  0) {
>>               FD_ZERO(&rfds);
>> -            FD_SET(xs_fileno(ctx->xsh),&rfds);
>> +            FD_SET(xs_fileno(clone.xsh),&rfds);
>>               if (select(nfds,&rfds, NULL, NULL,&tv)>  0) {
>> -                l1 = xs_read_watch(ctx->xsh,&num1);
>> +                l1 = xs_read_watch(clone.xsh,&num1);
>>                   if (l1 != NULL) {
>> -                    char *state = libxl_xs_read(ctx, XBT_NULL, l1[0]);
>> +                    char *state = libxl_xs_read(&clone, XBT_NULL, l1[0]);
>>                       if (!state || atoi(state) == 6) {
>> -                        xs_unwatch(ctx->xsh, l1[0], l1[1]);
>> -                        xs_rm(ctx->xsh, XBT_NULL, l1[1]);
>> -                        XL_LOG(ctx, XL_LOG_DEBUG, "Destroyed device backend
>> at %s", l1[1]);
>> +                        xs_unwatch(clone.xsh, l1[0], l1[1]);
>> +                        xs_rm(clone.xsh, XBT_NULL, l1[1]);
>> +                        XL_LOG(&clone, XL_LOG_DEBUG, "Destroyed device
>> backend at %s", l1[1]);
>>                           n_watches--;
>>                       }
>>                       free(l1);
>> @@ -263,9 +266,10 @@
>>       }
>>       for (i = 0; i<  n; i++) {
>>           flexarray_get(toremove, i, (void**)&path);
>> -        xs_rm(ctx->xsh, XBT_NULL, path);
>> +        xs_rm(clone.xsh, XBT_NULL, path);
>>       }
>>       flexarray_free(toremove);
>> +    xs_daemon_close(clone.xsh);
>>       return 0;
>>   }
>> 
>>    
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

[PATCH] libxenlight: fix multiple xenstore watches problem

[Stefano Stabellini]

Hi all,
this patch fixes the multiple xenstore watches problem in libxenlight
opening a new xenstore connection to set and read temporary watches on
the device state nodes.
This way they don't interfere with other long running watches.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

---

diff -r 96a9740f4f33 tools/libxl/libxl_device.c
--- a/tools/libxl/libxl_device.c	Wed Dec 02 14:29:24 2009 +0000
+++ b/tools/libxl/libxl_device.c	Wed Dec 02 14:43:06 2009 +0000
@@ -212,47 +212,50 @@
     fd_set rfds;
     struct timeval tv;
     flexarray_t *toremove;
+    struct libxl_ctx clone = *ctx;
 
+    clone.xsh = xs_daemon_open();
     toremove = flexarray_make(16, 1);
-    path = libxl_sprintf(ctx, "/local/domain/%d/device", domid);
-    l1 = libxl_xs_directory(ctx, XBT_NULL, path, &num1);
+    path = libxl_sprintf(&clone, "/local/domain/%d/device", domid);
+    l1 = libxl_xs_directory(&clone, XBT_NULL, path, &num1);
     if (!l1) {
-        XL_LOG(ctx, XL_LOG_ERROR, "%s is empty", path);
+        XL_LOG(&clone, XL_LOG_ERROR, "%s is empty", path);
+        xs_daemon_close(clone.xsh);
         return -1;
     }
     for (i = 0; i < num1; i++) {
-        path = libxl_sprintf(ctx, "/local/domain/%d/device/%s", domid, l1[i]);
-        l2 = libxl_xs_directory(ctx, XBT_NULL, path, &num2);
+        path = libxl_sprintf(&clone, "/local/domain/%d/device/%s", domid, l1[i]);
+        l2 = libxl_xs_directory(&clone, XBT_NULL, path, &num2);
         if (!l2)
             continue;
         for (j = 0; j < num2; j++) {
-            fe_path = libxl_sprintf(ctx, "/local/domain/%d/device/%s/%s", domid, l1[i], l2[j]);
-            be_path = libxl_xs_read(ctx, XBT_NULL, libxl_sprintf(ctx, "%s/backend", fe_path));
+            fe_path = libxl_sprintf(&clone, "/local/domain/%d/device/%s/%s", domid, l1[i], l2[j]);
+            be_path = libxl_xs_read(&clone, XBT_NULL, libxl_sprintf(&clone, "%s/backend", fe_path));
             if (be_path != NULL) {
-                if (libxl_device_destroy(ctx, be_path, force) > 0)
+                if (libxl_device_destroy(&clone, be_path, force) > 0)
                     n_watches++;
-                flexarray_set(toremove, n++, libxl_dirname(ctx, be_path));
+                flexarray_set(toremove, n++, libxl_dirname(&clone, be_path));
             } else {
-                xs_rm(ctx->xsh, XBT_NULL, path);
+                xs_rm(clone.xsh, XBT_NULL, path);
             }
         }
     }
     if (!force) {
-        nfds = xs_fileno(ctx->xsh) + 1;
+        nfds = xs_fileno(clone.xsh) + 1;
         /* Linux-ism */
         tv.tv_sec = LIBXL_DESTROY_TIMEOUT;
         tv.tv_usec = 0;
         while (n_watches > 0 && tv.tv_sec > 0) {
             FD_ZERO(&rfds);
-            FD_SET(xs_fileno(ctx->xsh), &rfds);
+            FD_SET(xs_fileno(clone.xsh), &rfds);
             if (select(nfds, &rfds, NULL, NULL, &tv) > 0) {
-                l1 = xs_read_watch(ctx->xsh, &num1);
+                l1 = xs_read_watch(clone.xsh, &num1);
                 if (l1 != NULL) {
-                    char *state = libxl_xs_read(ctx, XBT_NULL, l1[0]);
+                    char *state = libxl_xs_read(&clone, XBT_NULL, l1[0]);
                     if (!state || atoi(state) == 6) {
-                        xs_unwatch(ctx->xsh, l1[0], l1[1]);
-                        xs_rm(ctx->xsh, XBT_NULL, l1[1]);
-                        XL_LOG(ctx, XL_LOG_DEBUG, "Destroyed device backend at %s", l1[1]);
+                        xs_unwatch(clone.xsh, l1[0], l1[1]);
+                        xs_rm(clone.xsh, XBT_NULL, l1[1]);
+                        XL_LOG(&clone, XL_LOG_DEBUG, "Destroyed device backend at %s", l1[1]);
                         n_watches--;
                     }
                     free(l1);
@@ -263,9 +266,10 @@
     }
     for (i = 0; i < n; i++) {
         flexarray_get(toremove, i, (void**) &path);
-        xs_rm(ctx->xsh, XBT_NULL, path);
+        xs_rm(clone.xsh, XBT_NULL, path);
     }
     flexarray_free(toremove);
+    xs_daemon_close(clone.xsh);
     return 0;
 }

Re: [PATCH] libxenlight: fix multiple xenstore watches problem

[Vincent Hanquez]

On Wed, Dec 02, 2009 at 03:01:32PM +0000, Stefano Stabellini wrote:
> Hi all,
> this patch fixes the multiple xenstore watches problem in libxenlight
> opening a new xenstore connection to set and read temporary watches on
> the device state nodes.
> This way they don't interfere with other long running watches.

This thing doesn't make sense; you're solving the wrong problem with cloning
the context. you can open a cheap xenstore connection for temporary watches and
be done with it.

-- 
Vincent

Re: [PATCH] libxenlight: fix multiple xenstore watches problem

[Stefano Stabellini]

On Thu, 3 Dec 2009, Vincent Hanquez wrote:
> On Wed, Dec 02, 2009 at 03:01:32PM +0000, Stefano Stabellini wrote:
> > Hi all,
> > this patch fixes the multiple xenstore watches problem in libxenlight
> > opening a new xenstore connection to set and read temporary watches on
> > the device state nodes.
> > This way they don't interfere with other long running watches.
> 
> This thing doesn't make sense; you're solving the wrong problem with cloning
> the context. you can open a cheap xenstore connection for temporary watches and
> be done with it.
> 

You are right about that, but Andres found the bug and fixed it with his
"clone context to avoid GC corruption" patch.
We can limit the memory allocated for the new alloc_ptrs to something
much smaller than 256 in libxl_clone_context if you are worried about
memory allocation.

Re: [PATCH] libxenlight: fix multiple xenstore watches problem

[Vincent Hanquez]

On Thu, Dec 03, 2009 at 03:39:57PM +0000, Stefano Stabellini wrote:
> You are right about that, but Andres found the bug and fixed it with his
> "clone context to avoid GC corruption" patch.

that's not about whether or not it can works. I think it's just confusing to
clone and share a context in a mono thread where the only thing you want out of
it, is a xenstore handle. You can simply clone a new xs handle, and worst case
scenario, you can extends the xl context to store 2 xs connections.

In the end, the approch is overkill, and complex solution often lead to
unforseen bug (at this was the case here)

> We can limit the memory allocated for the new alloc_ptrs to something
> much smaller than 256 in libxl_clone_context if you are worried about
> memory allocation.

definitely not worried about that.  libxenlight is not a library where
performance matters. (not that i'm saying it will be slow either)

-- 
Vincent

Re: [PATCH] libxenlight: fix multiple xenstore watches problem

[Stefano Stabellini]

On Thu, 3 Dec 2009, Vincent Hanquez wrote:
> On Thu, Dec 03, 2009 at 03:39:57PM +0000, Stefano Stabellini wrote:
> > You are right about that, but Andres found the bug and fixed it with his
> > "clone context to avoid GC corruption" patch.
> 
> that's not about whether or not it can works. I think it's just confusing to
> clone and share a context in a mono thread where the only thing you want out of
> it, is a xenstore handle. You can simply clone a new xs handle, and worst case
> scenario, you can extends the xl context to store 2 xs connections.
> 
> In the end, the approch is overkill, and complex solution often lead to
> unforseen bug (at this was the case here)
> 

All right then, I am OK with not cloning the whole context but just the
xenstore connection, however it would also be nice to be able to wrap
the connection opening in a nice function like Andres did with the
context.

What about the following as a fix to the cloning context problem, and as
an alternative to Andres' 0/7 patch:

---


diff -r 0f35fb4f73d6 tools/libxl/libxl.c
--- a/tools/libxl/libxl.c	Thu Dec 03 13:52:02 2009 +0000
+++ b/tools/libxl/libxl.c	Thu Dec 03 18:17:13 2009 +0000
@@ -654,22 +654,20 @@
 void dm_xenstore_record_pid(struct libxl_ctx *ctx, void *for_spawn,
                             pid_t innerchild) {
     struct libxl_device_model_starting *starting = for_spawn;
-    struct libxl_ctx clone;
     char *kvs[3];
     int rc;
 
-    clone = *ctx;
-    clone.xsh = xs_daemon_open();
+    libxl_ctx_open_xstemp(ctx);
     /* we mustn't use the parent's handle in the child */
 
     kvs[0] = "image/device-model-pid";
-    kvs[1] = libxl_sprintf(&clone, "%d", innerchild);
+    kvs[1] = libxl_sprintf(ctx, "%d", innerchild);
     kvs[2] = NULL;
-    rc = libxl_xs_writev(&clone, XBT_NULL, starting->dom_path, kvs);
-    if (rc) XL_LOG_ERRNO(&clone, XL_LOG_ERROR,
+    rc = libxl_xs_writev(ctx, XBT_NULL, starting->dom_path, kvs);
+    if (rc) XL_LOG_ERRNO(ctx, XL_LOG_ERROR,
                          "Couldn't record device model pid %ld at %s/%s",
                          (unsigned long)innerchild, starting->dom_path, kvs);
-    xs_daemon_close(clone.xsh);
+    libxl_ctx_close_xstemp(ctx);
 }
 
 static int libxl_vfb_and_vkb_from_device_model_info(struct libxl_ctx *ctx,
diff -r 0f35fb4f73d6 tools/libxl/libxl.h
--- a/tools/libxl/libxl.h	Thu Dec 03 13:52:02 2009 +0000
+++ b/tools/libxl/libxl.h	Thu Dec 03 18:17:13 2009 +0000
@@ -35,6 +35,8 @@
 struct libxl_ctx {
     int xch;
     struct xs_handle *xsh;
+    struct xs_handle *xsh_temp;
+
     /* errors/debug buf */
     void *log_userdata;
     libxl_log_callback log_callback;
diff -r 0f35fb4f73d6 tools/libxl/libxl_device.c
--- a/tools/libxl/libxl_device.c	Thu Dec 03 13:52:02 2009 +0000
+++ b/tools/libxl/libxl_device.c	Thu Dec 03 18:17:13 2009 +0000
@@ -212,50 +212,49 @@
     fd_set rfds;
     struct timeval tv;
     flexarray_t *toremove;
-    struct libxl_ctx clone = *ctx;
 
-    clone.xsh = xs_daemon_open();
+    libxl_ctx_open_xstemp(ctx);
     toremove = flexarray_make(16, 1);
-    path = libxl_sprintf(&clone, "/local/domain/%d/device", domid);
-    l1 = libxl_xs_directory(&clone, XBT_NULL, path, &num1);
+    path = libxl_sprintf(ctx, "/local/domain/%d/device", domid);
+    l1 = libxl_xs_directory(ctx, XBT_NULL, path, &num1);
     if (!l1) {
-        XL_LOG(&clone, XL_LOG_ERROR, "%s is empty", path);
-        xs_daemon_close(clone.xsh);
+        XL_LOG(ctx, XL_LOG_ERROR, "%s is empty", path);
+        libxl_ctx_close_xstemp(ctx);
         return -1;
     }
     for (i = 0; i < num1; i++) {
-        path = libxl_sprintf(&clone, "/local/domain/%d/device/%s", domid, l1[i]);
-        l2 = libxl_xs_directory(&clone, XBT_NULL, path, &num2);
+        path = libxl_sprintf(ctx, "/local/domain/%d/device/%s", domid, l1[i]);
+        l2 = libxl_xs_directory(ctx, XBT_NULL, path, &num2);
         if (!l2)
             continue;
         for (j = 0; j < num2; j++) {
-            fe_path = libxl_sprintf(&clone, "/local/domain/%d/device/%s/%s", domid, l1[i], l2[j]);
-            be_path = libxl_xs_read(&clone, XBT_NULL, libxl_sprintf(&clone, "%s/backend", fe_path));
+            fe_path = libxl_sprintf(ctx, "/local/domain/%d/device/%s/%s", domid, l1[i], l2[j]);
+            be_path = libxl_xs_read(ctx, XBT_NULL, libxl_sprintf(ctx, "%s/backend", fe_path));
             if (be_path != NULL) {
-                if (libxl_device_destroy(&clone, be_path, force) > 0)
+                if (libxl_device_destroy(ctx, be_path, force) > 0)
                     n_watches++;
-                flexarray_set(toremove, n++, libxl_dirname(&clone, be_path));
+                flexarray_set(toremove, n++, libxl_dirname(ctx, be_path));
             } else {
-                xs_rm(clone.xsh, XBT_NULL, path);
+                xs_rm(ctx->xsh, XBT_NULL, path);
             }
         }
     }
     if (!force) {
-        nfds = xs_fileno(clone.xsh) + 1;
+        nfds = xs_fileno(ctx->xsh) + 1;
         /* Linux-ism */
         tv.tv_sec = LIBXL_DESTROY_TIMEOUT;
         tv.tv_usec = 0;
         while (n_watches > 0 && tv.tv_sec > 0) {
             FD_ZERO(&rfds);
-            FD_SET(xs_fileno(clone.xsh), &rfds);
+            FD_SET(xs_fileno(ctx->xsh), &rfds);
             if (select(nfds, &rfds, NULL, NULL, &tv) > 0) {
-                l1 = xs_read_watch(clone.xsh, &num1);
+                l1 = xs_read_watch(ctx->xsh, &num1);
                 if (l1 != NULL) {
-                    char *state = libxl_xs_read(&clone, XBT_NULL, l1[0]);
+                    char *state = libxl_xs_read(ctx, XBT_NULL, l1[0]);
                     if (!state || atoi(state) == 6) {
-                        xs_unwatch(clone.xsh, l1[0], l1[1]);
-                        xs_rm(clone.xsh, XBT_NULL, l1[1]);
-                        XL_LOG(&clone, XL_LOG_DEBUG, "Destroyed device backend at %s", l1[1]);
+                        xs_unwatch(ctx->xsh, l1[0], l1[1]);
+                        xs_rm(ctx->xsh, XBT_NULL, l1[1]);
+                        XL_LOG(ctx, XL_LOG_DEBUG, "Destroyed device backend at %s", l1[1]);
                         n_watches--;
                     }
                     free(l1);
@@ -266,10 +265,10 @@
     }
     for (i = 0; i < n; i++) {
         flexarray_get(toremove, i, (void**) &path);
-        xs_rm(clone.xsh, XBT_NULL, path);
+        xs_rm(ctx->xsh, XBT_NULL, path);
     }
     flexarray_free(toremove);
-    xs_daemon_close(clone.xsh);
+    libxl_ctx_close_xstemp(ctx);
     return 0;
 }
 
diff -r 0f35fb4f73d6 tools/libxl/libxl_internal.c
--- a/tools/libxl/libxl_internal.c	Thu Dec 03 13:52:02 2009 +0000
+++ b/tools/libxl/libxl_internal.c	Thu Dec 03 18:17:13 2009 +0000
@@ -26,6 +26,24 @@
 int libxl_error_set(struct libxl_ctx *ctx, int code)
 {
     return 0;
+}
+
+int libxl_ctx_open_xstemp(struct libxl_ctx *ctx)
+{
+    if (ctx->xsh_temp)
+        return -1;
+    ctx->xsh_temp = ctx->xsh;
+    ctx->xsh = xs_daemon_open();
+    return 0;
+}
+
+int libxl_ctx_close_xstemp(struct libxl_ctx *ctx)
+{
+    if (!ctx->xsh_temp)
+        return -1;
+    xs_daemon_close(ctx->xsh);
+    ctx->xsh = ctx->xsh_temp;
+    ctx->xsh_temp = NULL;
 }
 
 int libxl_ptr_add(struct libxl_ctx *ctx, void *ptr)
diff -r 0f35fb4f73d6 tools/libxl/libxl_internal.h
--- a/tools/libxl/libxl_internal.h	Thu Dec 03 13:52:02 2009 +0000
+++ b/tools/libxl/libxl_internal.h	Thu Dec 03 18:17:13 2009 +0000
@@ -61,6 +61,10 @@
 #define PCI_BAR_IO             0x01
 
 #define PRINTF_ATTRIBUTE(x, y) __attribute__((format(printf, x, y)))
+
+/* open\close a seconday xenstore connection */
+int libxl_ctx_open_xstemp(struct libxl_ctx *ctx);
+int libxl_ctx_close_xstemp(struct libxl_ctx *ctx);
 
 /* memory allocation tracking/helpers */
 int libxl_ptr_add(struct libxl_ctx *ctx, void *ptr);

Re: [PATCH] libxenlight: fix multiple xenstore watches problem

[Stefano Stabellini]

On Thu, 3 Dec 2009, Stefano Stabellini wrote:
> On Thu, 3 Dec 2009, Vincent Hanquez wrote:
> > On Thu, Dec 03, 2009 at 03:39:57PM +0000, Stefano Stabellini wrote:
> > > You are right about that, but Andres found the bug and fixed it with his
> > > "clone context to avoid GC corruption" patch.
> > 
> > that's not about whether or not it can works. I think it's just confusing to
> > clone and share a context in a mono thread where the only thing you want out of
> > it, is a xenstore handle. You can simply clone a new xs handle, and worst case
> > scenario, you can extends the xl context to store 2 xs connections.
> > 
> > In the end, the approch is overkill, and complex solution often lead to
> > unforseen bug (at this was the case here)
> > 
> 
> All right then, I am OK with not cloning the whole context but just the
> xenstore connection, however it would also be nice to be able to wrap
> the connection opening in a nice function like Andres did with the
> context.
> 
> What about the following as a fix to the cloning context problem, and as
> an alternative to Andres' 0/7 patch:
> 

Actually I have just realized that this code is racy and a really bad
idea (think is someone else is trying to access ctx->xsh at the same time).

At this point unless you have a better suggestion I stand behind Andres'
0/7 patch.