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

[Eduardo Habkost <ehabkost@redhat.com>]

On Thu, Mar 29, 2018 at 01:43:03PM +0200, Igor Mammedov wrote:
> On Wed, 28 Mar 2018 16:21:48 -0300
> Eduardo Habkost <ehabkost@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 <imammedo@redhat.com> 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:    
> > > > [...]  
> > > [...]  
> > > > > > @@ -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)

Oh, I missed that part.

> 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

That explains a lot, thanks.

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

Thanks, now variant 1 makes more sense to me.  But I really miss
here are very clear and explicit descriptions of what each state
really mean, and what are the differences between them.

It looks like the existing description for `prelaunch` isn't
accurate:

# @prelaunch: QEMU was started with -S and guest has not started

This is false, as QEMU can be in `prelaunch` state even if -S is
not used.


Also, this is the description you proposed for `preconfig`:

# @preconfig: QEMU is paused before board specific init callback is executed.
#             The state is reachable only if -preconfig CLI option is used.
#             (Since 2.12)

This seems wrong as well: the `prelaunch` state is reachable even
if `-preconfig` isn't used in the command-line (because it is the
initial state).


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

-- 
Eduardo