Re: [Qemu-devel] [PATCH v4 3/9] cli: add -preconfig option

[Igor Mammedov]

On Wed, 28 Mar 2018 16:17:32 -0300
Eduardo Habkost <address@hidden> wrote:

> On Tue, Mar 27, 2018 at 05:05:41PM +0200, Igor Mammedov wrote:
> > On Fri, 23 Mar 2018 18:25:08 -0300
> > Eduardo Habkost <address@hidden> wrote:
> >   
> > > On Mon, Mar 12, 2018 at 02:11:09PM +0100, Igor Mammedov wrote:  
> > [...]  
> > > > diff --git a/vl.c b/vl.c
> > > > index 3ef04ce..69b1997 100644
> > > > --- a/vl.c
> > > > +++ b/vl.c
> > > > @@ -593,7 +593,7 @@ static int default_driver_check(void *opaque, 
> > > > QemuOpts *opts, Error **errp)
> > > >  /***********************************************************/
> > > >  /* QEMU state */
> > > >  
> > > > -static RunState current_run_state = RUN_STATE_PRELAUNCH;
> > > > +static RunState current_run_state = RUN_STATE_PRECONFIG;
> > > >  
> > > >  /* We use RUN_STATE__MAX but any invalid value will do */
> > > >  static RunState vmstop_requested = RUN_STATE__MAX;
> > > > @@ -606,6 +606,9 @@ typedef struct {
> > > >  
> > > >  static const RunStateTransition runstate_transitions_def[] = {
> > > >      /*     from      ->     to      */
> > > > +    { RUN_STATE_PRECONFIG, RUN_STATE_PRELAUNCH },
> > > > +    { RUN_STATE_PRECONFIG, RUN_STATE_INMIGRATE },    
> > > 
> > > Don't this mean -preconfig and -incoming could work together?  
> > theoretically yes, but its not the reason why this transition is here.
> > It's mimicking existing approach where initial state
> >    { RUN_STATE_PRELAUNCH, RUN_STATE_INMIGRATE },
> > were allowed to move to the next possible (including RUN_STATE_INMIGRATE)  
> 
> I still don't get it.  Where this definition of "next possible"
> comes from?  If -incoming and -preconfig don't work together, why
> is PRECONFIG -> INMIGRATE migration considered possible?
I'd think it's the same (replacement) hack which we use now
   RUN_STATE_PRELAUNCH -> RUN_STATE_INMIGRATE
to allow following code to succeed:

      case QEMU_OPTION_incoming:
      if (!incoming) {                                                 
             runstate_set(RUN_STATE_INMIGRATE);                           
      }                                                                
      incoming = optarg;
 
I'd get rid of it and move state switching to the actual place
where migration starts if it were just that simple, but from
a quick look around it did look rather risky.
That's why I abandoned an idea of changing it within this series.

> > > >      { RUN_STATE_DEBUG, RUN_STATE_RUNNING },
> > > >      { RUN_STATE_DEBUG, RUN_STATE_FINISH_MIGRATE },
> > > >      { RUN_STATE_DEBUG, RUN_STATE_PRELAUNCH },
> > > > @@ -1629,6 +1632,7 @@ static pid_t shutdown_pid;
> > > >  static int powerdown_requested;
> > > >  static int debug_requested;
> > > >  static int suspend_requested;
> > > > +static bool preconfig_exit_requested = true;
> > > >  static WakeupReason wakeup_reason;
> > > >  static NotifierList powerdown_notifiers =
> > > >      NOTIFIER_LIST_INITIALIZER(powerdown_notifiers);
> > > > @@ -1713,6 +1717,11 @@ static int qemu_debug_requested(void)
> > > >      return r;
> > > >  }
> > > >  
> > > > +void qemu_exit_preconfig_request(void)
> > > > +{
> > > > +    preconfig_exit_requested = true;
> > > > +}
> > > > +
> > > >  /*
> > > >   * Reset the VM. Issue an event unless @reason is SHUTDOWN_CAUSE_NONE.
> > > >   */
> > > > @@ -1886,6 +1895,13 @@ static bool main_loop_should_exit(void)
> > > >      RunState r;
> > > >      ShutdownCause request;
> > > >  
> > > > +    if (preconfig_exit_requested) {
> > > > +        if (runstate_check(RUN_STATE_PRECONFIG)) {    
> > > 
> > > Is it possible to have preconfig_exit_request set outside of
> > > RUN_STATE_PRECONFIG?  When and why?  
> > preconfig_exit_requested is initialized with TRUE and
> > in combo with '-inmigrate' we need this runstate check.  
> 
> I think this now makes sense to me.  It still looks confusing,
> but I don't have a better suggestion right now.
> 
> Except...
> 
> Why exactly do you need to use main_loop() and
> main_loop_should_exit() for the preconfig loop?  What about a
> separate preconfig_loop() and preconfig_loop_should_exit()
> function?
that would duplicate main_loop() for practically no benefit at all,
hence I'm reusing existing main_loop()/main_loop_should_exit()
just by adding relevant exit condition. It also easier to read
when state transitions are kept close to each other.

 
> > it's the same as it was with
> >  { RUN_STATE_PRELAUNCH, RUN_STATE_INMIGRATE },
> > which I probably should remove (I need to check it though)
> >   
> > > > +            runstate_set(RUN_STATE_PRELAUNCH);
> > > > +        }
> > > > +        preconfig_exit_requested = false;  
> 
> What happens if we don't set preconfig_exit_requested=false here?
nothing should go wrong due to 'if (runstate_check(RUN_STATE_PRECONFIG))'
condition. It's the same what qemu_reset_requested()/qemu_shutdown_requested()
do with their respective request variables but not wrapped
into a separate function as it's the only place it's used.

 
> > > > +        return true;
> > > > +    }
> > > >      if (qemu_debug_requested()) {
> > > >          vm_stop(RUN_STATE_DEBUG);
> > > >      }
> > > > @@ -3697,6 +3713,14 @@ int main(int argc, char **argv, char **envp)
> > > >                      exit(1);
> > > >                  }
> > > >                  break;
> > > > +            case QEMU_OPTION_preconfig:
> > > > +                if (runstate_check(RUN_STATE_INMIGRATE)) {
> > > > +                    error_report("option can not be used with "
> > > > +                                 "-incoming option");
> > > > +                    exit(EXIT_FAILURE);
> > > > +                }    
> > > 
> > > So -incoming changes runstate as soon as the option is parsed?
> > > 
> > > Ouch.  
> > yep and it's rather fragile (it's well out of scope of
> > this series to re-factor this, so I'm not changing it here)
> >   
> > > I would rather not rely on that behavior and just do
> > > "if (incoming)".
> > > 
> > > Why exactly it's not possible to use -incoming with -preconfig?  
> > there are 2 reasons why I made options mutually exclusive
> > 1. (excuse ) '-incoming' is an option with non explicit side effects
> >    on other parts of code. It's hard to predict behavior
> >    of preconfig commands in combination with inmigrate.
> >    I wouldn't try to touch/change anything related to it
> >    in this series.
> >    If we need to change how option is handled, it should
> >    be separate series that focuses on it.
> > 2. (main reason) is to expose as minimal interface
> >    as possible. It's easier to extend/modify it future if
> >    necessary than cut it down after it was introduced.
> > 
> >    Not counting [1], I don't see a reason to permit
> >    'preconfig' while migration is in progress.
> >    Configuration commands that where used during 'preconfig'
> >    stage on source side, should use corresponding CLI options
> >    on target side. (it's the same behavior as with hotplugged
> >    devices, keeping migration work-flow the same)
> > 
> > In short I'd prefer to keep restriction until there will be
> > a real usecase for combo to work together.  
> 
> I understand the reasons, but I think we already have an
> important use case: live-migrating a VM with non-trivial NUMA
> config (that needs -preconfig).  Don't we?
Not really,
whatever we have configured on source side using -preconfig
(discovering valid topology in process), we should be able
to replicate using only CLI options on target since we
already have all necessary values for it from source (it's
certainly the case with this series set-numa-node command).

