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

[Eduardo Habkost <ehabkost@redhat.com>]

On Tue, Mar 27, 2018 at 05:05:41PM +0200, Igor Mammedov wrote:
> On Fri, 23 Mar 2018 18:25:08 -0300
> Eduardo Habkost <ehabkost@redhat.com> 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?


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


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


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


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


> Also it's easier to read as one doesn't have to jump around,
> all error handling is in place where where an option is parsed.
> But it's more style question, so if you prefer
> (incoming && preconfig) approach I can easily switch to it
> on respin.

I would prefer that.  We already have lots of configuration
validation after the option parsing loop, including but not
limited to:

    error_report("Invalid SMP CPUs %d. The min CPUs "
                 "supported by machine '%s' is %d", smp_cpus,
                 machine_class->name, machine_class->min_cpus);
    error_report("Invalid SMP CPUs %d. The max CPUs "
                 "supported by machine '%s' is %d", max_cpus,
                 machine_class->name, machine_class->max_cpus);
    error_report("-nographic cannot be used with -daemonize");
    error_report("curses display cannot be used with -daemonize");
    error_report("-no-frame, -alt-grab and -ctrl-grab are only valid "
                 "for SDL, ignoring option");
    error_report("-no-quit is only valid for GTK and SDL, "
                 "ignoring option");
    error_report("OpenGL is not supported by the display");
    error_report("OpenGL support is disabled");
    error_report("-append only allowed with -kernel option");
    error_report("-initrd only allowed with -kernel option");
    error_report("-icount is not allowed with hardware virtualization");
    error_report("at most 2047 MB RAM can be simulated");


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.

> 
> > >                  if (!incoming) {
> > >                      runstate_set(RUN_STATE_INMIGRATE);
> > >                  }
> > > @@ -4594,6 +4623,10 @@ int main(int argc, char **argv, char **envp)
> > >      }
> > >      parse_numa_opts(current_machine);
> > >  
> > > +    /* do monitor/qmp handling at preconfig state if requested */
> > > +    main_loop();  
> > 
> > Wouldn't it be simpler to do "if (!preconfig) { main_loop(); }"
> > instead of entering main_loop() just to exit immediately?
> The thought didn't cross my mind, it might work and more readable
> as one doesn't have to jump into main_loop() to find out that
> it would exit immediately.
> I'll try to it on respin.

Thanks!

> 
> > > +
> > > +    /* from here on runstate is RUN_STATE_PRELAUNCH */
> > >      machine_run_board_init(current_machine);
> > >  
> > >      realtime_init();
> > > -- 
> > > 2.7.4
> > > 
> > >   
> > 
> 

-- 
Eduardo