On Thu, Mar 29, 2018 at 03:01:12PM +0200, Igor Mammedov wrote:
> On Wed, 28 Mar 2018 16:17:32 -0300
> Eduardo Habkost <ehabk...@redhat.com> 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 <ehabk...@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?
> 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.
Yeah, I now see that the initial state is PRECONFIG.
>
> > > > > { 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.
I wouldn't say that main_loop_should_exit() is easy to read, but
I understand that this is the existing style, so no objection.
>
>
> > > 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.
Well, if management software developers tell us that -preconfig
will be already useful without -incoming support, I won't object.
But it would be very nice for management software if they can
simply assume that -preconfig and -incoming will work together
since the first version. Can we have this as a goal for 2.13?
>
> > > > > + 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.
>
> [...]
>
--
Eduardo
