On Wed, 28 Mar 2018 16:21:48 -0300
Eduardo Habkost <ehabk...@redhat.com> wrote:
> On Wed, Mar 28, 2018 at 01:48:35PM +0200, Igor Mammedov wrote:
> > On Tue, 27 Mar 2018 17:05:41 +0200
> > Igor Mammedov <imamm...@redhat.com> 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:
> > > [...]
> > [...]
> > > > > @@ -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.
> > > 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)
> > [...]
> >
> > > > > @@ -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.
> > Well doing as suggested end ups more messy:
> >
> > @@static bool main_loop_should_exit(void)
> > ...
> > if (preconfig_exit_requested) {
> > runstate_set(RUN_STATE_PRELAUNCH);
> >
> > return true;
> > }
> >
> > @@main
> > /* do monitor/qmp handling at preconfig state if requested */
> > if (!preconfig_exit_requested) {
> > main_loop();
> > } else if (runstate_check(RUN_STATE_PRECONFIG)) {
> > runstate_set(RUN_STATE_PRELAUNCH);
> > }
>
> This doesn't make sense to me. Why would we enter
> RUN_STATE_PRECONFIG state if -preconfig is not used at all?
because of RUN_STATE_PRECONFIG becomes new initial state of
our state machine where we start of (used to be RUN_STATE_PRELAUNCH)
Lets call it variant 1:
with this we have 2 possible transitions:
RUN_STATE_PRECONFIG -> RUN_STATE_PRELAUNCH (machine_init)
and
RUN_STATE_PRECONFIG -> RUN_STATE_INMIGRATE
ugly but it was the same with RUN_STATE_PRELAUNCH initial transition
Another variant 2, in case we switch to RUN_STATE_PRECONFIG only on -preconfig
transitions would be
RUN_STATE_PRELAUNCH -> RUN_STATE_PRECONFIG
(allow switch from initial to -preconfig)
RUN_STATE_PRECONFIG -> RUN_STATE_PRELAUNCH
while the last is valid transition, the 1st one isn't really
valid because of (beside of switching from initial state) it
allows bouncing back to RUN_STATE_PRECONFIG later.
If we consider only state machine transitions, I think it's
cleaner to start with variant 1 with the same
-inmigrate hack we already have (which potentially could
be fixed later), than allowing arbitrary bouncing to
RUN_STATE_PRECONFIG at later stage.
With this approach all processing before machine_init()
would run at RUN_STATE_PRECONFIG and then we would switch
to RUN_STATE_PRELAUNCH. Even though it is far reaching
goal but at least that's where we should be moving to
have sane initialization flow in vl.c
> > preconfig_exit_requested = false;
> > ...
> >
> > I'd prefer original v4 approach, where only main_loop_should_exit()
> > has to deal with state transitions and book-keeping.
>
> If the above is unavoidable, I agree. But I still don't
> understand we have to enter PRECONFIG state if the user didn't
> specify -preconfig.
>
