Re: [Xen-devel] [PATCH v2 1/7] xenstore-read: add support for a retry open limit on xenstored

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Wed, 2014-03-19 at 13:58 -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, each retry is separated by 1 second. This should allow
> us to simplify both our LSB init scripts and eventually our systemd
> service files for starting the xenstored.
> 
> 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>
> ---
>  tools/xenstore/xenstore_client.c | 31 +++++++++++++++++++++++++------

Please update docs/man/xenstore* too.

> @@ -535,10 +536,11 @@ main(int argc, char **argv)
>           {"upto",    0, 0, 'u'}, /* MODE_chmod */
>           {"recurse", 0, 0, 'r'}, /* MODE_chmod */
>           {"number",  1, 0, 'n'}, /* MODE_watch */
> +         {"limit",   1, 0, 'l'}, /* MODE_read */

Limit is an odd name for this options, --retries seems more plausible.

Since this only impacts the xc_open, is there any reason to limit it to
read? I might want to retry on any of the operations.

>           {0, 0, 0, 0}
>       };
>  
> -     c = getopt_long(argc - switch_argv, argv + switch_argv, "hfspturn:",
> +     c = getopt_long(argc - switch_argv, argv + switch_argv, "hfspturn:l:",
>                       long_options, &index);
>       if (c == -1)
>           break;
> @@ -589,6 +591,16 @@ main(int argc, char **argv)
>           else
>               usage(mode, switch_argv, argv[0]);
>           break;
> +     case 'l':
> +         if (mode == MODE_read)
> +             limit = atoi(optarg);
> +         else
> +             usage(mode, switch_argv, argv[0]);
> +         if (limit < 0) {
> +             limit = 1;

Pointless?

> +             usage(mode, switch_argv, argv[0]);
> +         }
> +         break;
>       }
>      }
>  
> @@ -632,8 +644,15 @@ main(int argc, char **argv)
>           max_width = ws.ws_col - 2;
>      }
>  
> -    xsh = xs_open(socket ? XS_OPEN_SOCKETONLY : 0);

It occurs to me that retrying might be incompatible with the ring base
(i.e. non-socket) method of talking to xenstored. Harmless to retry
though  I suppose.

> -    if (xsh == NULL) err(1, "xs_open");
> +    for (open_tries = 0; open_tries < limit; open_tries++) {

"while(limit--)" (well, retries--) and the adjustments inside the loop
which this implies would be more natural I think.

> +         xsh = xs_open(socket ? XS_OPEN_SOCKETONLY : 0);
> +         if (xsh)
> +                 break;
> +         if (limit > 1)
> +                 sleep(1);
> +    }
> +    if (!xsh)
> +         err(1, "xs_open");
>  
>  again:
>      if (transaction) {



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