[PATCH V2] tools/xenstore-watch: Add new timeout parameter

[PATCH V2] tools/xenstore-watch: Add new timeout parameter

[Razvan Cojocaru]

This patch allows xenstore-watch to exit even if no changes to its
XenStore key have occured in a specified interval (in seconds), via
a new -T parameter.

Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>

---
Changes since V1:
 - Exiting the timeout loop even on EINTR.
---
 tools/xenstore/xenstore_client.c | 64 ++++++++++++++++++++++++++++++----------
 1 file changed, 48 insertions(+), 16 deletions(-)

diff --git a/tools/xenstore/xenstore_client.c b/tools/xenstore/xenstore_client.c
index 3d14d37..6788158 100644
--- a/tools/xenstore/xenstore_client.c
+++ b/tools/xenstore/xenstore_client.c
@@ -12,6 +12,7 @@
 #include <errno.h>
 #include <fcntl.h>
 #include <getopt.h>
+#include <poll.h>
 #include <stdarg.h>
 #include <stdio.h>
 #include <stdlib.h>
@@ -99,7 +100,7 @@ usage(enum mode mode, int incl_mode, const char *progname)
 	errx(1, "Usage: %s %s[-h] [-u] [-r] [-s] key <mode [modes...]>", progname, mstr);
     case MODE_watch:
 	mstr = incl_mode ? "watch " : "";
-	errx(1, "Usage: %s %s[-h] [-n NR] key", progname, mstr);
+	errx(1, "Usage: %s %s[-h] [-n NR] [-T TIMEOUT] key", progname, mstr);
     }
 }
 
@@ -273,27 +274,49 @@ do_chmod(char *path, struct xs_permissions *perms, int nperms, int upto,
 }
 
 static void
-do_watch(struct xs_handle *xsh, int max_events)
+do_watch(struct xs_handle *xsh, int max_events, int timeout)
 {
-    int count = 0;
+    int rc, ms_timeout = timeout * 1000;
     char **vec = NULL;
+    struct pollfd fd;
 
-    for ( count = 0; max_events == -1 || count < max_events; count++ ) {
-	unsigned int num;
+    fd.fd = xs_fileno(xsh);
+    fd.events = POLLIN | POLLERR;
 
-	vec = xs_read_watch(xsh, &num);
-	if (vec == NULL)
-	    continue;
+    do {
+        rc = poll(&fd, 1, 100);
 
-	printf("%s\n", vec[XS_WATCH_PATH]);
-	fflush(stdout);
-	free(vec);
-    }
+        if (rc == 0) {
+            ms_timeout -= 100;
+            continue;
+        }
+
+        if (rc < 0) {
+            err(1, "poll() failed: %s", strerror(errno));
+            return;
+        }
+
+        if (fd.revents & POLLIN) {
+            unsigned int num;
+
+            --max_events;
+            vec = xs_read_watch(xsh, &num);
+
+            if (vec == NULL)
+                continue;
+
+            printf("%s\n", vec[XS_WATCH_PATH]);
+            fflush(stdout);
+            free(vec);
+        }
+
+    } while ((ms_timeout > 0 || timeout == -1) && max_events > 0);
 }
 
 static int
 perform(enum mode mode, int optind, int argc, char **argv, struct xs_handle *xsh,
-        xs_transaction_t xth, int prefix, int tidy, int upto, int recurse, int nr_watches)
+        xs_transaction_t xth, int prefix, int tidy, int upto, int recurse, int nr_watches,
+        int timeout)
 {
     switch (mode) {
     case MODE_ls:
@@ -461,7 +484,7 @@ perform(enum mode mode, int optind, int argc, char **argv, struct xs_handle *xsh
                 if (!xs_watch(xsh, w, w))
                     errx(1, "Unable to add watch on %s\n", w);
             }
-            do_watch(xsh, nr_watches);
+            do_watch(xsh, nr_watches, timeout);
         }
         }
     }
@@ -505,6 +528,7 @@ main(int argc, char **argv)
     int upto = 0;
     int recurse = 0;
     int nr_watches = -1;
+    int timeout = -1;
     int transaction;
     struct winsize ws;
     enum mode mode;
