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

Commit Message

Razvan Cojocaru March 17, 2016, 8:55 a.m. UTC

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 <[e-mail]>

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

Comments

Ian Jackson March 17, 2016, 4:39 p.m. UTC | #1

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.

Razvan Cojocaru March 17, 2016, 4:46 p.m. UTC | #2

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

Patch

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