Stefano Stabellini
2010-Jun-23 11:15 UTC
[Xen-devel] [PATCH] libxl: make libxl_wait_for_device_model not racy
Hi all,
at the moment libxl_wait_for_device_model waits on a xenstore watch
before checking the current value of the xenstore node, that might
contain already the value the function was looking for.
This patch changes libxl_wait_for_device_model so that it checks the
value of the xenstore node first, then waits for the watch.
Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
diff -r e2f5e4f3481c tools/libxl/libxl_device.c
--- a/tools/libxl/libxl_device.c Tue Jun 22 16:22:30 2010 +0100
+++ b/tools/libxl/libxl_device.c Wed Jun 23 11:04:49 2010 +0100
@@ -418,7 +418,7 @@
char *path;
char *p;
unsigned int len;
- int rc;
+ int rc = 0;
struct xs_handle *xsh;
int nfds;
fd_set rfds;
@@ -432,28 +432,29 @@
tv.tv_sec = LIBXL_DEVICE_MODEL_START_TIMEOUT;
tv.tv_usec = 0;
nfds = xs_fileno(xsh) + 1;
- while (tv.tv_sec > 0) {
+ while (rc > 0 || (!rc && tv.tv_sec > 0)) {
+ p = xs_read(xsh, XBT_NULL, path, &len);
+ if (p && (!state || !strcmp(state, p))) {
+ free(p);
+ xs_unwatch(xsh, path, path);
+ xs_daemon_close(xsh);
+ if (check_callback) {
+ rc = check_callback(ctx, check_callback_userdata);
+ if (rc) return rc;
+ }
+ return 0;
+ }
+ free(p);
+again:
FD_ZERO(&rfds);
FD_SET(xs_fileno(xsh), &rfds);
- if (select(nfds, &rfds, NULL, NULL, &tv) > 0) {
+ rc = select(nfds, &rfds, NULL, NULL, &tv);
+ if (rc > 0) {
l = xs_read_watch(xsh, &num);
- if (l != NULL) {
+ if (l != NULL)
free(l);
- p = xs_read(xsh, XBT_NULL, path, &len);
- if (!p)
- continue;
- if (!state || !strcmp(state, p)) {
- free(p);
- xs_unwatch(xsh, path, path);
- xs_daemon_close(xsh);
- if (check_callback) {
- rc = check_callback(ctx, check_callback_userdata);
- if (rc) return rc;
- }
- return 0;
- }
- free(p);
- }
+ else
+ goto again;
}
}
xs_unwatch(xsh, path, path);
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Vincent Hanquez
2010-Jun-23 11:25 UTC
Re: [Xen-devel] [PATCH] libxl: make libxl_wait_for_device_model not racy
On 23/06/10 12:15, Stefano Stabellini wrote:> Hi all, > at the moment libxl_wait_for_device_model waits on a xenstore watch > before checking the current value of the xenstore node, that might > contain already the value the function was looking for. > This patch changes libxl_wait_for_device_model so that it checks the > value of the xenstore node first, then waits for the watch. > > Signed-off-by: Stefano Stabellini<stefano.stabellini@eu.citrix.com> >xenstore watch automatically fire one time when you install them for this exact same purpose. Is this patch actually solving any issue you saw ? -- Vincent _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Stefano Stabellini
2010-Jun-23 13:15 UTC
Re: [Xen-devel] [PATCH] libxl: make libxl_wait_for_device_model not racy
On Wed, 23 Jun 2010, Vincent Hanquez wrote:> On 23/06/10 12:15, Stefano Stabellini wrote: > > Hi all, > > at the moment libxl_wait_for_device_model waits on a xenstore watch > > before checking the current value of the xenstore node, that might > > contain already the value the function was looking for. > > This patch changes libxl_wait_for_device_model so that it checks the > > value of the xenstore node first, then waits for the watch. > > > > Signed-off-by: Stefano Stabellini<stefano.stabellini@eu.citrix.com> > > > xenstore watch automatically fire one time when you install them for > this exact same purpose. Is this patch actually solving any issue you saw ?No, it was just refactoring. Are you sure that both xenstore implementations always do that? Is this a "feature" we can safely rely on? If both answers are positive, then we can disregard this patch. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Vincent Hanquez
2010-Jun-23 13:36 UTC
Re: [Xen-devel] [PATCH] libxl: make libxl_wait_for_device_model not racy
On 23/06/10 14:15, Stefano Stabellini wrote:> No, it was just refactoring. > > Are you sure that both xenstore implementations always do that? >yes.> Is this a "feature" we can safely rely on? >yes. this feature has been here since xenstore has watches, for this exact purpose: not having to check before the watch fire that the values you''re expecting is there already. -- Vincent _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Jackson
2010-Jun-23 16:03 UTC
Re: [Xen-devel] [PATCH] libxl: make libxl_wait_for_device_model not racy
Vincent Hanquez writes ("Re: [Xen-devel] [PATCH] libxl: make
libxl_wait_for_device_model not racy"):> yes. this feature has been here since xenstore has watches, for this
> exact purpose: not having to check before the watch fire that the values
> you''re expecting is there already.
I think the new code is preferable because it''s clearer, even if we
never undo that xenstore wrinkle. (I agree that we probably can''t.)
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Jeremy Fitzhardinge
2010-Jun-23 16:50 UTC
Re: [Xen-devel] [PATCH] libxl: make libxl_wait_for_device_model not racy
On 06/23/2010 12:15 PM, Stefano Stabellini wrote:> Hi all, > at the moment libxl_wait_for_device_model waits on a xenstore watch > before checking the current value of the xenstore node, that might > contain already the value the function was looking for. > This patch changes libxl_wait_for_device_model so that it checks the > value of the xenstore node first, then waits for the watch. >That can''t help because it''s still racy: what if the value changes between the first check and the wait? The watch must fire immediately if the value is already in the desired state, or there''s an unavoidable deadlock. On the other hand, the check-then-wait pattern reads more clearly, I think. J> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> > > --- > > diff -r e2f5e4f3481c tools/libxl/libxl_device.c > --- a/tools/libxl/libxl_device.c Tue Jun 22 16:22:30 2010 +0100 > +++ b/tools/libxl/libxl_device.c Wed Jun 23 11:04:49 2010 +0100 > @@ -418,7 +418,7 @@ > char *path; > char *p; > unsigned int len; > - int rc; > + int rc = 0; > struct xs_handle *xsh; > int nfds; > fd_set rfds; > @@ -432,28 +432,29 @@ > tv.tv_sec = LIBXL_DEVICE_MODEL_START_TIMEOUT; > tv.tv_usec = 0; > nfds = xs_fileno(xsh) + 1; > - while (tv.tv_sec > 0) { > + while (rc > 0 || (!rc && tv.tv_sec > 0)) { > + p = xs_read(xsh, XBT_NULL, path, &len); > + if (p && (!state || !strcmp(state, p))) { > + free(p); > + xs_unwatch(xsh, path, path); > + xs_daemon_close(xsh); > + if (check_callback) { > + rc = check_callback(ctx, check_callback_userdata); > + if (rc) return rc; > + } > + return 0; > + } > + free(p); > +again: > FD_ZERO(&rfds); > FD_SET(xs_fileno(xsh), &rfds); > - if (select(nfds, &rfds, NULL, NULL, &tv) > 0) { > + rc = select(nfds, &rfds, NULL, NULL, &tv); > + if (rc > 0) { > l = xs_read_watch(xsh, &num); > - if (l != NULL) { > + if (l != NULL) > free(l); > - p = xs_read(xsh, XBT_NULL, path, &len); > - if (!p) > - continue; > - if (!state || !strcmp(state, p)) { > - free(p); > - xs_unwatch(xsh, path, path); > - xs_daemon_close(xsh); > - if (check_callback) { > - rc = check_callback(ctx, check_callback_userdata); > - if (rc) return rc; > - } > - return 0; > - } > - free(p); > - } > + else > + goto again; > } > } > xs_unwatch(xsh, path, path); > > _______________________________________________ > 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
2010-Jun-23 16:52 UTC
Re: [Xen-devel] [PATCH] libxl: make libxl_wait_for_device_model not racy
On Wed, 23 Jun 2010, Jeremy Fitzhardinge wrote:> On 06/23/2010 12:15 PM, Stefano Stabellini wrote: > > Hi all, > > at the moment libxl_wait_for_device_model waits on a xenstore watch > > before checking the current value of the xenstore node, that might > > contain already the value the function was looking for. > > This patch changes libxl_wait_for_device_model so that it checks the > > value of the xenstore node first, then waits for the watch. > > > > That can''t help because it''s still racy: what if the value changes > between the first check and the wait? The watch must fire immediately > if the value is already in the desired state, or there''s an unavoidable > deadlock.The check is done after the watch is set, so the wait would return immediately.> > On the other hand, the check-then-wait pattern reads more clearly, I think. >I agree on this. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jeremy Fitzhardinge
2010-Jun-23 16:54 UTC
Re: [Xen-devel] [PATCH] libxl: make libxl_wait_for_device_model not racy
On 06/23/2010 05:52 PM, Stefano Stabellini wrote:> The check is done after the watch is set, so the wait would return > immediately. >Yes, realized that just after sending. J _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel