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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Sat, 2014-03-22 at 02:33 +0100, Luis R. Rodriguez wrote:
> 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.

We have to start somewhere and the policy is to try not to make things
worse.

(obviously if you want to document some of the other options then feel
free, it would be most appreciated, but I'll only insist on the new one)

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

I think it would be fine to have this option be a long-option only one,
its mainly for a pretty specific use case.

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

I meant the assignment, not the if. Since the next thing you do is call
usage() which always exits.

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

IIRC opening (or maybe using) the ring based interface while the daemon
isn't running will hang not fail. But retrying is rather moot in that
case.

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

I meant any necessary adjustments implied by changing from a for over
open_tries to a while over limit, maybe there aren't any, or maybe that
if (limit) sleep needs adjusting I wasn't sure.

Ian.


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