Re: [PATCH 4 of 5] tools: set migration constraints from cmdline

[Olaf Hering <olaf@aepfle.de>]

On Tue, Mar 12, Ian Campbell wrote:

> > -M <number>   Number of iterations before final suspend (default: 30)
> 
> Do the defaults here reflect the current behaviour or are they in
> themselves a change?

The numbers do not change. Currently 0 is passed into xc_domain_save,
which means that function will pick default values.

> > -A            Abort migration instead of doing final suspend.
>                                                              ... if <what>

If the number of iterations or the total amount of memory transfered is
exceeded. 
You are right, that knob needs a better description.


> > -int libxl_domain_suspend(libxl_ctx *ctx, uint32_t domid, int fd, int flags,
> > +int libxl_domain_suspend(libxl_ctx *ctx, uint32_t domid, int fd,
> > +                         const libxl_domain_suspend_properties *props,
> >                           const libxl_asyncop_how *ao_how)
> >  {
> >      AO_CREATE(ctx, domid, ao_how);
> > @@ -777,8 +778,13 @@ int libxl_domain_suspend(libxl_ctx *ctx,
> >      dss->domid = domid;
> >      dss->fd = fd;
> >      dss->type = type;
> > -    dss->live = flags & LIBXL_SUSPEND_LIVE;
> > -    dss->debug = flags & LIBXL_SUSPEND_DEBUG;
> > +    if (props) {
> > +        dss->live = props->flags & LIBXL_SUSPEND_LIVE;
> > +        dss->debug = props->flags & LIBXL_SUSPEND_DEBUG;
> > +        dss->max_iters = props->max_iters;
> > +        dss->max_factor = props->max_factor;
> > +        dss->xlflags = props->flags;
> > +    }
> 
> Do these things all get sane defaults if !props? Or is !props
> disallowed? (in which case error return required)

If no props given the migration will not be a live migration. Everything
else will get sane defaults. And the hvm flag is written somewhere down
the callchain.

> > 
> >      libxl__domain_suspend(egc, dss);
> >      return AO_INPROGRESS;
> > @@ -3769,13 +3778,17 @@ int main_migrate(int argc, char **argv)
> >      char *rune = NULL;
> >      char *host;
> >      int opt, daemonize = 1, monitor = 1, debug = 0;
> > +    int max_iters = 0, max_factor = 0, abort_if_busy = 0;
> >      static struct option opts[] = {
> >          {"debug", 0, 0, 0x100},
> > +        {"max_iters", 1, 0, 'M'},
> > +        {"max_factor", 1, 0, 'm'},
> 
> Wouldn't I or i and F or f be more descriptive than M and m (without
> looking, tell me which is which? ;-) )
> 
> I wouldn't especially object if these were long only options, I expect
> using them will be something only a tiny minority of users even think
> of, unless some higher level entity does it for them (in which case the
> length of the option is irrelevant). 

I can tweak this part and make it long-only options.

> > +      "-A, --abort_if_busy  Abort migration instead of doing final suspend."
> 
> As with other similar locations the text of -A needs to say under what
> conditions it aborts, I think.

I agree, will resend this patch with updated descriptions.

Olaf