Re: [Qemu-devel] [PATCH v3 5/9] QAPI: allow to specify valid runstates per command
On Wed, 7 Mar 2018 14:16:50 +
"Dr. David Alan Gilbert" wrote:
> * Igor Mammedov (imamm...@redhat.com) wrote:
> > Add optional 'runstates' parameter in QAPI command definition,
> > which will permit to specify RunState variations in which
> > a command could be exectuted via QMP monitor.
> >
> > For compatibility reasons, commands, that don't use
> > 'runstates' explicitly, are not permited to run in
> > preconfig state but allowed in all other states.
> >
> > New option will be used to allow commands, which are
> > prepared/need to run this early, to run in preconfig state.
> > It will include query-hotpluggable-cpus and new set-numa-node
> > commands. Other commands that should be able to run in
> > preconfig state, should be ammeded to not expect machine
> > in initialized state.
> >
> > Signed-off-by: Igor Mammedov
> > ---
[...]
> > @@ -224,6 +224,25 @@ static mon_cmd_t mon_cmds[];
> > static mon_cmd_t info_cmds[];
> >
> > QmpCommandList qmp_commands, qmp_cap_negotiation_commands;
> > +const RunState cap_negotiation_valid_runstates[] = {
> > +RUN_STATE_DEBUG,
> > +RUN_STATE_INMIGRATE,
> > +RUN_STATE_INTERNAL_ERROR,
> > +RUN_STATE_IO_ERROR,
> > +RUN_STATE_PAUSED,
> > +RUN_STATE_POSTMIGRATE,
> > +RUN_STATE_PRELAUNCH,
> > +RUN_STATE_FINISH_MIGRATE,
> > +RUN_STATE_RESTORE_VM,
> > +RUN_STATE_RUNNING,
> > +RUN_STATE_SAVE_VM,
> > +RUN_STATE_SHUTDOWN,
> > +RUN_STATE_SUSPENDED,
> > +RUN_STATE_WATCHDOG,
> > +RUN_STATE_GUEST_PANICKED,
> > +RUN_STATE_COLO,
> > +RUN_STATE_PRECONFIG,
> > +};
>
> It's a shame this is such a manual copy.
>
> While you're taking a big hammer to HMP in the preconfig case,
> it's worth noting this more specific code is only protecting QMP
> commands.
>
>
> It's a shame in all the uses below we can't be working with bitmasks
> of run-state's; it would feel a lot easier to have a default and mask
> out or or in extra states.
>
> Dave
[...]
> > @@ -219,7 +219,11 @@
> > # Note: This example has been shortened as the real response is too long.
> > #
> > ##
> > -{ 'command': 'query-commands', 'returns': ['CommandInfo'] }
> > +{ 'command': 'query-commands', 'returns': ['CommandInfo'],
> > + 'runstates': [ 'debug', 'inmigrate', 'internal-error', 'io-error',
> > 'paused',
> > + 'postmigrate', 'prelaunch', 'finish-migrate',
> > 'restore-vm',
> > + 'running', 'save-vm', 'shutdown', 'suspended', 'watchdog',
> > + 'guest-panicked', 'colo', 'preconfig' ] }
>
> That's going to get to be a pain to update as we add more states.
[...]
Yep it would be a pain so,
In v4 this approach/patch is replaced by a simpler one that Eric's suggested.
It will provide preconfig specific flag in command, similar to what allow-oob
patches on list do. So it would look like following:
docs/devel/qapi-code-gen.txt -
index 25b7180..170f15f 100644
@@ -556,7 +556,8 @@ following example objects:
Usage: { 'command': STRING, '*data': COMPLEX-TYPE-NAME-OR-DICT,
'*returns': TYPE-NAME, '*boxed': true,
- '*gen': false, '*success-response': false }
+ '*gen': false, '*success-response': false,
+ '*allowed-in-preconfig': true }
Commands are defined by using a dictionary containing several members,
where three members are most common. The 'command' member is a
@@ -636,6 +637,13 @@ possible, the command expression should include the
optional key
'success-response' with boolean value false. So far, only QGA makes
use of this member.
+A command may use optional 'allowed-in-preconfig' key to permit
+its execution at early runtime configuration stage (preconfig runstate).
+If not specified then a command defaults to 'allowed-in-preconfig: false'.
+
+An example of declaring preconfig enabled command:
+ { 'command': 'qmp_capabilities',
+ 'allowed-in-preconfig': true }
Re: [Qemu-devel] [PATCH v3 5/9] QAPI: allow to specify valid runstates per command
* Igor Mammedov (imamm...@redhat.com) wrote:
> Add optional 'runstates' parameter in QAPI command definition,
> which will permit to specify RunState variations in which
> a command could be exectuted via QMP monitor.
>
> For compatibility reasons, commands, that don't use
> 'runstates' explicitly, are not permited to run in
> preconfig state but allowed in all other states.
>
> New option will be used to allow commands, which are
> prepared/need to run this early, to run in preconfig state.
> It will include query-hotpluggable-cpus and new set-numa-node
> commands. Other commands that should be able to run in
> preconfig state, should be ammeded to not expect machine
> in initialized state.
>
> Signed-off-by: Igor Mammedov
> ---
> include/qapi/qmp/dispatch.h | 5 +++-
> monitor.c | 28 +---
> qapi-schema.json| 12 +++--
> qapi/qmp-dispatch.c | 39
> qapi/qmp-registry.c | 4 ++-
> qapi/run-state.json | 6 -
> scripts/qapi-commands.py| 46
> ++---
> scripts/qapi-introspect.py | 2 +-
> scripts/qapi.py | 15 +++
> scripts/qapi2texi.py| 2 +-
> tests/qapi-schema/doc-good.out | 4 +--
> tests/qapi-schema/ident-with-escape.out | 2 +-
> tests/qapi-schema/indented-expr.out | 4 +--
> tests/qapi-schema/qapi-schema-test.out | 18 ++---
> tests/qapi-schema/test-qapi.py | 6 ++---
> 15 files changed, 151 insertions(+), 42 deletions(-)
>
> diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h
> index 1e694b5..f821781 100644
> --- a/include/qapi/qmp/dispatch.h
> +++ b/include/qapi/qmp/dispatch.h
> @@ -15,6 +15,7 @@
> #define QAPI_QMP_DISPATCH_H
>
> #include "qemu/queue.h"
> +#include "qapi-types.h"
>
> typedef void (QmpCommandFunc)(QDict *, QObject **, Error **);
>
> @@ -31,12 +32,14 @@ typedef struct QmpCommand
> QmpCommandOptions options;
> QTAILQ_ENTRY(QmpCommand) node;
> bool enabled;
> +const RunState *valid_runstates;
> } QmpCommand;
>
> typedef QTAILQ_HEAD(QmpCommandList, QmpCommand) QmpCommandList;
>
> void qmp_register_command(QmpCommandList *cmds, const char *name,
> - QmpCommandFunc *fn, QmpCommandOptions options);
> + QmpCommandFunc *fn, QmpCommandOptions options,
> + const RunState valid_runstates[]);
> void qmp_unregister_command(QmpCommandList *cmds, const char *name);
> QmpCommand *qmp_find_command(QmpCommandList *cmds, const char *name);
> QObject *qmp_dispatch(QmpCommandList *cmds, QObject *request);
> diff --git a/monitor.c b/monitor.c
> index fcb5386..2edc96c 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -224,6 +224,25 @@ static mon_cmd_t mon_cmds[];
> static mon_cmd_t info_cmds[];
>
> QmpCommandList qmp_commands, qmp_cap_negotiation_commands;
> +const RunState cap_negotiation_valid_runstates[] = {
> +RUN_STATE_DEBUG,
> +RUN_STATE_INMIGRATE,
> +RUN_STATE_INTERNAL_ERROR,
> +RUN_STATE_IO_ERROR,
> +RUN_STATE_PAUSED,
> +RUN_STATE_POSTMIGRATE,
> +RUN_STATE_PRELAUNCH,
> +RUN_STATE_FINISH_MIGRATE,
> +RUN_STATE_RESTORE_VM,
> +RUN_STATE_RUNNING,
> +RUN_STATE_SAVE_VM,
> +RUN_STATE_SHUTDOWN,
> +RUN_STATE_SUSPENDED,
> +RUN_STATE_WATCHDOG,
> +RUN_STATE_GUEST_PANICKED,
> +RUN_STATE_COLO,
> +RUN_STATE_PRECONFIG,
> +};
It's a shame this is such a manual copy.
While you're taking a big hammer to HMP in the preconfig case,
it's worth noting this more specific code is only protecting QMP
commands.
It's a shame in all the uses below we can't be working with bitmasks
of run-state's; it would feel a lot easier to have a default and mask
out or or in extra states.
Dave
> Monitor *cur_mon;
>
> @@ -1016,17 +1035,18 @@ void monitor_init_qmp_commands(void)
>
> qmp_register_command(&qmp_commands, "query-qmp-schema",
> qmp_query_qmp_schema,
> - QCO_NO_OPTIONS);
> + QCO_NO_OPTIONS, NULL);
> qmp_register_command(&qmp_commands, "device_add", qmp_device_add,
> - QCO_NO_OPTIONS);
> + QCO_NO_OPTIONS, NULL);
> qmp_register_command(&qmp_commands, "netdev_add", qmp_netdev_add,
> - QCO_NO_OPTIONS);
> + QCO_NO_OPTIONS, NULL);
>
> qmp_unregister_commands_hack();
>
> QTAILQ_INIT(&qmp_cap_negotiation_commands);
> qmp_register_command(&qmp_cap_negotiation_commands, "qmp_capabilities",
> - qmp_marshal_qmp_capabilities, QCO_NO_OPTIONS);
> + qmp_marshal_qmp_capabilities, QCO_NO_OPTIONS,
> + cap_negotiation_valid_runstates);
> }
>
>
Re: [Qemu-devel] [PATCH v3 5/9] QAPI: allow to specify valid runstates per command
On Tue, 27 Feb 2018 16:10:31 -0600 Eric Blake wrote: > On 02/16/2018 06:37 AM, Igor Mammedov wrote: > > Add optional 'runstates' parameter in QAPI command definition, > > which will permit to specify RunState variations in which > > a command could be exectuted via QMP monitor. > > s/exectuted/executed/ > > > > > For compatibility reasons, commands, that don't use > > s/commands,/commands/ > > > 'runstates' explicitly, are not permited to run in > > s/explicitly,/explicitly/ > s/permited/permitted/ > > > preconfig state but allowed in all other states. > > > > New option will be used to allow commands, which are > > prepared/need to run this early, to run in preconfig state. > > It will include query-hotpluggable-cpus and new set-numa-node > > commands. Other commands that should be able to run in > > preconfig state, should be ammeded to not expect machine > > s/ammeded/amended/ > > > in initialized state. > > > > Signed-off-by: Igor Mammedov > > --- > > include/qapi/qmp/dispatch.h | 5 +++- > > monitor.c | 28 +--- > > qapi-schema.json| 12 +++-- > > qapi/qmp-dispatch.c | 39 > > qapi/qmp-registry.c | 4 ++- > > qapi/run-state.json | 6 - > > scripts/qapi-commands.py| 46 > > ++--- > > scripts/qapi-introspect.py | 2 +- > > scripts/qapi.py | 15 +++ > > scripts/qapi2texi.py| 2 +- > > tests/qapi-schema/doc-good.out | 4 +-- > > tests/qapi-schema/ident-with-escape.out | 2 +- > > tests/qapi-schema/indented-expr.out | 4 +-- > > tests/qapi-schema/qapi-schema-test.out | 18 ++--- > > tests/qapi-schema/test-qapi.py | 6 ++--- > > 15 files changed, 151 insertions(+), 42 deletions(-) > [...] > Overall, this is adding a lot of complexity to QMP; are we sure this is > the interface libvirt wants to use for early NUMA configuration? Can we > simplify it to just a new bool, similar to 'allow-oob'? Being unsure if preconfig only specific knob would fly, I was trying to introduce a generic mechanism that could be used to limit any command to certain runstates. I'd gladly to scrape it off and implement something like 'allow-oob' if that's acceptable. It could be "preconfig-enabled" which defaults to false and necessary commands would be explicitly marked as preconfig enabled. Then I could just build into 'set-numa-node' C callback an extra check to limit it only to preconfig state, like we typically do in other handlers instead of going for more complex generic approach. That would significantly simplify series.
Re: [Qemu-devel] [PATCH v3 5/9] QAPI: allow to specify valid runstates per command
On 02/16/2018 06:37 AM, Igor Mammedov wrote:
Add optional 'runstates' parameter in QAPI command definition,
which will permit to specify RunState variations in which
a command could be exectuted via QMP monitor.
s/exectuted/executed/
For compatibility reasons, commands, that don't use
s/commands,/commands/
'runstates' explicitly, are not permited to run in
s/explicitly,/explicitly/
s/permited/permitted/
preconfig state but allowed in all other states.
New option will be used to allow commands, which are
prepared/need to run this early, to run in preconfig state.
It will include query-hotpluggable-cpus and new set-numa-node
commands. Other commands that should be able to run in
preconfig state, should be ammeded to not expect machine
s/ammeded/amended/
in initialized state.
Signed-off-by: Igor Mammedov
---
include/qapi/qmp/dispatch.h | 5 +++-
monitor.c | 28 +---
qapi-schema.json| 12 +++--
qapi/qmp-dispatch.c | 39
qapi/qmp-registry.c | 4 ++-
qapi/run-state.json | 6 -
scripts/qapi-commands.py| 46 ++---
scripts/qapi-introspect.py | 2 +-
scripts/qapi.py | 15 +++
scripts/qapi2texi.py| 2 +-
tests/qapi-schema/doc-good.out | 4 +--
tests/qapi-schema/ident-with-escape.out | 2 +-
tests/qapi-schema/indented-expr.out | 4 +--
tests/qapi-schema/qapi-schema-test.out | 18 ++---
tests/qapi-schema/test-qapi.py | 6 ++---
15 files changed, 151 insertions(+), 42 deletions(-)
Missing mention in docs/; among other things, see how the OOB series
adds a similar per-command witness during QMP on whether the command can
be used in certain contexts:
https://lists.gnu.org/archive/html/qemu-devel/2018-01/msg05789.html
including edits to docs/devel/qapi-code-gen.txt (definitely needed here)
and docs/interop/qmp-spec.txt (may or may not be needed here).
diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h
index 1e694b5..f821781 100644
--- a/include/qapi/qmp/dispatch.h
+++ b/include/qapi/qmp/dispatch.h
@@ -15,6 +15,7 @@
#define QAPI_QMP_DISPATCH_H
#include "qemu/queue.h"
+#include "qapi-types.h"
Probably conflict with the pending work from Markus to reorganize the
QAPI header files to be more modular.
+++ b/qapi-schema.json
@@ -219,7 +219,11 @@
# Note: This example has been shortened as the real response is too long.
#
##
-{ 'command': 'query-commands', 'returns': ['CommandInfo'] }
+{ 'command': 'query-commands', 'returns': ['CommandInfo'],
+ 'runstates': [ 'debug', 'inmigrate', 'internal-error', 'io-error', 'paused',
+ 'postmigrate', 'prelaunch', 'finish-migrate', 'restore-vm',
+ 'running', 'save-vm', 'shutdown', 'suspended', 'watchdog',
+ 'guest-panicked', 'colo', 'preconfig' ] }
Wow, that's going to be a lot of states to list for every command that
is interested in the non-default state. Would a simple bool flag be any
easier than a list of states, since it looks like preconfig is the only
special state?
##
# @LostTickPolicy:
@@ -1146,7 +1150,11 @@
# <- { "return": {} }
#
##
-{ 'command': 'cont' }
+{ 'command': 'cont',
+ 'runstates': [ 'debug', 'inmigrate', 'internal-error', 'io-error', 'paused',
+ 'postmigrate', 'prelaunch', 'finish-migrate', 'restore-vm',
+ 'running', 'save-vm', 'shutdown', 'suspended', 'watchdog',
+ 'guest-panicked', 'colo', 'preconfig' ] }
Should 'stop' also be permitted in the preconfig state, to get to the
state that used to be provided by 'qemu -S'?
@@ -65,6 +66,40 @@ static QDict *qmp_dispatch_check_obj(const QObject *request,
Error **errp)
return dict;
}
+static bool is_cmd_permited(const QmpCommand *cmd, Error **errp)
s/permited/permitted/g
+{
+int i;
+char *list = NULL;
+
+/* Old commands that don't have explicit runstate in schema
+ * are permited to run except of at PRECONFIG stage
including in the comments
+ */
+if (!cmd->valid_runstates) {
+if (runstate_check(RUN_STATE_PRECONFIG)) {
+const char *state = RunState_str(RUN_STATE_PRECONFIG);
+error_setg(errp, "The command '%s' isn't valid in '%s'",
+ cmd->name, state);
+return false;
+}
+return true;
+}
+
+for (i = 0; cmd->valid_runstates[i] != RUN_STATE__MAX; i++) {
+if (runstate_check(cmd->valid_runstates[i])) {
+return true;
+}
+}
+
+for (i = 0; cmd->valid_runstates[i] != RUN_STATE__MAX; i++) {
+const char *state = RunState_str(cmd->valid_runstates[i]);
+list = g_strjoin(", ", state, list, NULL);
This is rather complex compare
[Qemu-devel] [PATCH v3 5/9] QAPI: allow to specify valid runstates per command
Add optional 'runstates' parameter in QAPI command definition,
which will permit to specify RunState variations in which
a command could be exectuted via QMP monitor.
For compatibility reasons, commands, that don't use
'runstates' explicitly, are not permited to run in
preconfig state but allowed in all other states.
New option will be used to allow commands, which are
prepared/need to run this early, to run in preconfig state.
It will include query-hotpluggable-cpus and new set-numa-node
commands. Other commands that should be able to run in
preconfig state, should be ammeded to not expect machine
in initialized state.
Signed-off-by: Igor Mammedov
---
include/qapi/qmp/dispatch.h | 5 +++-
monitor.c | 28 +---
qapi-schema.json| 12 +++--
qapi/qmp-dispatch.c | 39
qapi/qmp-registry.c | 4 ++-
qapi/run-state.json | 6 -
scripts/qapi-commands.py| 46 ++---
scripts/qapi-introspect.py | 2 +-
scripts/qapi.py | 15 +++
scripts/qapi2texi.py| 2 +-
tests/qapi-schema/doc-good.out | 4 +--
tests/qapi-schema/ident-with-escape.out | 2 +-
tests/qapi-schema/indented-expr.out | 4 +--
tests/qapi-schema/qapi-schema-test.out | 18 ++---
tests/qapi-schema/test-qapi.py | 6 ++---
15 files changed, 151 insertions(+), 42 deletions(-)
diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h
index 1e694b5..f821781 100644
--- a/include/qapi/qmp/dispatch.h
+++ b/include/qapi/qmp/dispatch.h
@@ -15,6 +15,7 @@
#define QAPI_QMP_DISPATCH_H
#include "qemu/queue.h"
+#include "qapi-types.h"
typedef void (QmpCommandFunc)(QDict *, QObject **, Error **);
@@ -31,12 +32,14 @@ typedef struct QmpCommand
QmpCommandOptions options;
QTAILQ_ENTRY(QmpCommand) node;
bool enabled;
+const RunState *valid_runstates;
} QmpCommand;
typedef QTAILQ_HEAD(QmpCommandList, QmpCommand) QmpCommandList;
void qmp_register_command(QmpCommandList *cmds, const char *name,
- QmpCommandFunc *fn, QmpCommandOptions options);
+ QmpCommandFunc *fn, QmpCommandOptions options,
+ const RunState valid_runstates[]);
void qmp_unregister_command(QmpCommandList *cmds, const char *name);
QmpCommand *qmp_find_command(QmpCommandList *cmds, const char *name);
QObject *qmp_dispatch(QmpCommandList *cmds, QObject *request);
diff --git a/monitor.c b/monitor.c
index fcb5386..2edc96c 100644
--- a/monitor.c
+++ b/monitor.c
@@ -224,6 +224,25 @@ static mon_cmd_t mon_cmds[];
static mon_cmd_t info_cmds[];
QmpCommandList qmp_commands, qmp_cap_negotiation_commands;
+const RunState cap_negotiation_valid_runstates[] = {
+RUN_STATE_DEBUG,
+RUN_STATE_INMIGRATE,
+RUN_STATE_INTERNAL_ERROR,
+RUN_STATE_IO_ERROR,
+RUN_STATE_PAUSED,
+RUN_STATE_POSTMIGRATE,
+RUN_STATE_PRELAUNCH,
+RUN_STATE_FINISH_MIGRATE,
+RUN_STATE_RESTORE_VM,
+RUN_STATE_RUNNING,
+RUN_STATE_SAVE_VM,
+RUN_STATE_SHUTDOWN,
+RUN_STATE_SUSPENDED,
+RUN_STATE_WATCHDOG,
+RUN_STATE_GUEST_PANICKED,
+RUN_STATE_COLO,
+RUN_STATE_PRECONFIG,
+};
Monitor *cur_mon;
@@ -1016,17 +1035,18 @@ void monitor_init_qmp_commands(void)
qmp_register_command(&qmp_commands, "query-qmp-schema",
qmp_query_qmp_schema,
- QCO_NO_OPTIONS);
+ QCO_NO_OPTIONS, NULL);
qmp_register_command(&qmp_commands, "device_add", qmp_device_add,
- QCO_NO_OPTIONS);
+ QCO_NO_OPTIONS, NULL);
qmp_register_command(&qmp_commands, "netdev_add", qmp_netdev_add,
- QCO_NO_OPTIONS);
+ QCO_NO_OPTIONS, NULL);
qmp_unregister_commands_hack();
QTAILQ_INIT(&qmp_cap_negotiation_commands);
qmp_register_command(&qmp_cap_negotiation_commands, "qmp_capabilities",
- qmp_marshal_qmp_capabilities, QCO_NO_OPTIONS);
+ qmp_marshal_qmp_capabilities, QCO_NO_OPTIONS,
+ cap_negotiation_valid_runstates);
}
void qmp_qmp_capabilities(Error **errp)
diff --git a/qapi-schema.json b/qapi-schema.json
index 0262b9f..ee1053d 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -219,7 +219,11 @@
# Note: This example has been shortened as the real response is too long.
#
##
-{ 'command': 'query-commands', 'returns': ['CommandInfo'] }
+{ 'command': 'query-commands', 'returns': ['CommandInfo'],
+ 'runstates': [ 'debug', 'inmigrate', 'internal-error', 'io-error', 'paused',
+ 'postmigrate', 'prelaunch', 'finish-migrate', 'restore-vm',
+ 'running', 'save-vm', 'shutdown', 'suspended', 'watchdog',
+
