On Wed, Mar 28, 2018 at 02:29:57PM +0200, Igor Mammedov wrote:
> On Fri, 23 Mar 2018 18:28:37 -0300
> Eduardo Habkost <ehabk...@redhat.com> wrote:
>
> > On Mon, Mar 12, 2018 at 02:11:11PM +0100, Igor Mammedov wrote:
> > > New option will be used to allow commands, which are prepared/need
> > > to run run in preconfig state. Other commands that should be able
> > > to run in preconfig state, should be ammeded to not expect machine
> > > in initialized state or deal with it.
> > >
> > > For compatibility reasons, commands, that don't use new flag
> > > 'allowed-in-preconfig' explicitly, are not permited to run in
> > > preconfig state but allowed in all other states like they used
> > > to be.
> > >
> > > Within this patch allow following commands in preconfig state:
> > > qmp_capabilities
> > > query-qmp-schema
> > > query-commands
> > > query-status
> > > cont
> > > to allow qmp connection, basic introspection and moving to the next
> > > state.
> > >
> > > PS:
> > > set-numa-node and query-hotpluggable-cpus will be enabled later in
> > > a separate patch.
> > >
> > > Signed-off-by: Igor Mammedov <imamm...@redhat.com>
> >
> > I didn't review the code yet, but:
> >
> > Shouldn't this be applied before patch 3/9, for bisectability?
> > Otherwise it will be very easy to crash QEMU after applying patch
> > 3/9.
> no, it isn't going to work.
> This patch depends on RUN_STATE_PRECONFIG that is introduced in 3/9.
>
> It could be fine to merge into 3/9 during merge, but then history
> wise it would be difficult to read it later with 2 big and mostly
> separate changes within one patch.
Yeah, I don't think squashing would be the right answer.
>
> Considering -preconfig if off by default it shouldn't affect
> bisectability in general so I'd keep current patch order.
Well, it would affect bisectability if debugging a crash that
happens using -preconfig.
The only hunk in this patch that really depends on patch 3/9
seems to be:
@@ -92,6 +93,13 @@ static QObject *do_qmp_dispatch(QmpCommandList *cmds,
QObject *request,
return NULL;
}
+ if (runstate_check(RUN_STATE_PRECONFIG) &&
+ !(cmd->options & QCO_ALLOWED_IN_PRECONFIG)) {
+ error_setg(errp, "The command '%s' isn't permitted in '%s' state",
+ cmd->name, RunState_str(RUN_STATE_PRECONFIG));
+ return NULL;
+ }
+
if (!qdict_haskey(dict, "arguments")) {
args = qdict_new();
} else {
What about moving it to patch 3/9?
Or, an alternative is to move the following hunk from patch 3/9 to this patch:
diff --git a/qapi/run-state.json b/qapi/run-state.json
index 1c9fff3aef..ce846a570e 100644
--- a/qapi/run-state.json
+++ b/qapi/run-state.json
@@ -49,12 +49,15 @@
# @colo: guest is paused to save/restore VM state under colo checkpoint,
# VM can not get into this state unless colo capability is enabled
# for migration. (since 2.8)
+# @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)
##
{ 'enum': 'RunState',
'data': [ 'debug', 'inmigrate', 'internal-error', 'io-error', 'paused',
'postmigrate', 'prelaunch', 'finish-migrate', 'restore-vm',
'running', 'save-vm', 'shutdown', 'suspended', 'watchdog',
- 'guest-panicked', 'colo' ] }
+ 'guest-panicked', 'colo', 'preconfig' ] }
##
# @StatusInfo:
Which could be an interesting idea, because the QAPI schema
changes would be all grouped inside a single patch, and then
followed by the actual implementation of the -preconfig option.
--
Eduardo
