diff mbox

tools/xenstore-watch: Add new timeout parameter

Message ID 1458143446-18346-1-git-send-email-rcojocaru@bitdefender.com (mailing list archive)
State New, archived
Headers show

Commit Message

Razvan Cojocaru March 16, 2016, 3:50 p.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 <rcojocaru@bitdefender.com>
---
 tools/xenstore/xenstore_client.c | 64 ++++++++++++++++++++++++++++++----------
 1 file changed, 48 insertions(+), 16 deletions(-)

Comments

Konrad Rzeszutek Wilk March 16, 2016, 5:43 p.m. UTC | #1 
On Wed, Mar 16, 2016 at 05:50:46PM +0200, Razvan Cojocaru wrote:
> 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.

Should there be an update to the manpage as well?

> 
> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
> ---
>  tools/xenstore/xenstore_client.c | 64 ++++++++++++++++++++++++++++++----------
>  1 file changed, 48 insertions(+), 16 deletions(-)
>
Andrew Cooper March 16, 2016, 5:50 p.m. UTC | #2 
On 16/03/16 17:43, Konrad Rzeszutek Wilk wrote:
> On Wed, Mar 16, 2016 at 05:50:46PM +0200, Razvan Cojocaru wrote:
>> 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.
> Should there be an update to the manpage as well?

There are currently isn't a manpage covering xenstore-watch, and the
more general xenstore(1) refers to the non-existent xenstore-watch(1).

Introducing one would be great.  xenstore-ls looks like a good example.

~Andrew
Wei Liu March 16, 2016, 6:46 p.m. UTC | #3 
On Wed, Mar 16, 2016 at 05:50:46PM +0200, Razvan Cojocaru wrote:
[...]
>  }
>  
> @@ -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 || (rc < 0 && errno == EINTR)) {
> +            ms_timeout -= 100;

Shouldn't you just exit the loop when getting EINTR?

And as others have mentioned, please also patch manpage.

Wei.
Razvan Cojocaru March 16, 2016, 6:52 p.m. UTC | #4 
On 03/16/2016 08:46 PM, Wei Liu wrote:
> On Wed, Mar 16, 2016 at 05:50:46PM +0200, Razvan Cojocaru wrote:
> [...]
>>  }
>>  
>> @@ -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 || (rc < 0 && errno == EINTR)) {
>> +            ms_timeout -= 100;
> 
> Shouldn't you just exit the loop when getting EINTR?

I don't think so, an EINTR means that a signal has been caught during
poll() - it's not really a failure case.

> And as others have mentioned, please also patch manpage.

As Andrew has pointed out, there is no man page to patch. I agree that
adding one would be great, but I hope this patch can get through without
it being required - it can be added later.


Thanks,
Razvan
Wei Liu March 16, 2016, 6:57 p.m. UTC | #5 
On Wed, Mar 16, 2016 at 08:52:57PM +0200, Razvan Cojocaru wrote:
> On 03/16/2016 08:46 PM, Wei Liu wrote:
> > On Wed, Mar 16, 2016 at 05:50:46PM +0200, Razvan Cojocaru wrote:
> > [...]
> >>  }
> >>  
> >> @@ -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 || (rc < 0 && errno == EINTR)) {
> >> +            ms_timeout -= 100;
> > 
> > Shouldn't you just exit the loop when getting EINTR?
> 
> I don't think so, an EINTR means that a signal has been caught during
> poll() - it's not really a failure case.
> 

What about Ctrl-C ? I think it is reasonable that user want to interrupt
the process.

> > And as others have mentioned, please also patch manpage.
> 
> As Andrew has pointed out, there is no man page to patch. I agree that
> adding one would be great, but I hope this patch can get through without
> it being required - it can be added later.
> 

Yes, follow-up patch is OK.

Wei.

> 
> Thanks,
> Razvan
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
Razvan Cojocaru March 16, 2016, 7:06 p.m. UTC | #6 
On 03/16/2016 08:57 PM, Wei Liu wrote:
> On Wed, Mar 16, 2016 at 08:52:57PM +0200, Razvan Cojocaru wrote:
>> On 03/16/2016 08:46 PM, Wei Liu wrote:
>>> On Wed, Mar 16, 2016 at 05:50:46PM +0200, Razvan Cojocaru wrote:
>>> [...]
>>>>  }
>>>>  
>>>> @@ -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 || (rc < 0 && errno == EINTR)) {
>>>> +            ms_timeout -= 100;
>>>
>>> Shouldn't you just exit the loop when getting EINTR?
>>
>> I don't think so, an EINTR means that a signal has been caught during
>> poll() - it's not really a failure case.
>>
> 
> What about Ctrl-C ? I think it is reasonable that user want to interrupt
> the process.

Fair enough, I'll re-spin V2.


Thanks,
Razvan
diff mbox

Patch

diff --git a/tools/xenstore/xenstore_client.c b/tools/xenstore/xenstore_client.c
index 3d14d37..2873b71 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 || (rc < 0 && errno == EINTR)) {
+            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) {