As for the future, I agree it would be much more flexible
to allow both -preconfig and -incoming at the same time,
so we could start target with empty CLI, and then feed it
options from source. It would require audit/refactoring of
INMIGRATE state and making 'all' current CLI options
available via QMP interface.

But for now I'd prefer to keep using old way to start target.

> > > > +                preconfig_exit_requested = false;
> > > > +                break;
> > > >              case QEMU_OPTION_enable_kvm:
> > > >                  olist = qemu_find_opts("machine");
> > > >                  qemu_opts_parse_noisily(olist, "accel=kvm", false);
> > > > @@ -3902,6 +3926,11 @@ int main(int argc, char **argv, char **envp)
> > > >                  }
> > > >                  break;
> > > >              case QEMU_OPTION_incoming:
> > > > +                if (!preconfig_exit_requested) {
> > > > +                    error_report("option can not be used with "
> > > > +                                 "-preconfig option");
> > > > +                    exit(EXIT_FAILURE);
> > > > +                }    
> > > 
> > > Instead of reimplementing the same check in two separate places,
> > > why not validate options and check for (incoming && preconfig)
> > > after the option parsing loop?  
> > it could be done this way, but then we would lose specialized
> > error message.
> > Even though the way I did it, it is more code but that code
> > is close to related options and allows for specialized error
> > message in the order options are parsed.  
> 
> What do you mean by specialized user message?  Both have exactly
> the same information: "-incoming and -preconfig can't be used
> together", just written in a different way.
[...]
> 
> I agree with the argument that validation of config options
> should be done all in the same place.  But I disagree that the
> body of the option parsing loop is the right place for that.
Ok, I'll move it out of loop as you suggested.

[...]