@@ -539,10 +563,11 @@ main(int argc, char **argv)
 	    {"upto",    0, 0, 'u'}, /* MODE_chmod */
 	    {"recurse", 0, 0, 'r'}, /* MODE_chmod */
 	    {"number",  1, 0, 'n'}, /* MODE_watch */
+	    {"timeout", 1, 0, 'T'}, /* MODE_watch */
 	    {0, 0, 0, 0}
 	};
 
-	c = getopt_long(argc - switch_argv, argv + switch_argv, "hfspturn:",
+	c = getopt_long(argc - switch_argv, argv + switch_argv, "hfspturn:T:",
 			long_options, &index);
 	if (c == -1)
 	    break;
@@ -593,6 +618,12 @@ main(int argc, char **argv)
 	    else
 		usage(mode, switch_argv, argv[0]);
 	    break;
+	case 'T':
+	    if ( mode == MODE_watch )
+		timeout = atoi(optarg);
+	    else
+		usage(mode, switch_argv, argv[0]);
+	    break;
 	}
     }
 
@@ -646,7 +677,8 @@ again:
 	    errx(1, "couldn't start transaction");
     }
 
-    ret = perform(mode, optind, argc - switch_argv, argv + switch_argv, xsh, xth, prefix, tidy, upto, recurse, nr_watches);
+    ret = perform(mode, optind, argc - switch_argv, argv + switch_argv, xsh, xth, prefix, tidy, upto, recurse,
+                  nr_watches, timeout);
 
     if (transaction && !xs_transaction_end(xsh, xth, ret)) {
 	if (ret == 0 && errno == EAGAIN) {
-- 
1.9.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH V2] tools/xenstore-watch: Add new timeout parameter

[Ian Jackson]

Razvan Cojocaru writes ("[PATCH V2] tools/xenstore-watch: Add new timeout parameter"):
> This patch allows xenstore-watch to exit even if no changes to its
> XenStore key have occured in a specified interval (in seconds), via
> a new -T parameter.

I realise I come a bit late with this, but:

I'm not opposed to making it possible to wait with a timeout for a
suitable situation in xenstore.

However, xenstore-watch needs to be run in a loop because of the
possibility of spurious watch triggers.  And this means that to use
your new option and get the timeout right, the caller would need to
write shell code to query the time, do arithmetic to calculate the
appropriate timeout, and so on.

Or to put it another way, xenstore-watch is a fine debugging and
monitoring tool but not very good for scripting.

For scripting what you probably want is something like:

  "wait up to T seconds for xenstore node /X/Y/Z
   to contain something matching PAT; bail immediately if
   /X/Y/Z contains something matching QAT".

How are you using this new option ?

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH V2] tools/xenstore-watch: Add new timeout parameter

[Razvan Cojocaru]

On 03/17/2016 06:39 PM, Ian Jackson wrote:
> Razvan Cojocaru writes ("[PATCH V2] tools/xenstore-watch: Add new timeout parameter"):
>> This patch allows xenstore-watch to exit even if no changes to its
>> XenStore key have occured in a specified interval (in seconds), via
>> a new -T parameter.
> 
> I realise I come a bit late with this, but:
> 
> I'm not opposed to making it possible to wait with a timeout for a
> suitable situation in xenstore.
> 
> However, xenstore-watch needs to be run in a loop because of the
> possibility of spurious watch triggers.  And this means that to use
> your new option and get the timeout right, the caller would need to
> write shell code to query the time, do arithmetic to calculate the
> appropriate timeout, and so on.
> 
> Or to put it another way, xenstore-watch is a fine debugging and
> monitoring tool but not very good for scripting.
> 
> For scripting what you probably want is something like:
> 
>   "wait up to T seconds for xenstore node /X/Y/Z
>    to contain something matching PAT; bail immediately if
>    /X/Y/Z contains something matching QAT".
> 
> How are you using this new option ?

I've actually ended up doing something very similar to what you're
describing, so this is no longer needed here, but I thought that it
might benefit someone in the community so I've carried on.

I basically wanted to be able to shut a program down via writing to a
XenStore key - the program acknowledges that it shut down by removing
the key, but it could also crash before it does that. So in that worst
case scenario, I didn't want xenstore-watch to wait forever. If you
don't feel that's a valid use for it I don't mind dropping the patch,
but I think it might benefit someone.


Thanks,
Razvan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel