On Mon, 4 Jun 2018 21:35:46 -0300
Eduardo Habkost <ehabk...@redhat.com> wrote:
> On Mon, Jun 04, 2018 at 07:37:15PM +0200, Igor Mammedov wrote:
> > On Mon, 4 Jun 2018 17:11:57 +0100
> > Daniel P. Berrangé <berra...@redhat.com> wrote:
> >
> > > On Mon, Jun 04, 2018 at 05:40:32PM +0200, Igor Mammedov wrote:
> > > > On Mon, 4 Jun 2018 13:03:44 +0100
> > > > Daniel P. Berrangé <berra...@redhat.com> wrote:
> > > >
> > > > > The RUN_STATE_PRECONFIG state is not supposed to be reachable unless
> > > > > the
> > > > > --preconfig argument is given to QEMU, but when it was introduced in:
> > > > >
> > > > > commit 047f7038f586d2150f16c6d9ba9cfd0479f0f6ac
> > > > > Author: Igor Mammedov <imamm...@redhat.com>
> > > > > Date: Fri May 11 19:24:43 2018 +0200
> > > > >
> > > > > cli: add --preconfig option
> > > > >
> > > > > ...the global 'current_run_state' variable was changed to have an
> > > > > initial
> > > > > value of RUN_STATE_PRECONFIG regardless of whether --preconfig is
> > > > > given.
> > > > >
> > > > > A second invokation of main_loop() was added which then toggles it
> > > > > back
> > > > > to RUN_STATE_PRELAUNCH when --preconfig is not given. This is racy
> > > > > because it leaves a window where QEMU is in RUN_STATE_PRECONFIG
> > > > > despite
> > > > > --preconfig not being given.
> > > > Above statements isn't exactly correct, PRECONFIG were supposed to be
> > > > the new state of QEMU from start up till machine_run_board_init(),
> > > > that would separate stage of initial configuration out from all
> > > > encompassing PRELAUNCH state. So I'd scratch out above part.
> > >
> > > That doesn't really make sense, given that --preconfig is *not* the
> > > default and thus not supposed to be an active state unless the app
> > > has explicitly opted in.
> > >
> > > IMHO running PRECONFIG state for any period of time when the app
> > > has not requested --preconfig is simply broken, and a recipe for
> > > obscure bugs like the ones we've seen right now.
> > mgmt hasn't opted in for default PRELAUNCH either, for it default
> > PRECONFIG runstate is not visible unless it opts in so nothing
> > is broken in regards to this.
> > Default runstate selection is QEMU's internal impl. detail.
>
> Daniel's description is how he expects it to work, but your
> description reflects the way the state machine actually works
> today (and how it worked befor the --preconfig series).
>
> However, I agree with Daniel that moving to PRECONFIG or
> PRELAUNCH if neither --preconfig or -S were specified is
> confusing, and I would prefer to change it the way he suggests.
>
> But:
>
> [...]
> > > $START -------------> PRELAUNCH {-> INMIGRATE]
> > > | ^
> > > | |
> > > +-- PRECONFIG -+
> > >
> > > By marking the current state as PRECONFIG by default, we've essentially
> > > given 2 meanings to PRECONFIG - sometimes it means stuff that can be
> > > unconditionally run in early startup, and sometimes it means stuff that
> > > can only be run if --preconfig is given. Introducing the separate NONE
> > > state clarifies that inherant contradiction in startup phases.
> > Yep, one can say it this way (as merged PRECONFIG was early
> > configuration stage regardless of if it's unconditional early
> > initialization or early CLI parsing/QMP configuration).
> >
> > Maybe adding NONE state makes sense but I'm not quite sure,
> > that's why I'd not rush it in and discuss if we really need
> > fragment early configuration into more stages.
> > (we can do it later as both stages aren't visible to user by default).
>
> Is it possible to fix the bugs first, and discuss how to refactor
> the state machine later?
Agreed, we can discuss and settle this internal to QEMU implementation
detail independently on top of actual fix.
> In the meantime, we could even document preconfig more accurately
> to avoid additional confusion:
>
> # @preconfig: Board initialization was not run yet. The state is
> # visible to the outside only if the --preconfig CLI
> # option is used. (Since 3.0)
I'll post a proper patch with it.
