Re: [Xen-devel] [PATCH v4 01/15] xenstore: add support for a retry open limit on xenstored

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Tue, 2014-04-29 at 18:11 -0700, Luis R. Rodriguez wrote:
> From: "Luis R. Rodriguez" <mcgrof@xxxxxxxx>
> 
> This adds support for a customizable retry limit on trying to open
> the xenstored unix domain socket, each retry is separated by 1 second.
> This should allow us to simplify both our LSB init scripts. For systemd
> we'll use socket activation.
> 
> Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> Cc: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
> Cc: Ian Campbell <ian.campbell@xxxxxxxxxx>
> Cc: Jan RÄkorajski <baggins@xxxxxxxxxxxxx>
> Cc: M A Young <m.a.young@xxxxxxxxxxxx>
> Cc: Jacek Konieczny <jajcus@xxxxxxxxxx>
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx
> Signed-off-by: Luis R. Rodriguez <mcgrof@xxxxxxxx>
> ---
>  docs/man/xenstore.pod.1          |  7 ++++++-
>  tools/xenstore/xenstore_client.c | 40 
> +++++++++++++++++++++++++++++-----------
>  2 files changed, 35 insertions(+), 12 deletions(-)
> 
> diff --git a/docs/man/xenstore.pod.1 b/docs/man/xenstore.pod.1
> index 7417289..9647143 100644
> --- a/docs/man/xenstore.pod.1
> +++ b/docs/man/xenstore.pod.1
> @@ -4,12 +4,17 @@ xenstore - interact with Xenstore
>  
>  =head1 SYNOPSIS
>  
> -B<xenstore> I<CMD> ...
> +B<xenstore> I<CMD> I[ --retries <num_open_retries> ] ...

I think "--retries <N>" is sufficient and less likely to push things up
to >80 columns. (Same in the errx output which is in main())
>  
>  =head1 DESCRIPTION

>  A set of utilities for interacting with Xenstore.

I think a bit more formatting can be applied here.

+ =head2 COMMON OPTIONS 
+ =over
+ =item B<--retries <N>>

> +All xenstore-* commands support the --retries long option which
> +can be used to try to opening the unix domain socket a certain
> +number of retries before giving up. Each new try occurs every
> +second.

"Retry opening the Unix domain socket once a second for I<N> attempts."

+ =back

+ =head2 COMMANDS
+ =over

(some playing with it in real life might be required).

> [...]
> @@ -493,9 +494,9 @@ static enum mode lookup_mode(const char *m)
>  int
>  main(int argc, char **argv)
>  {
> -    struct xs_handle *xsh;
> +    struct xs_handle *xsh = NULL;
>      xs_transaction_t xth = XBT_NULL;
> -    int ret = 0, socket = 0;
> +    int ret = 0, socket = 0, retries = 1;
>      int prefix = 0;
>      int tidy = 0;
>      int upto = 0;
> @@ -535,6 +536,7 @@ main(int argc, char **argv)
>           {"upto",    0, 0, 'u'}, /* MODE_chmod */
>           {"recurse", 0, 0, 'r'}, /* MODE_chmod */
>           {"number",  1, 0, 'n'}, /* MODE_watch */
> +         {"retries", 1, 0,  0 }, /* index 8 - MODE_* */


The usual way to deal with the lack of a short character is to use a
small non-zero integer unique to each long opt here and to switch on it.

So:

#define OPT_RETRIES 1 
...
{"retries", 1, 0, OPT_RETRIES },
...
>       switch (c) {
case OPT_RETIRES:
        retires = atoi(...)
        etc.

> @@ -632,8 +643,15 @@ main(int argc, char **argv)
>           max_width = ws.ws_col - 2;
>      }
>  
> -    xsh = xs_open(socket ? XS_OPEN_SOCKETONLY : 0);
> -    if (xsh == NULL) err(1, "xs_open");
> +    while (retries--) {
> +         xsh = xs_open(socket ? XS_OPEN_SOCKETONLY : 0);
> +         if (xsh)
> +                 break;
> +         if (retries)
> +                 sleep(1);

I think a do {} while() would remove the need for the if(retries) (IOW
sleep unconditionally, I don't think we need to worry unduly about an
extra 1s sleep after the last attempt)

> +    }
> +    if (!xsh)
> +         err(1, "xs_open");
>  
>  again:
>      if (transaction) {



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