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

["Luis R. Rodriguez" <mcgrof@xxxxxxxx>]

On Fri, Mar 21, 2014 at 03:21:10PM +0000, Ian Campbell wrote:
> 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.

docs/man/xenstore.pod.1 has no documentation yet on any arguments passed.
I can't see why this would be a requirement for -l if none of the others
are documented yet. Documenting only one argument would likely be pretty
confusing.

> > @@ -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.

--retries is perfect but the --recurse option takes on the -r smaller version
flag so I went with that. Let me know how you'd like to proceed. I'd prefer
to call it --socket-retries as that would remove ambiguation from ioctl()
open retries or transaaction retries, etc, but again, s is taken for socket
only mode.

> 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.

Technically no, we need to open the socket for any other mode of operation.

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

No, limit is an int, so it can be negative. That implies nr_watches
could be negative as it didn't have this check, I can add that if you like
as part of my series. Fortunately I see no issues with a negative
nr_watches though as in the loop that its used it will always be less
than 0, so a fix technically wouldn't do anything, but it would be correct
to add it.

> > +           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.

How so, I mean, if it can only succeed or not, and if it didn't I don't
see how retrying to open the same path would be incompatible?

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

Yeah that saves us an int.

>  and the adjustments inside the loop
> which this implies would be more natural I think.

Which adjustment? If you mean the branch check on if (limit > 1) -- that
is still needed otherwise we sleep(1) even if it worked.

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

  Luis

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