Stefano Stabellini
2009-Dec-02 15:01 UTC
[Xen-devel] [PATCH] libxenlight: fix multiple xenstore watches problem
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
Andres Lagar-Cavilla
2009-Dec-02 18:08 UTC
[Xen-devel] Re: [PATCH] libxenlight: fix multiple xenstore watches problem
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
Keir Fraser
2009-Dec-02 18:26 UTC
Re: [Xen-devel] Re: [PATCH] libxenlight: fix multiple xenstore watches problem
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_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Stefano Stabellini
2009-Dec-03 15:39 UTC
Re: [Xen-devel] [PATCH] libxenlight: fix multiple xenstore watches problem
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. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Vincent Hanquez
2009-Dec-03 15:41 UTC
Re: [Xen-devel] [PATCH] libxenlight: fix multiple xenstore watches problem
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Vincent Hanquez
2009-Dec-03 17:08 UTC
Re: [Xen-devel] [PATCH] libxenlight: fix multiple xenstore watches problem
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Stefano Stabellini
2009-Dec-03 18:31 UTC
Re: [Xen-devel] [PATCH] libxenlight: fix multiple xenstore watches problem
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); _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Stefano Stabellini
2009-Dec-03 20:43 UTC
Re: [Xen-devel] [PATCH] libxenlight: fix multiple xenstore watches problem
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. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel