[PATCH] libxl: make libxl_wait_for_device_model not racy

[PATCH] libxl: make libxl_wait_for_device_model not racy

[Stefano Stabellini]

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);

Re: [PATCH] libxl: make libxl_wait_for_device_model not racy

[Vincent Hanquez]

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

Re: [PATCH] libxl: make libxl_wait_for_device_model not racy

[Stefano Stabellini]

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.

Re: [PATCH] libxl: make libxl_wait_for_device_model not racy

[Vincent Hanquez]

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

Re: [PATCH] libxl: make libxl_wait_for_device_model not racy

[Ian Jackson]

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.

Re: [PATCH] libxl: make libxl_wait_for_device_model not racy

[Jeremy Fitzhardinge]

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
>
>   

Re: [PATCH] libxl: make libxl_wait_for_device_model not racy

[Stefano Stabellini]

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.

Re: [PATCH] libxl: make libxl_wait_for_device_model not racy

[Jeremy Fitzhardinge]

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