monitor: Wean core off QError, and other cleanups
cover monitor: Wean core off QError, and other cleanups
Command handlers still use QError. Left for another day.
Markus Armbruster (20):
monitor: Drop broken, unused asynchronous command interface
monitor: Clean up after previous commit
monitor: Improve and document client_migrate_info protocol error
monitor: Convert client_migrate_info to QAPI
monitor: Use traditional command interface for HMP drive_del
monitor: Use traditional command interface for HMP device_add
monitor: Use trad. command interface for HMP pcie_aer_inject_error
monitor: Drop unused "new" HMP command interface
monitor: Propagate errors through qmp_check_client_args()
monitor: Propagate errors through qmp_check_input_obj()
monitor: Wean monitor_protocol_emitter() off mon->error
monitor: Inline monitor_has_error() into its only caller
monitor: Limit QError use to command handlers
monitor: Rename handle_user_command() to handle_hmp_command()
monitor: Rename monitor_control_read(), monitor_control_event()
monitor: Unbox Monitor member mc and rename to qmp
monitor: Drop do_qmp_capabilities()'s superfluous QMP check
monitor: Turn int command_mode into bool in_command_mode
monitor: Rename monitor_ctrl_mode() to monitor_is_qmp()
monitor: Change return type of monitor_cur_is_qmp() to bool
blockdev.c | 9 +- hmp-commands.hx | 13 +- hmp.c | 23 +++ hmp.h | 2 + hw/pci/pci-stub.c | 14 +- hw/pci/pcie_aer.c | 39 ++--- include/monitor/monitor.h | 7 +- include/sysemu/blockdev.h | 2 +- include/sysemu/sysemu.h | 4 +- monitor.c | 378 ++++++++++++++++------------------------------ qapi-schema.json | 20 +++ qmp-commands.hx | 5 +- stubs/mon-is-qmp.c | 2 +- 13 files changed, 214 insertions(+), 304 deletions(-)
Signature · 1 line
1.9.3
PATCH 1/20 monitor: Drop broken, unused asynchronous command interface
The asynchronous monitor command interface goes back to commit 940cc30 (Jan 2010). Added a third case to command execution. The hope back then according to the commit message was that all commands get converted to the asynchronous interface, killing off the other two cases. Didn't happen.
The initial asynchronous commands balloon and info balloon were converted back to synchronous long ago (commit 96637bc and d72f32), with commit messages calling the asynchronous interface "not fully working" and "deprecated". The only other user went away in commit 3b5704b.
New code generally uses synchronous commands and asynchronous events.
What exactly is still "not fully working" with asynchronous commands? Well, here's a bug that defeats actual asynchronous use pretty
reliably: the reply's ID is wrong (and has always been wrong) unless you use the command synchronously! To reproduce, we need an asynchronous command, so we have to go back before the commit 3b5704b. Run QEMU with spice:
$ qemu-system-x86_64 -nodefaults -S -spice port=5900,disable-ticketing -qmp stdio
{"QMP": {"version": {"qemu": {"micro": 50, "minor": 2, "major":
2}, "package": ""}, "capabilities": []}}Connect a spice client in another terminal:
$ remote-viewer spice://localhost:5900
Set up a migration destination dummy in a third terminal:
$ socat TCP-LISTEN:12345 STDIO
Now paste the following into the QMP monitor:
{ "execute": "qmp_capabilities" }
{ "execute": "client_migrate_info", "id": "i1", "arguments": { "protocol": "spice", "hostname": "localhost", "port": 12345 } }
{ "execute": "query-kvm", "id": "i2" }Produces two replies immediately, one to qmp_capabilities, and one to query-kvm:
{"return": {}}
{"return": {"enabled": false, "present": true}, "id": "i2"}Both are correct. Two lines of debug output from libspice-server not shown.
Now EOF socat's standard input to make it close the connection. This makes the asynchronous client_migrate_info complete. It replies:
{"return": {}}Bug: "id": "i1" is missing. Two lines of debug output from libspice-server not shown. Cherry on top: storage for the missing ID is leaked.
Get rid of this stuff before somebody hurts himself with it.
--- include/monitor/monitor.h | 5 ---- monitor.c | 68 ++--------------------------------------------- 2 files changed, 2 insertions(+), 71 deletions(-)
include/monitor/monitor.h +0 -5
@@ -16,9 +16,6 @@ extern Monitor *default_mon;#define MONITOR_USE_CONTROL 0x04#define MONITOR_USE_PRETTY 0x08 -/* flags for monitor commands */ -#define MONITOR_CMD_ASYNC 0x0001 -int monitor_cur_is_qmp (void );void monitor_init (CharDriverState *chr,int flags);
@@ -43,8 +40,6 @@ void monitor_flush(Monitor *mon);int monitor_set_cpu (int cpu_index);int monitor_get_cpu_index (void ); -typedef void (MonitorCompletion )(void *opaque,QObject *ret_data); -void monitor_set_error (Monitor *mon,QError *qerror);void monitor_read_command (Monitor *mon,int show_prompt); int monitor_read_password(Monitor *mon, ReadLineFunc *readline_func,
monitor.c +2 -66
105 lines not on this page · raw message
Signature · 1 line
1.9.3
Re: [Qemu-devel] [PATCH 01/20] monitor: Drop broken, unused asynchronous command interface
Quoted 11 lines · Markus Armbruster
The asynchronous monitor command interface goes back to commit 940cc30 (Jan 2010). Added a third case to command execution. The hope back then according to the commit message was that all commands get converted to the asynchronous interface, killing off the other two cases. Didn't happen.
The initial asynchronous commands balloon and info balloon were converted back to synchronous long ago (commit 96637bc and d72f32), with commit messages calling the asynchronous interface "not fully working" and "deprecated". The only other user went away in commit 3b5704b.
The history is helpful; thanks.
Quoted 18 lines
New code generally uses synchronous commands and asynchronous events.
What exactly is still "not fully working" with asynchronous commands? Well, here's a bug that defeats actual asynchronous use pretty
reliably: the reply's ID is wrong (and has always been wrong) unless you use the command synchronously! To reproduce, we need an asynchronous command, so we have to go back before the commit 3b5704b. Run QEMU with spice:$ qemu-system-x86_64 -nodefaults -S -spice port=5900,disable-ticketing -qmp stdio {"QMP": {"version": {"qemu": {"micro": 50, "minor": 2, "major": 2}, "package": ""}, "capabilities": []}}Connect a spice client in another terminal:
$ remote-viewer spice://localhost:5900
Quoted 7 lines
Set up a migration destination dummy in a third terminal:
$ socat TCP-LISTEN:12345 STDIONow paste the following into the QMP monitor:
{ "execute": "qmp_capabilities" }
If you stick and "id":"i0" here...
Quoted 7 lines
{ "execute": "client_migrate_info", "id": "i1", "arguments": { "protocol": "spice", "hostname": "localhost", "port": 12345 } } { "execute": "query-kvm", "id": "i2" }Produces two replies immediately, one to
qmp_capabilities, and one to query-kvm:{"return": {}}
...it should show up here, for even less ambiguity about the problems with the async response.
Quoted 14 lines
{"return": {"enabled": false, "present": true}, "id": "i2"}Both are correct. Two lines of debug output from libspice-server not shown.
Now EOF socat's standard input to make it close the connection. This makes the asynchronous
client_migrate_infocomplete. It replies:{"return": {}}Bug: "id": "i1" is missing. Two lines of debug output from libspice-server not shown. Cherry on top: storage for the missing ID is leaked.
Quoted 7 lines
Get rid of this stuff before somebody hurts himself with it.
Signed-off-by Markus Armbruster <[email protected]>--- include/monitor/monitor.h | 5 ---- monitor.c | 68 ++--------------------------------------------- 2 files changed, 2 insertions(+), 71 deletions(-)
Certainly takes an axe to it!
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Re: [Qemu-devel] [PATCH 01/20] monitor: Drop broken, unused asynchronous command interface
Quoted 48 lines · Eric Blake
Quoted 11 lines · Markus Armbruster
The asynchronous monitor command interface goes back to commit 940cc30 (Jan 2010). Added a third case to command execution. The hope back then according to the commit message was that all commands get converted to the asynchronous interface, killing off the other two cases. Didn't happen.
The initial asynchronous commands balloon and info balloon were converted back to synchronous long ago (commit 96637bc and d72f32), with commit messages calling the asynchronous interface "not fully working" and "deprecated". The only other user went away in commit 3b5704b.
The history is helpful; thanks.
Quoted 19 lines
New code generally uses synchronous commands and asynchronous events.
What exactly is still "not fully working" with asynchronous commands? Well, here's a bug that defeats actual asynchronous use pretty
reliably: the reply's ID is wrong (and has always been wrong) unless you use the command synchronously! To reproduce, we need an asynchronous command, so we have to go back before the commit 3b5704b. Run QEMU with spice:$ qemu-system-x86_64 -nodefaults -S -spice port=5900,disable-ticketing -qmp stdio {"QMP": {"version": {"qemu": {"micro": 50, "minor": 2, "major": 2}, "package": ""}, "capabilities": []}}Connect a spice client in another terminal:
$ remote-viewer spice://localhost:5900Quoted 7 lines
Set up a migration destination dummy in a third terminal:
$ socat TCP-LISTEN:12345 STDIONow paste the following into the QMP monitor:
{ "execute": "qmp_capabilities" }If you stick and "id":"i0" here...
Quoted 4 lines
{ "execute": "client_migrate_info", "id": "i1", "arguments": { "protocol": "spice", "hostname": "localhost", "port": 12345 } } { "execute": "query-kvm", "id": "i2" }
Quoted 7 lines
Quoted 4 lines
Produces two replies immediately, one to
qmp_capabilities, and one to query-kvm:{"return": {}}...it should show up here, for even less ambiguity about the problems with the async response.
Can do.
Quoted 22 lines
Quoted 14 lines
{"return": {"enabled": false, "present": true}, "id": "i2"}Both are correct. Two lines of debug output from libspice-server not shown.
Now EOF socat's standard input to make it close the connection. This makes the asynchronous
client_migrate_infocomplete. It replies:{"return": {}}Bug: "id": "i1" is missing. Two lines of debug output from libspice-server not shown. Cherry on top: storage for the missing ID is leaked.
Quoted 7 lines
Get rid of this stuff before somebody hurts himself with it.
Signed-off-by Markus Armbruster <[email protected]>--- include/monitor/monitor.h | 5 ---- monitor.c | 68 ++--------------------------------------------- 2 files changed, 2 insertions(+), 71 deletions(-)
Quoted 3 lines
Certainly takes an axe to it!
Reviewed-by Eric Blake <[email protected]>
Thanks!
PATCH 2/20 monitor: Clean up after previous commit
In reply to Markus Armbruster--- monitor.c | 40 +++++++++++----------------------------- 1 file changed, 11 insertions(+), 29 deletions(-)
monitor.c +11 -29
@@ -4045,18 +4045,6 @@ void monitor_set_error(Monitor *mon, QError *qerror) } } -static void handler_audit (Monitor *mon,const mon_cmd_t *cmd,int ret) -{ -if (ret && !monitor_has_error (mon)) { -/* -* If it returns failure, it must have passed on error. -* -* Action: Report an internal error to the client if in QMP. -*/ -qerror_report (QERR_UNDEFINED_ERROR ); - } -} -static void handle_user_command (Monitor *mon,const char *cmdline) {QDict *qdict;
@@ -5015,28 +5003,17 @@ static QDict *qmp_check_input_obj(QObject *input_obj)return input_dict; } -static void qmp_call_cmd (Monitor *mon,const mon_cmd_t *cmd, -const QDict *params) -{ -int ret; -QObject *data =NULL ; - - ret = cmd->mhandler.cmd_new (mon, params, &data); -handler_audit (mon, cmd, ret); -monitor_protocol_emitter (mon, data); -qobject_decref (data); -} -static void handle_qmp_command (JSONMessageParser *parser,QList *tokens) {int err; -QObject *obj; +QObject *obj, *data;QDict *input, *args;const mon_cmd_t *cmd;const char *cmd_name;Monitor *mon = cur_mon; args = input =NULL ; + data =NULL ; obj =json_parser_parse (tokens,NULL );if (!obj) {
@@ -5079,12 +5056,17 @@ static void handle_qmp_command(JSONMessageParser *parser, QList *tokens) goto err_out; } -qmp_call_cmd (mon, cmd, args); - goto out; +if (cmd->mhandler.cmd_new (mon, args, &data)) { +/* Command failed... */ +if (!monitor_has_error (mon)) { +/* ... without setting an error, so make one up */ +qerror_report (QERR_UNDEFINED_ERROR ); + } + } err_out: -monitor_protocol_emitter (mon,NULL ); -out: +monitor_protocol_emitter (mon, data); +qobject_decref (data);QDECREF (input);QDECREF (args); }
Signature · 1 line
1.9.3
Re: [Qemu-devel] [PATCH 02/20] monitor: Clean up after previous commit
Quoted 2 lines · Markus Armbruster
Signed-off-by Markus Armbruster <[email protected]>---
Commit message is sparse; I would have mentioned [1] and [2].
Quoted 5 lines
monitor.c | 40 +++++++++++----------------------------- 1 file changed, 11 insertions(+), 29 deletions(-)monitor.c +0
-1-static void handler_audit (Monitor *mon,const mon_cmd_t *cmd,int ret)
monitor.c void monitor_set_error(Monitor *mon, QError *qerror) · line 4058
Quoted 10 lines
monitor.c +0
-10-{ -if (ret && !monitor_has_error (mon)) { -/* -* If it returns failure, it must have passed on error. -* -* Action: Report an internal error to the client if in QMP. -*/ -qerror_report (QERR_UNDEFINED_ERROR ); - } -}
[2] - handler_audit() goes away now that it has no caller
Quoted 9 lines
monitor.c +0
-2-static void handle_user_command (Monitor *mon,const char *cmdline) {QDict *qdict;@@ -5015,28 +5003,17 @@ static QDict *qmp_check_input_obj(QObject *input_obj)return input_dict; } -static void qmp_call_cmd(Monitor *mon, const mon_cmd_t *cmd,
monitor.c static QDict *qmp_check_input_obj(QObject *input_obj) · line 5028
Quoted 10 lines
monitor.c +0
-10- const QDict *params) -{ -int ret; -QObject *data =NULL ; - - ret = cmd->mhandler.cmd_new (mon, params, &data); -handler_audit (mon, cmd, ret); -monitor_protocol_emitter (mon, data); -qobject_decref (data); -}
[1] - qmp_call_cmd() is inlined and simplified...
Quoted 19 lines
monitor.c +2
-2-static void handle_qmp_command (JSONMessageParser *parser,QList *tokens) {int err; -QObject *obj; +QObject *obj, *data;QDict *input, *args;const mon_cmd_t *cmd;const char *cmd_name;Monitor *mon = cur_mon; args = input =NULL ; + data =NULL ; obj =json_parser_parse (tokens,NULL );if (!obj) {@@ -5079,12 +5056,17 @@ static void handle_qmp_command(JSONMessageParser *parser, QList *tokens) goto err_out; }
monitor.c static void handle_qmp_command(JSONMessageParser *parser, QList *tokens) · line 5065
Quoted 10 lines
monitor.c +7
-2-qmp_call_cmd (mon, cmd, args); - goto out; +if (cmd->mhandler.cmd_new (mon, args, &data)) { +/* Command failed... */ +if (!monitor_has_error (mon)) { +/* ... without setting an error, so make one up */ +qerror_report (QERR_UNDEFINED_ERROR ); + } + }
...into its lone caller.
monitor.c static void handle_qmp_command(JSONMessageParser *parser, QList *tokens)
Quoted 10 lines
monitor.c +2
-2err_out: -monitor_protocol_emitter (mon,NULL ); -out: +monitor_protocol_emitter (mon, data); +qobject_decref (data);QDECREF (input);QDECREF (args); }
But the code cleanup itself looks sane, so with the improved commit message:
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Re: [Qemu-devel] [PATCH 02/20] monitor: Clean up after previous commit
Quoted 66 lines · Eric Blake
Quoted 2 lines · Markus Armbruster
Signed-off-by Markus Armbruster <[email protected]>---
Commit message is sparse; I would have mentioned [1] and [2].
Quoted 5 lines
monitor.c | 40 +++++++++++----------------------------- 1 file changed, 11 insertions(+), 29 deletions(-)monitor.c +0
-1-static void handler_audit (Monitor *mon,const mon_cmd_t *cmd,int ret)monitor.c void monitor_set_error(Monitor *mon, QError *qerror) · line 4058
Quoted 10 lines
monitor.c +0
-10-{ -if (ret && !monitor_has_error (mon)) { -/* -* If it returns failure, it must have passed on error. -* -* Action: Report an internal error to the client if in QMP. -*/ -qerror_report (QERR_UNDEFINED_ERROR ); - } -}[2] -
handler_audit()goes away now that it has no callerQuoted 9 lines
monitor.c +0
-2-static void handle_user_command (Monitor *mon,const char *cmdline) {QDict *qdict;@@ -5015,28 +5003,17 @@ static QDict *qmp_check_input_obj(QObject *input_obj)return input_dict; } -static void qmp_call_cmd(Monitor *mon, const mon_cmd_t *cmd,monitor.c static QDict *qmp_check_input_obj(QObject *input_obj) · line 5028
Quoted 10 lines
monitor.c +0
-10- const QDict *params) -{ -int ret; -QObject *data =NULL ; - - ret = cmd->mhandler.cmd_new (mon, params, &data); -handler_audit (mon, cmd, ret); -monitor_protocol_emitter (mon, data); -qobject_decref (data); -}[1] -
qmp_call_cmd()is inlined and simplified...Quoted 20 lines
monitor.c +2
-2-static void handle_qmp_command (JSONMessageParser *parser,QList *tokens) {int err; -QObject *obj; +QObject *obj, *data;QDict *input, *args;const mon_cmd_t *cmd;const char *cmd_name;Monitor *mon = cur_mon; args = input =NULL ; + data =NULL ; obj =json_parser_parse (tokens,NULL );if (!obj) {@@ -5079,12 +5056,17 @@ static void handle_qmp_command(JSONMessageParser *parser, QList *tokens) goto err_out; }
Quoted 11 lines
monitor.c static void handle_qmp_command(JSONMessageParser *parser, QList *tokens) · line 5065
Quoted 9 lines
monitor.c +7
-2-qmp_call_cmd (mon, cmd, args); - goto out; +if (cmd->mhandler.cmd_new (mon, args, &data)) { +/* Command failed... */ +if (!monitor_has_error (mon)) { +/* ... without setting an error, so make one up */ +qerror_report (QERR_UNDEFINED_ERROR ); + } + }...into its lone caller.
Can do.
Quoted 11 lines
monitor.c static void handle_qmp_command(JSONMessageParser *parser, QList *tokens)
Quoted 10 lines
monitor.c +2
-2err_out: -monitor_protocol_emitter (mon,NULL ); -out: +monitor_protocol_emitter (mon, data); +qobject_decref (data);QDECREF (input);QDECREF (args); }
Quoted 3 lines
But the code cleanup itself looks sane, so with the improved commit message:
Reviewed-by Eric Blake <[email protected]>
Thanks!
PATCH 3/20 monitor: Improve and document client_migrate_info protocol error
In reply to Markus ArmbrusterProtocol must be spice, vnc isn't implemented. Fix up documentation.
Attempts to use vnc or any other unknown protocol yield the misleading error message "Invalid parameter 'protocol'". Improve it to "Parameter 'protocol' expects spice".
--- hmp-commands.hx | 1 + monitor.c | 2 +- qmp-commands.hx | 3 ++- 3 files changed, 4 insertions(+), 2 deletions(-)
hmp-commands.hx +1 -0
@@ -1022,6 +1022,7 @@ STEXI Set the spice/vnc connection info for the migration target. The spice/vnc server will ask the spice/vnc client to automatically reconnect using the new parameters (if specified) once the vm migration finished successfully. +Not yet implemented for VNC. ETEXI {
monitor.c +1 -1
@@ -1063,7 +1063,7 @@ static int client_migrate_info(Monitor *mon, const QDict *qdict,return 0 ; } -qerror_report (QERR_INVALID_PARAMETER ,"protocol" ); +qerror_report (QERR_INVALID_PARAMETER_VALUE ,"protocol" ,"spice" );return -1 ; }
qmp-commands.hx +2 -1
@@ -795,10 +795,11 @@ client_migrate_info Set the spice/vnc connection info for the migration target. The spice/vnc server will ask the spice/vnc client to automatically reconnect using the new parameters (if specified) once the vm migration finished successfully. +Not yet implemented for VNC. Arguments: -- "protocol": protocol: "spice" or "vnc" (json-string) +- "protocol": must be "spice" (json-string) - "hostname": migration target hostname (json-string) - "port": spice/vnc tcp port for plaintext channels (json-int, optional) - "tls-port": spice tcp port for tls-secured channels (json-int, optional)
Signature · 1 line
1.9.3
Re: [Qemu-devel] [PATCH 03/20] monitor: Improve and document client_migrate_info protocol error
Quoted 6 lines · Markus Armbruster
Protocol must be spice, vnc isn't implemented. Fix up documentation.
Attempts to use vnc or any other unknown protocol yield the misleading error message "Invalid parameter 'protocol'". Improve it to "Parameter 'protocol' expects spice".
Quoted 7 lines
Signed-off-by Markus Armbruster <[email protected]>--- hmp-commands.hx | 1 + monitor.c | 2 +- qmp-commands.hx | 3 ++- 3 files changed, 4 insertions(+), 2 deletions(-)
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
PATCH 4/20 monitor: Convert client_migrate_info to QAPI
In reply to Markus Armbruster--- hmp-commands.hx | 3 +-- hmp.c | 17 +++++++++++++++++ hmp.h | 1 + monitor.c | 42 ++++++++++++++++++------------------------ qapi-schema.json | 20 ++++++++++++++++++++ qmp-commands.hx | 2 +- 6 files changed, 58 insertions(+), 27 deletions(-)
hmp-commands.hx +1 -2
@@ -1012,8 +1012,7 @@ ETEXI .args_type = "protocol:s,hostname:s,port:i?,tls-port:i?,cert-subject:s?", .params = "protocol hostname port tls-port cert-subject", .help = "send migration info to spice/vnc client", - .user_print = monitor_user_noop, - .mhandler.cmd_new = client_migrate_info, + .mhandler.cmd = hmp_client_migrate_info, }, STEXI
hmp.c +17 -0
@@ -1250,6 +1250,23 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict) } } +void hmp_client_migrate_info (Monitor *mon,const QDict *qdict) +{ +Error *err =NULL ; +const char *protocol =qdict_get_str (qdict,"protocol" ); +const char *hostname =qdict_get_str (qdict,"hostname" ); +bool has_port =qdict_haskey (qdict,"port" ); +int port =qdict_get_try_int (qdict,"port" ,-1 ); +bool has_tls_port =qdict_haskey (qdict,"tls-port" ); +int tls_port =qdict_get_try_int (qdict,"tls-port" ,-1 ); +const char *cert_subject =qdict_get_try_str (qdict,"cert-subject" ); + +qmp_client_migrate_info (protocol, hostname, + has_port, port, has_tls_port, tls_port, + !!cert_subject, cert_subject, &err); +hmp_handle_error (mon, &err); +} +void hmp_set_password (Monitor *mon,const QDict *qdict) {const char *protocol =qdict_get_str (qdict,"protocol" );
hmp.h +1 -0
@@ -67,6 +67,7 @@ void hmp_migrate_set_speed(Monitor *mon, const QDict *qdict);void hmp_migrate_set_capability (Monitor *mon,const QDict *qdict);void hmp_migrate_set_parameter (Monitor *mon,const QDict *qdict);void hmp_migrate_set_cache_size (Monitor *mon,const QDict *qdict); +void hmp_client_migrate_info (Monitor *mon,const QDict *qdict);void hmp_set_password (Monitor *mon,const QDict *qdict);void hmp_expire_password (Monitor *mon,const QDict *qdict);void hmp_eject (Monitor *mon,const QDict *qdict);
monitor.c +18 -24
57 lines not on this page · raw message
qapi-schema.json +20 -0
@@ -638,6 +638,26 @@ 'returns': 'MigrationParameters' } ## +# @client_migrate_info +# +# Set the spice/vnc connection info for the migration target. The +# spice/vnc server will ask the spice/vnc client to automatically +# reconnect using the new parameters (if specified) once the vm +# migration finished successfully. Not yet implemented for VNC. +# +# @protocol: must be "spice" +# @hostname: migration target hostname +# @port: #optional spice/vnc tcp port for plaintext channels +# @tls-port: #optional spice tcp port for tls-secured channels +# @cert-subject: #optional server certificate subject +# +# Since: 0.14.0 +## +{ 'command': 'client_migrate_info', + 'data': { 'protocol': 'str', 'hostname': 'str', '*port': 'int', + '*tls-port': 'int', '*cert-subject': 'str' } } + +## # @MouseInfo: # # Information about a mouse device.
qmp-commands.hx +1 -1
@@ -785,7 +785,7 @@ EQMP .args_type = "protocol:s,hostname:s,port:i?,tls-port:i?,cert-subject:s?", .params = "protocol hostname port tls-port cert-subject", .help = "send migration info to spice/vnc client", - .mhandler.cmd_new = client_migrate_info, + .mhandler.cmd_new = qmp_marshal_input_client_migrate_info, }, SQMP
Signature · 1 line
1.9.3
Re: [Qemu-devel] [PATCH 04/20] monitor: Convert client_migrate_info to QAPI
Quoted 13 lines · Markus Armbruster
Signed-off-by Markus Armbruster <[email protected]>--- hmp-commands.hx | 3 +-- hmp.c | 17 +++++++++++++++++ hmp.h | 1 + monitor.c | 42 ++++++++++++++++++------------------------ qapi-schema.json | 20 ++++++++++++++++++++ qmp-commands.hx | 2 +- 6 files changed, 58 insertions(+), 27 deletions(-)+++
b/qapi-schema.json
qapi-schema.json line 641
Quoted 4 lines
qapi-schema.json +1
-0@@ -638,6 +638,26 @@ 'returns': 'MigrationParameters' } ## +# @client_migrate_info
Should we name this new command 'client-migrate-info'?
/me goes and checks...
Oh, wow, the QMP command IS advertised by 'query-commands' in spite of not having this wrapper, even in qemu 2.3.
qapi-schema.json line 650
Quoted 9 lines
qapi-schema.json +9
-0+# +# Set the spice/vnc connection info for the migration target. The +# spice/vnc server will ask the spice/vnc client to automatically +# reconnect using the new parameters (if specified) once the vm +# migration finished successfully. Not yet implemented for VNC. +# +# @protocol: must be "spice" +# @hostname: migration target hostname +# @port: #optional spice/vnc tcp port for plaintext channels
Is it worth documenting vnc, when we just stated earlier that protocol must be spice?
qapi-schema.json line 654
Quoted 4 lines
qapi-schema.json +4
-0+# @tls-port: #optional spice tcp port for tls-secured channels +# @cert-subject: #optional server certificate subject +# +# Since: 0.14.0
So this 'Since:' designation is correct, and we are just _finally_ documenting something that has silently been sitting around in QMP for a looooong time.
qapi-schema.json line 658
Quoted 4 lines
qapi-schema.json +4
-0+## +{ 'command': 'client_migrate_info', + 'data': { 'protocol': 'str', 'hostname': 'str', '*port': 'int', + '*tls-port': 'int', '*cert-subject': 'str' } }
Idea for followups - since 'protocol' must be "spice", should we:
1) make it an enum type rather than open-coded str
2) make it an optional parameter, so that omitting it defaults to spice?
Quoted 10 lines
qapi-schema.json +2
-0+ +## # @MouseInfo: # # Information about a mouse device.qmp-commands.hx +0
-0
qmp-commands.hx EQMP · line 788
Quoted 5 lines
qmp-commands.hx +1
-1@@ -785,7 +785,7 @@ EQMP .args_type = "protocol:s,hostname:s,port:i?,tls-port:i?,cert-subject:s?", .params = "protocol hostname port tls-port cert-subject", .help = "send migration info to spice/vnc client", - .mhandler.cmd_new = client_migrate_info, + .mhandler.cmd_new = qmp_marshal_input_client_migrate_info,
How many entries in qmp-commands.hx are NOT present in qapi? It looks like this patch gets us one step closer to ditching the need to maintain this file, which is a good thing.
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Re: [Qemu-devel] [PATCH 04/20] monitor: Convert client_migrate_info to QAPI
Quoted 21 lines · Eric Blake
Quoted 13 lines · Markus Armbruster
Signed-off-by Markus Armbruster <[email protected]>--- hmp-commands.hx | 3 +-- hmp.c | 17 +++++++++++++++++ hmp.h | 1 + monitor.c | 42 ++++++++++++++++++------------------------ qapi-schema.json | 20 ++++++++++++++++++++ qmp-commands.hx | 2 +- 6 files changed, 58 insertions(+), 27 deletions(-)+++
b/qapi-schema.jsonqapi-schema.json line 641
Quoted 4 lines
qapi-schema.json +1
-0@@ -638,6 +638,26 @@ 'returns': 'MigrationParameters' } ## +# @client_migrate_infoShould we name this new command 'client-migrate-info'?
Quoted 4 lines
/me goes and checks...
Oh, wow, the QMP command IS advertised by 'query-commands' in spite of not having this wrapper, even in qemu 2.3.
Yup. query-commands uses qmp_cmds[], not QAPI.
Quoted 4 lines
Quoted 4 lines
qapi-schema.json +4
-0+# +# Set the spice/vnc connection info for the migration target. The +# spice/vnc server will ask the spice/vnc client to automatically +# reconnect using the new parameters (if specified) once the vm
Quoted 8 lines
qapi-schema.json line 650
Quoted 5 lines
qapi-schema.json +5
-0+# migration finished successfully. Not yet implemented for VNC. +# +# @protocol: must be "spice" +# @hostname: migration target hostname +# @port: #optional spice/vnc tcp port for plaintext channelsIs it worth documenting vnc, when we just stated earlier that protocol must be spice?
I think this is a question for Gerd (cc'ed).
Quoted 8 lines
qapi-schema.json line 654
Quoted 4 lines
qapi-schema.json +4
-0+# @tls-port: #optional spice tcp port for tls-secured channels +# @cert-subject: #optional server certificate subject +# +# Since: 0.14.0So this 'Since:' designation is correct, and we are just _finally_ documenting something that has silently been sitting around in QMP for a looooong time.
It's always been documented in qmp-commands.hx.
qmp-commands.hx predates QAPI. We still use it to generate qmp-commands.txt and qmp-commands-old.h. The latter provides the initializer for qmp_cmds[], thus determines the reply to query-commands.
qmp-commands.hx is complete. The QAPI schema contains only qapified commands, and therefore cannot serve as single authoritative source.
An awful lot of information is duplicated between qmp-commands.hx and the QAPI schema. To clean up this mess, we need to finish qapifying commands, and then fold qmp-commands.hx's functionality into the schema.
Quoted 7 lines
qapi-schema.json line 658
Quoted 4 lines
qapi-schema.json +4
-0+## +{ 'command': 'client_migrate_info', + 'data': { 'protocol': 'str', 'hostname': 'str', '*port': 'int', + '*tls-port': 'int', '*cert-subject': 'str' } }Idea for followups - since 'protocol' must be "spice", should we:
1) make it an enum type rather than open-coded str
Useful cleanup.
Quoted 1 line
2) make it an optional parameter, so that omitting it defaults to spice?
Not sure. Gerd?
Quoted 9 lines
Quoted 9 lines
qapi-schema.json +2
-0+ +## # @MouseInfo: # # Information about a mouse device.qmp-commands.hx +0
-0
Quoted 8 lines
qmp-commands.hx EQMP · line 788
Quoted 6 lines
qmp-commands.hx +1
-1@@ -785,7 +785,7 @@ EQMP .args_type = "protocol:s,hostname:s,port:i?,tls-port:i?,cert-subject:s?", .params = "protocol hostname port tls-port cert-subject", .help = "send migration info to spice/vnc client", - .mhandler.cmd_new = client_migrate_info, + .mhandler.cmd_new = qmp_marshal_input_client_migrate_info,How many entries in qmp-commands.hx are NOT present in qapi?
Not many, but I don't remember the exact list offhand. The most difficult one is probably device_add. I got patches getting that one a bit closer, but they depend by a straightforward PPC patch I've been trying to get in since February.
Quoted 3 lines
It looks
like this patch gets us one step closer to ditching the need to maintain this file, which is a good thing.
Yes.
Quoted 1 line
Reviewed-by Eric Blake <[email protected]>
Thanks!
Re: [Qemu-devel] [PATCH 04/20] monitor: Convert client_migrate_info to QAPI
Hi,
Quoted 13 lines
Quoted 4 lines
Quoted 4 lines
qapi-schema.json +4
-0+# +# Set the spice/vnc connection info for the migration target. The +# spice/vnc server will ask the spice/vnc client to automatically +# reconnect using the new parameters (if specified) once the vmQuoted 8 lines
qapi-schema.json line 650
Quoted 5 lines
qapi-schema.json +5
-0+# migration finished successfully. Not yet implemented for VNC. +# +# @protocol: must be "spice" +# @hostname: migration target hostname +# @port: #optional spice/vnc tcp port for plaintext channelsIs it worth documenting vnc, when we just stated earlier that protocol must be spice?
Quoted 1 line
I think this is a question for Gerd (cc'ed).
IIRC Daniel (added to Cc:) had plans to create a vnc extension for that.
Which was the reason to explicitly add the protocol here, so we can use the same command for both spice and vnc some day.
Quoted 9 lines
Quoted 7 lines
qapi-schema.json line 658
Quoted 4 lines
qapi-schema.json +4
-0+## +{ 'command': 'client_migrate_info', + 'data': { 'protocol': 'str', 'hostname': 'str', '*port': 'int', + '*tls-port': 'int', '*cert-subject': 'str' } }Idea for followups - since 'protocol' must be "spice", should we:
1) make it an enum type rather than open-coded strUseful cleanup.
Agree.
Quoted 3 lines
Quoted 1 line
2) make it an optional parameter, so that omitting it defaults to spice?
Not sure. Gerd?
Not sure this buys us much. I guess libvirt will continue to pass 'spice' because everything else doesn't make much sense.
cheers,
Gerd
Re: [Qemu-devel] [PATCH 04/20] monitor: Convert client_migrate_info to QAPI
Quoted 15 lines · Gerd Hoffmann
Hi,
Quoted 13 lines
Quoted 12 lines
qapi-schema.json line 650
Quoted 9 lines
qapi-schema.json +9
-0+# +# Set the spice/vnc connection info for the migration target. The +# spice/vnc server will ask the spice/vnc client to automatically +# reconnect using the new parameters (if specified) once the vm +# migration finished successfully. Not yet implemented for VNC. +# +# @protocol: must be "spice" +# @hostname: migration target hostname +# @port: #optional spice/vnc tcp port for plaintext channelsIs it worth documenting vnc, when we just stated earlier that protocol must be spice?
Quoted 6 lines
Quoted 1 line
I think this is a question for Gerd (cc'ed).
IIRC Daniel (added to Cc:) had plans to create a vnc extension for that.
Which was the reason to explicitly add the protocol here, so we can use the same command for both spice and vnc some day.
Yeah, it would be nice to do a VNC extension, though realistically I'm not going to have time for that anytime in the forseeable future.
Regards,
Daniel
Signature · 4 lines
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
Re: [Qemu-devel] [PATCH 04/20] monitor: Convert client_migrate_info to QAPI
Quoted 18 lines · Daniel P. Berrange
Quoted 17 lines · Gerd Hoffmann
Hi,
Quoted 14 lines
Quoted 4 lines
Quoted 4 lines
qapi-schema.json +4
-0+# +# Set the spice/vnc connection info for the migration target. The +# spice/vnc server will ask the spice/vnc client to automatically +# reconnect using the new parameters (if specified) once the vmQuoted 8 lines
qapi-schema.json line 650
Quoted 5 lines
qapi-schema.json +5
-0+# migration finished successfully. Not yet implemented for VNC. +# +# @protocol: must be "spice" +# @hostname: migration target hostname +# @port: #optional spice/vnc tcp port for plaintext channelsIs it worth documenting vnc, when we just stated earlier that protocol must be spice?
I think this is a question for Gerd (cc'ed).
Quoted 7 lines
Quoted 4 lines
IIRC Daniel (added to Cc:) had plans to create a vnc extension for that.
Which was the reason to explicitly add the protocol here, so we can use the same command for both spice and vnc some day.
Yeah, it would be nice to do a VNC extension, though realistically I'm not going to have time for that anytime in the forseeable future.
Should we continue to document the command is about "spice/vnc connection info", or should we rephrase? Something like:
client_migrate_info
-------------------
Set remote display connection information for migration. This makes the server ask the client to automatically reconnect using the new parameters once migration finished successfully. Only implemented for SPICE.
Arguments:
- "protocol": must be "spice" (json-string) - "hostname": migration target hostname (json-string) - "port": spice/vnc tcp port for plaintext channels (json-int, optional) - "tls-port": spice tcp port for tls-secured channels (json-int, optional)
- "cert-subject": server certificate subject (json-string, optional)
Re: [Qemu-devel] [PATCH 04/20] monitor: Convert client_migrate_info to QAPI
Hi,
Quoted 11 lines
Should we continue to document the command is about "spice/vnc connection info", or should we rephrase? Something like:
client_migrate_info
-------------------Set remote display connection information for migration. This makes the server ask the client to automatically reconnect using the new parameters once migration finished successfully. Only implemented for SPICE.
Quoted 7 lines
Arguments:
- "protocol": must be "spice" (json-string) - "hostname": migration target hostname (json-string) - "port": spice/vnc tcp port for plaintext channels (json-int, optional) - "tls-port": spice tcp port for tls-secured channels (json-int, optional)- "cert-subject": server certificate subject (json-string, optional)
Looks good to me.
cheers,
Gerd
Re: [Qemu-devel] [PATCH 04/20] monitor: Convert client_migrate_info to QAPI
Quoted 15 lines · Gerd Hoffmann
Hi,
Quoted 13 lines
Should we continue to document the command is about "spice/vnc connection info", or should we rephrase? Something like:
client_migrate_info
-------------------Set remote display connection information for migration. This makes the server ask the client to automatically reconnect using the new parameters once migration finished successfully. Only implemented for SPICE.
Arguments:
Quoted 8 lines
Quoted 6 lines
- "protocol": must be "spice" (json-string)
- "hostname": migration target hostname (json-string)
- "port": spice/vnc tcp port for plaintext channels (json-int, optional)
- "tls-port": spice tcp port for tls-secured channels (json-int, optional)
- "cert-subject": server certificate subject (json-string, optional)Looks good to me.
Okay, I'll squash something like that into PATCH 03, and cc: you for review.
PATCH 5/20 monitor: Use traditional command interface for HMP drive_del
In reply to Markus ArmbrusterAll QMP commands use the "new" handler interface (mhandler.cmd_new). Most HMP commands still use the traditional interface (mhandler.cmd), but a few use the "new" one. Complicates handle_user_command() for no gain, so I'm converting these to the traditional interface.
For drive_del, that's easy: hmp_drive_del() sheds its unused last parameter, and its return value, which the caller ignored anyway.
--- blockdev.c | 9 ++++----- hmp-commands.hx | 3 +-- include/sysemu/blockdev.h | 2 +- 3 files changed, 6 insertions(+), 8 deletions(-)
blockdev.c +4 -5
@@ -2113,7 +2113,7 @@ void qmp_block_dirty_bitmap_clear(const char *node, const char *name,aio_context_release (aio_context); } -int hmp_drive_del (Monitor *mon,const QDict *qdict,QObject **ret_data) +void hmp_drive_del (Monitor *mon,const QDict *qdict) {const char *id =qdict_get_str (qdict,"id" );BlockBackend *blk;
@@ -2124,14 +2124,14 @@ int hmp_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data) blk =blk_by_name (id);if (!blk) {error_report ("Device '%s' not found" , id); -return -1 ; +return ; } bs =blk_bs (blk);if (!blk_legacy_dinfo (blk)) {error_report ("Deleting device added with blockdev-add" " is not supported" ); -return -1 ; +return ; } aio_context =bdrv_get_aio_context (bs);
@@ -2140,7 +2140,7 @@ int hmp_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data)if (bdrv_op_is_blocked (bs,BLOCK_OP_TYPE_DRIVE_DEL , &local_err)) {error_report_err (local_err);aio_context_release (aio_context); -return -1 ; +return ; }/* quiesce block driver; prevent further io */
@@ -2163,7 +2163,6 @@ int hmp_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data) }aio_context_release (aio_context); -return 0 ; } void qmp_block_resize(bool has_device, const char *device,
hmp-commands.hx +1 -2
@@ -178,8 +178,7 @@ ETEXI .args_type = "id:B", .params = "device", .help = "remove host block device", - .user_print = monitor_user_noop, - .mhandler.cmd_new = hmp_drive_del, + .mhandler.cmd = hmp_drive_del, }, STEXI
include/sysemu/blockdev.h +1 -1
@@ -66,5 +66,5 @@ DriveInfo *drive_new(QemuOpts *arg, BlockInterfaceType block_default_type);void qmp_change_blockdev (const char *device,const char *filename,const char *format,Error **errp);void hmp_commit (Monitor *mon,const QDict *qdict); -int hmp_drive_del (Monitor *mon,const QDict *qdict,QObject **ret_data); +void hmp_drive_del (Monitor *mon,const QDict *qdict);#endif
Signature · 1 line
1.9.3
Re: [Qemu-devel] [PATCH 05/20] monitor: Use traditional command interface for HMP drive_del
Quoted 8 lines · Markus Armbruster
All QMP commands use the "new" handler interface (mhandler.cmd_new). Most HMP commands still use the traditional interface (mhandler.cmd), but a few use the "new" one. Complicates
handle_user_command()for no gain, so I'm converting these to the traditional interface.For
drive_del, that's easy:hmp_drive_del()sheds its unused last parameter, and its return value, which the caller ignored anyway.
Quoted 7 lines
Signed-off-by Markus Armbruster <[email protected]>--- blockdev.c | 9 ++++----- hmp-commands.hx | 3 +-- include/sysemu/blockdev.h | 2 +- 3 files changed, 6 insertions(+), 8 deletions(-)
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
PATCH 6/20 monitor: Use traditional command interface for HMP device_add
In reply to Markus ArmbrusterAll QMP commands use the "new" handler interface (mhandler.cmd_new). Most HMP commands still use the traditional interface (mhandler.cmd), but a few use the "new" one. Complicates handle_user_command() for no gain, so I'm converting these to the traditional interface.
For device_add, that's easy: just wrap the obvious hmp_device_add() around do_device_add().
monitor_user_noop() is now unused, drop it.
--- hmp-commands.hx | 3 +-- hmp.c | 6 ++++++ hmp.h | 1 + monitor.c | 2 -- 4 files changed, 8 insertions(+), 4 deletions(-)
hmp-commands.hx +1 -2
@@ -653,8 +653,7 @@ ETEXI .args_type = "device:O", .params = "driver[,prop=value][,...]", .help = "add device, like -device on the command line", - .user_print = monitor_user_noop, - .mhandler.cmd_new = do_device_add, + .mhandler.cmd = hmp_device_add, .command_completion = device_add_completion, },
hmp.c +6 -0
@@ -22,6 +22,7 @@#include "qmp-commands.h" #include "qemu/sockets.h" #include "monitor/monitor.h" +#include "monitor/qdev.h" #include "qapi/opts-visitor.h" #include "qapi/string-output-visitor.h" #include "qapi-visit.h"
@@ -1499,6 +1500,11 @@ void hmp_migrate(Monitor *mon, const QDict *qdict) } } +void hmp_device_add (Monitor *mon,const QDict *qdict) +{ +do_device_add (mon, qdict,NULL ); +} +void hmp_device_del (Monitor *mon,const QDict *qdict) {const char *id =qdict_get_str (qdict,"id" );
hmp.h +1 -0
@@ -80,6 +80,7 @@ void hmp_block_job_pause(Monitor *mon, const QDict *qdict);void hmp_block_job_resume (Monitor *mon,const QDict *qdict);void hmp_block_job_complete (Monitor *mon,const QDict *qdict);void hmp_migrate (Monitor *mon,const QDict *qdict); +void hmp_device_add (Monitor *mon,const QDict *qdict);void hmp_device_del (Monitor *mon,const QDict *qdict);void hmp_dump_guest_memory (Monitor *mon,const QDict *qdict);void hmp_netdev_add (Monitor *mon,const QDict *qdict);
monitor.c +0 -2
@@ -378,8 +378,6 @@ static int GCC_FMT_ATTR(2, 3) monitor_fprintf(FILE *stream,return 0 ; } -static void monitor_user_noop (Monitor *mon,const QObject *data) { } -static inline int handler_is_qobject (const mon_cmd_t *cmd) {return cmd->user_print !=NULL ;
Signature · 1 line
1.9.3
Re: [Qemu-devel] [PATCH 06/20] monitor: Use traditional command interface for HMP device_add
Quoted 10 lines · Markus Armbruster
All QMP commands use the "new" handler interface (mhandler.cmd_new). Most HMP commands still use the traditional interface (mhandler.cmd), but a few use the "new" one. Complicates
handle_user_command()for no gain, so I'm converting these to the traditional interface.For
device_add, that's easy: just wrap the obvioushmp_device_add()arounddo_device_add().
monitor_user_noop()is now unused, drop it.
Quoted 8 lines
Signed-off-by Markus Armbruster <[email protected]>--- hmp-commands.hx | 3 +-- hmp.c | 6 ++++++ hmp.h | 1 + monitor.c | 2 -- 4 files changed, 8 insertions(+), 4 deletions(-)
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
PATCH 7/20 monitor: Use trad. command interface for HMP pcie_aer_inject_error
In reply to Markus ArmbrusterAll QMP commands use the "new" handler interface (mhandler.cmd_new). Most HMP commands still use the traditional interface (mhandler.cmd), but a few use the "new" one. Complicates handle_user_command() for no gain, so I'm converting these to the traditional interface.
pcie_aer_inject_error's implementation is split into the hmp_pcie_aer_inject_error() and pcie_aer_inject_error_print(). The former is a peculiar crossbreed between HMP and QMP handler. On success, it works like a QMP handler: store QDict through ret_data parameter, return 0. Printing the QDict is left to pcie_aer_inject_error_print(). On failure, it works more like an HMP
handler: print error to monitor, return negative number.
To convert to the traditional interface, turn pcie_aer_inject_error_print() into a command handler wrapping around hmp_pcie_aer_inject_error(). By convention, this command handler should be called hmp_pcie_aer_inject_error(), so rename the existing hmp_pcie_aer_inject_error() to do_pcie_aer_inject_error().
--- hmp-commands.hx | 3 +-- hw/pci/pci-stub.c | 14 +------------- hw/pci/pcie_aer.c | 39 ++++++++++++++++++++++----------------- include/sysemu/sysemu.h | 4 +--- 4 files changed, 25 insertions(+), 35 deletions(-)
hmp-commands.hx +1 -2
@@ -1184,8 +1184,7 @@ ETEXI "<error_status> = error string or 32bit\n\t\t\t" "<tlb header> = 32bit x 4\n\t\t\t" "<tlb header prefix> = 32bit x 4", - .user_print = pcie_aer_inject_error_print, - .mhandler.cmd_new = hmp_pcie_aer_inject_error, + .mhandler.cmd = hmp_pcie_aer_inject_error, }, STEXI
hw/pci/pci-stub.c +1 -13
@@ -29,19 +29,7 @@ PciInfoList *qmp_query_pci(Error **errp)return NULL ; } -static void pci_error_message (Monitor *mon) +void hmp_pcie_aer_inject_error (Monitor *mon,const QDict *qdict) {monitor_printf (mon,"PCI devices not supported\n" ); } - -int hmp_pcie_aer_inject_error (Monitor *mon, -const QDict *qdict,QObject **ret_data) -{ -pci_error_message (mon); -return -ENOSYS ; -} - -void pcie_aer_inject_error_print (Monitor *mon,const QObject *data) -{ -pci_error_message (mon); -}
hw/pci/pcie_aer.c +22 -17
@@ -815,21 +815,6 @@ const VMStateDescription vmstate_pcie_aer_log = { } }; -void pcie_aer_inject_error_print (Monitor *mon,const QObject *data) -{ -QDict *qdict; -int devfn; -assert (qobject_type (data) ==QTYPE_QDICT ); - qdict =qobject_to_qdict (data); - - devfn = (int )qdict_get_int (qdict,"devfn" ); -monitor_printf (mon,"OK id: %s root bus: %s, bus: %x devfn: %x.%x\n" , -qdict_get_str (qdict,"id" ), -qdict_get_str (qdict,"root_bus" ), - (int )qdict_get_int (qdict,"bus" ), -PCI_SLOT (devfn),PCI_FUNC (devfn)); -} - typedef struct PCIEAERErrorName { const char *name; uint32_t val;
@@ -962,8 +947,8 @@ static int pcie_aer_parse_error_string(const char *error_name,return -EINVAL ; } -int hmp_pcie_aer_inject_error (Monitor *mon, -const QDict *qdict,QObject **ret_data) +static int do_pcie_aer_inject_error (Monitor *mon, +const QDict *qdict,QObject **ret_data) {const char *id =qdict_get_str (qdict,"id" );const char *error_name;
@@ -1035,3 +1020,23 @@ int hmp_pcie_aer_inject_error(Monitor *mon,return 0 ; } + +void hmp_pcie_aer_inject_error (Monitor *mon,const QDict *qdict) +{ +QObject *data; +int devfn; + +if (do_pcie_aer_inject_error (mon, qdict, &data) <0 ) { +return ; + } + +assert (qobject_type (data) ==QTYPE_QDICT ); + qdict =qobject_to_qdict (data); + + devfn = (int )qdict_get_int (qdict,"devfn" ); +monitor_printf (mon,"OK id: %s root bus: %s, bus: %x devfn: %x.%x\n" , +qdict_get_str (qdict,"id" ), +qdict_get_str (qdict,"root_bus" ), + (int )qdict_get_int (qdict,"bus" ), +PCI_SLOT (devfn),PCI_FUNC (devfn)); +}
include/sysemu/sysemu.h +1 -3
@@ -161,9 +161,7 @@ extern unsigned int nb_prom_envs;void hmp_drive_add (Monitor *mon,const QDict *qdict);/* pcie aer error injection */ -void pcie_aer_inject_error_print (Monitor *mon,const QObject *data); -int hmp_pcie_aer_inject_error (Monitor *mon, -const QDict *qdict,QObject **ret_data); +void hmp_pcie_aer_inject_error (Monitor *mon,const QDict *qdict);/* serial ports */
Signature · 1 line
1.9.3
Re: [Qemu-devel] [PATCH 07/20] monitor: Use trad. command interface for HMP pcie_aer_inject_error
Quoted 28 lines · Markus Armbruster
All QMP commands use the "new" handler interface (mhandler.cmd_new). Most HMP commands still use the traditional interface (mhandler.cmd), but a few use the "new" one. Complicates
handle_user_command()for no gain, so I'm converting these to the traditional interface.pcie_aer_inject_error's implementation is split into the
hmp_pcie_aer_inject_error()andpcie_aer_inject_error_print(). The former is a peculiar crossbreed between HMP and QMP handler. On success, it works like a QMP handler: store QDict throughret_dataparameter, return 0. Printing the QDict is left topcie_aer_inject_error_print(). On failure, it works more like an HMP
handler: print error to monitor, return negative number.To convert to the traditional interface, turn
pcie_aer_inject_error_print()into a command handler wrapping aroundhmp_pcie_aer_inject_error(). By convention, this command handler should be calledhmp_pcie_aer_inject_error(), so rename the existinghmp_pcie_aer_inject_error()todo_pcie_aer_inject_error().Signed-off-by Markus Armbruster <[email protected]>--- hmp-commands.hx | 3 +-- hw/pci/pci-stub.c | 14 +------------- hw/pci/pcie_aer.c | 39 ++++++++++++++++++++++----------------- include/sysemu/sysemu.h | 4 +--- 4 files changed, 25 insertions(+), 35 deletions(-)
Review
Quoted 2 lines
(quoted diff) +2
-0+ + devfn = (int)qdict_get_int(qdict, "devfn");
Code motion, so the problem is pre-existing, but this cast is unneeded.
Up to you if you want to clean it up now.
hw/pci/pcie_aer.c int hmp_pcie_aer_inject_error(Monitor *mon, · line 1040
Quoted 4 lines
hw/pci/pcie_aer.c +4
-0+ monitor_printf(mon, "OK id: %s root bus: %s, bus: %x devfn: %x.%x\n", + qdict_get_str(qdict, "id"), + qdict_get_str(qdict, "root_bus"), + (int) qdict_get_int(qdict, "bus"),
However, this one still is necessary, unless you tweak the format string (if you haven't guessed, I prefer avoiding casts when other mechanisms work equally well).
But that's minor; whether or not you clean things up,
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Re: [Qemu-devel] [PATCH 07/20] monitor: Use trad. command interface for HMP pcie_aer_inject_error
Quoted 29 lines · Eric Blake
Quoted 27 lines · Markus Armbruster
All QMP commands use the "new" handler interface (mhandler.cmd_new). Most HMP commands still use the traditional interface (mhandler.cmd), but a few use the "new" one. Complicates
handle_user_command()for no gain, so I'm converting these to the traditional interface.pcie_aer_inject_error's implementation is split into the
hmp_pcie_aer_inject_error()andpcie_aer_inject_error_print(). The former is a peculiar crossbreed between HMP and QMP handler. On success, it works like a QMP handler: store QDict throughret_dataparameter, return 0. Printing the QDict is left topcie_aer_inject_error_print(). On failure, it works more like an HMP
handler: print error to monitor, return negative number.To convert to the traditional interface, turn
pcie_aer_inject_error_print()into a command handler wrapping aroundhmp_pcie_aer_inject_error(). By convention, this command handler should be calledhmp_pcie_aer_inject_error(), so rename the existinghmp_pcie_aer_inject_error()todo_pcie_aer_inject_error().Signed-off-by Markus Armbruster <[email protected]>--- hmp-commands.hx | 3 +-- hw/pci/pci-stub.c | 14 +------------- hw/pci/pcie_aer.c | 39 ++++++++++++++++++++++----------------- include/sysemu/sysemu.h | 4 +--- 4 files changed, 25 insertions(+), 35 deletions(-)
Quoted 5 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ + devfn = (int)qdict_get_int(qdict, "devfn");Code motion, so the problem is pre-existing, but this cast is unneeded.
Up to you if you want to clean it up now.
While I occasionally do fold cleanups into mechanical changes, e.g in PATCH 15, I'm reluctant to fold them into code motion.
Quoted 8 lines
hw/pci/pcie_aer.c int hmp_pcie_aer_inject_error(Monitor *mon, · line 1040
Quoted 4 lines
hw/pci/pcie_aer.c +4
-0+ monitor_printf(mon, "OK id: %s root bus: %s, bus: %x devfn: %x.%x\n", + qdict_get_str(qdict, "id"), + qdict_get_str(qdict, "root_bus"), + (int) qdict_get_int(qdict, "bus"),However, this one still is necessary, unless you tweak the format string (if you haven't guessed, I prefer avoiding casts when other mechanisms work equally well).
I share your preference, but I want to keep the changes in PCI code to a trivial minimum, to keep this series firmly in the monitor subsystem.
Quoted 3 lines
But that's minor; whether or not you clean things up,
Reviewed-by Eric Blake <[email protected]>
Thanks!
Re: [Qemu-devel] [PATCH 07/20] monitor: Use trad. command interface for HMP pcie_aer_inject_error
Quoted 10 lines · Markus Armbruster
Quoted 8 lines · Eric Blake
Quoted 5 lines · Markus Armbruster
All QMP commands use the "new" handler interface (mhandler.cmd_new). Most HMP commands still use the traditional interface (mhandler.cmd), but a few use the "new" one. Complicates
handle_user_command()for no gain, so I'm converting these to the traditional interface.
Quoted 8 lines
Quoted 5 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ + devfn = (int)qdict_get_int(qdict, "devfn");Code motion, so the problem is pre-existing, but this cast is unneeded.
Up to you if you want to clean it up now.While I occasionally do fold cleanups into mechanical changes, e.g in PATCH 15, I'm reluctant to fold them into code motion.
I totally agree - mixing cleanups into code motion makes the motion harder to review. If it gets cleaned up, it should be as a separate patch, at which point it starts to be out of scope of the goal of this series, and better as a separate patch to qemu-trivial or to the PCI tree.
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
PATCH 8/20 monitor: Drop unused "new" HMP command interface
In reply to Markus Armbruster--- monitor.c | 22 +--------------------- 1 file changed, 1 insertion(+), 21 deletions(-)
monitor.c +1 -21
@@ -123,7 +123,6 @@ typedef struct mon_cmd_t {const char *args_type;const char *params;const char *help; -void (*user_print)(Monitor *mon,const QObject *data);union {void (*cmd)(Monitor *mon,const QDict *qdict);int (*cmd_new)(Monitor *mon,const QDict *params,QObject **ret_data);
@@ -378,11 +377,6 @@ static int GCC_FMT_ATTR(2, 3) monitor_fprintf(FILE *stream,return 0 ; } -static inline int handler_is_qobject (const mon_cmd_t *cmd) -{ -return cmd->user_print !=NULL ; -} -static inline int monitor_has_error (const Monitor *mon) {return mon->error !=NULL ;
@@ -4045,24 +4039,10 @@ static void handle_user_command(Monitor *mon, const char *cmdline) qdict =qdict_new (); cmd =monitor_parse_command (mon, cmdline,0 , mon->cmd_table, qdict); -if (!cmd) - goto out; - -if (handler_is_qobject (cmd)) { -QObject *data =NULL ; - -/* XXX: ignores the error code */ - cmd->mhandler.cmd_new (mon, qdict, &data); -assert (!monitor_has_error (mon)); -if (data) { - cmd->user_print (mon, data); -qobject_decref (data); - } - }else { +if (cmd) { cmd->mhandler.cmd (mon, qdict); } -out:QDECREF (qdict); }
Signature · 1 line
1.9.3
Re: [Qemu-devel] [PATCH 08/20] monitor: Drop unused "new" HMP command interface
Quoted 5 lines · Markus Armbruster
Signed-off-by Markus Armbruster <[email protected]>--- monitor.c | 22 +--------------------- 1 file changed, 1 insertion(+), 21 deletions(-)
Goodbye! Nice knowing you! (For how many incomplete conversions we have scattered throughout the tree, it's interesting to know that this one was easier to get rid of by reverting instead of finishing the conversion)
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
PATCH 9/20 monitor: Propagate errors through qmp_check_client_args()
In reply to Markus Armbruster--- monitor.c | 65 ++++++++++++++++++++++++++++++++------------------------------- 1 file changed, 33 insertions(+), 32 deletions(-)
monitor.c +33 -32
@@ -4736,8 +4736,9 @@ static bool invalid_qmp_mode(const Monitor *mon, const mon_cmd_t *cmd)* the QMP_ACCEPT_UNKNOWNS flag is set, then the * checking is skipped for it. */ -static int check_client_args_type (const QDict *client_args, -const QDict *cmd_args,int flags) +static void check_client_args_type (const QDict *client_args, +const QDict *cmd_args,int flags, +Error **errp) {const QDictEntry *ent;
@@ -4754,8 +4755,8 @@ static int check_client_args_type(const QDict *client_args,continue ; }/* client arg doesn't exist */ -qerror_report (QERR_INVALID_PARAMETER , client_arg_name); -return -1 ; +error_set (errp,QERR_INVALID_PARAMETER , client_arg_name); +return ; } arg_type =qobject_to_qstring (obj);
@@ -4767,9 +4768,9 @@ static int check_client_args_type(const QDict *client_args,case 'B' :case 's' :if (qobject_type (client_arg) !=QTYPE_QSTRING ) { -qerror_report (QERR_INVALID_PARAMETER_TYPE , client_arg_name, -"string" ); -return -1 ; +error_set (errp,QERR_INVALID_PARAMETER_TYPE , + client_arg_name,"string" ); +return ; }break ;case 'i' :
@@ -4777,25 +4778,25 @@ static int check_client_args_type(const QDict *client_args,case 'M' :case 'o' :if (qobject_type (client_arg) !=QTYPE_QINT ) { -qerror_report (QERR_INVALID_PARAMETER_TYPE , client_arg_name, -"int" ); -return -1 ; +error_set (errp,QERR_INVALID_PARAMETER_TYPE , + client_arg_name,"int" ); +return ; }break ;case 'T' :if (qobject_type (client_arg) !=QTYPE_QINT &&qobject_type (client_arg) !=QTYPE_QFLOAT ) { -qerror_report (QERR_INVALID_PARAMETER_TYPE , client_arg_name, -"number" ); -return -1 ; +error_set (errp,QERR_INVALID_PARAMETER_TYPE , + client_arg_name,"number" ); +return ; }break ;case 'b' :case '-' :if (qobject_type (client_arg) !=QTYPE_QBOOL ) { -qerror_report (QERR_INVALID_PARAMETER_TYPE , client_arg_name, -"bool" ); -return -1 ; +error_set (errp,QERR_INVALID_PARAMETER_TYPE , + client_arg_name,"bool" ); +return ; }break ;case 'O' :
@@ -4814,16 +4815,15 @@ static int check_client_args_type(const QDict *client_args,abort (); } } - -return 0 ; }/* * - Check if the client has passed all mandatory args * - Set special flags for argument validation */ -static int check_mandatory_args (const QDict *cmd_args, -const QDict *client_args,int *flags) +static void check_mandatory_args (const QDict *cmd_args, +const QDict *client_args,int *flags, +Error **errp) {const QDictEntry *ent;
@@ -4838,12 +4838,10 @@ static int check_mandatory_args(const QDict *cmd_args, }else if (qstring_get_str (type)[0 ] !='-' &&qstring_get_str (type)[1 ] !='?' && !qdict_haskey (client_args, cmd_arg_name)) { -qerror_report (QERR_MISSING_PARAMETER , cmd_arg_name); -return -1 ; +error_set (errp,QERR_MISSING_PARAMETER , cmd_arg_name); +return ; } } - -return 0 ; }static QDict *qdict_from_args_type (const char *args_type)
@@ -4899,24 +4897,26 @@ out:* 3. Each argument provided by the client must have the type expected * by the command */ -static int qmp_check_client_args (const mon_cmd_t *cmd,QDict *client_args) +static void qmp_check_client_args (const mon_cmd_t *cmd,QDict *client_args, +Error **errp) { -int flags, err; +Error *err =NULL ; +int flags;QDict *cmd_args; cmd_args =qdict_from_args_type (cmd->args_type); flags =0 ; - err =check_mandatory_args (cmd_args, client_args, &flags); +check_mandatory_args (cmd_args, client_args, &flags, &err);if (err) { goto out; } - err =check_client_args_type (client_args, cmd_args, flags); +check_client_args_type (client_args, cmd_args, flags, &err); out: +error_propagate (errp, err);QDECREF (cmd_args); -return err; }/*
@@ -4977,7 +4977,7 @@ static QDict *qmp_check_input_obj(QObject *input_obj)static void handle_qmp_command (JSONMessageParser *parser,QList *tokens) { -int err; +Error *local_err =NULL ;QObject *obj, *data;QDict *input, *args;const mon_cmd_t *cmd;
@@ -5023,8 +5023,9 @@ static void handle_qmp_command(JSONMessageParser *parser, QList *tokens)QINCREF (args); } - err =qmp_check_client_args (cmd, args); -if (err <0 ) { +qmp_check_client_args (cmd, args, &local_err); +if (local_err) { +qerror_report_err (local_err); goto err_out; }
Signature · 1 line
1.9.3
Re: [Qemu-devel] [PATCH 09/20] monitor: Propagate errors through qmp_check_client_args()
Quoted 5 lines · Markus Armbruster
Signed-off-by Markus Armbruster <[email protected]>--- monitor.c | 65 ++++++++++++++++++++++++++++++++------------------------------- 1 file changed, 33 insertions(+), 32 deletions(-)
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
PATCH 10/20 monitor: Propagate errors through qmp_check_input_obj()
In reply to Markus Armbruster--- monitor.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-)
monitor.c +10 -9
@@ -4929,14 +4929,14 @@ out:* 5. If the "id" key exists, it can be anything (ie. json-value) * 6. Any argument not listed above is considered invalid */ -static QDict *qmp_check_input_obj (QObject *input_obj) +static QDict *qmp_check_input_obj (QObject *input_obj,Error **errp) {const QDictEntry *ent;int has_exec_key =0 ;QDict *input_dict;if (qobject_type (input_obj) !=QTYPE_QDICT ) { -qerror_report (QERR_QMP_BAD_INPUT_OBJECT ,"object" ); +error_set (errp,QERR_QMP_BAD_INPUT_OBJECT ,"object" );return NULL ; }
@@ -4948,27 +4948,27 @@ static QDict *qmp_check_input_obj(QObject *input_obj)if (!strcmp (arg_name,"execute" )) {if (qobject_type (arg_obj) !=QTYPE_QSTRING ) { -qerror_report (QERR_QMP_BAD_INPUT_OBJECT_MEMBER ,"execute" , -"string" ); +error_set (errp,QERR_QMP_BAD_INPUT_OBJECT_MEMBER , +"execute" ,"string" );return NULL ; } has_exec_key =1 ; }else if (!strcmp (arg_name,"arguments" )) {if (qobject_type (arg_obj) !=QTYPE_QDICT ) { -qerror_report (QERR_QMP_BAD_INPUT_OBJECT_MEMBER ,"arguments" , -"object" ); +error_set (errp,QERR_QMP_BAD_INPUT_OBJECT_MEMBER , +"arguments" ,"object" );return NULL ; } }else if (!strcmp (arg_name,"id" )) {/* FIXME: check duplicated IDs for async commands */ }else { -qerror_report (QERR_QMP_EXTRA_MEMBER , arg_name); +error_set (errp,QERR_QMP_EXTRA_MEMBER , arg_name);return NULL ; } }if (!has_exec_key) { -qerror_report (QERR_QMP_BAD_INPUT_OBJECT ,"execute" ); +error_set (errp,QERR_QMP_BAD_INPUT_OBJECT ,"execute" );return NULL ; }
@@ -4994,8 +4994,9 @@ static void handle_qmp_command(JSONMessageParser *parser, QList *tokens) goto err_out; } - input =qmp_check_input_obj (obj); + input =qmp_check_input_obj (obj, &local_err);if (!input) { +qerror_report_err (local_err);qobject_decref (obj); goto err_out; }
Signature · 1 line
1.9.3
Re: [Qemu-devel] [PATCH 10/20] monitor: Propagate errors through qmp_check_input_obj()
monitor.c static QDict *qmp_check_input_obj(QObject *input_obj) · line 4951
Quoted 11 lines · Markus Armbruster
Signed-off-by Markus Armbruster <[email protected]>--- monitor.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-)monitor.c +0
-0@@ -4948,27 +4948,27 @@ static QDict *qmp_check_input_obj(QObject *input_obj) } }else if (!strcmp (arg_name,"id" )) {/* FIXME: check duplicated IDs for async commands */
Is this comment dead, now that you killed async commands? If so, should it be nuked earlier in the series?
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Re: [Qemu-devel] [PATCH 10/20] monitor: Propagate errors through qmp_check_input_obj()
Quoted 7 lines · Eric Blake
Quoted 5 lines · Markus Armbruster
Signed-off-by Markus Armbruster <[email protected]>--- monitor.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-)
Quoted 8 lines
monitor.c static QDict *qmp_check_input_obj(QObject *input_obj) · line 4951
Quoted 5 lines
monitor.c +0
-0@@ -4948,27 +4948,27 @@ static QDict *qmp_check_input_obj(QObject *input_obj) } }else if (!strcmp (arg_name,"id" )) {/* FIXME: check duplicated IDs for async commands */ Is this comment dead, now that you killed async commands? If so, should it be nuked earlier in the series?
I figure you're right. I'll kill the comment together with asynchronous commands.
Quoted 1 line
Reviewed-by Eric Blake <[email protected]>
Thanks!
PATCH 11/20 monitor: Wean monitor_protocol_emitter() off mon->error
In reply to Markus ArmbrusterMove mon->error handling to its caller handle_qmp_command().
--- monitor.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)
monitor.c +7 -6
@@ -407,13 +407,14 @@ static QDict *build_qmp_error_dict(const QError *err)return qobject_to_qdict (obj); } -static void monitor_protocol_emitter (Monitor *mon,QObject *data) +static void monitor_protocol_emitter (Monitor *mon,QObject *data, +QError *err) {QDict *qmp;trace_monitor_protocol_emitter (mon); -if (!monitor_has_error (mon)) { +if (!err) {/* success response */ qmp =qdict_new ();if (data) {
@@ -425,9 +426,7 @@ static void monitor_protocol_emitter(Monitor *mon, QObject *data) } }else {/* error response */ - qmp =build_qmp_error_dict (mon->error); -QDECREF (mon->error); - mon->error =NULL ; + qmp =build_qmp_error_dict (err); }if (mon->mc->id) {
@@ -5039,8 +5038,10 @@ static void handle_qmp_command(JSONMessageParser *parser, QList *tokens) } err_out: -monitor_protocol_emitter (mon, data); +monitor_protocol_emitter (mon, data, mon->error);qobject_decref (data); +QDECREF (mon->error); + mon->error =NULL ;QDECREF (input);QDECREF (args); }
Signature · 1 line
1.9.3
Re: [Qemu-devel] [PATCH 11/20] monitor: Wean monitor_protocol_emitter() off mon->error
Quoted 7 lines · Markus Armbruster
Move
mon->errorhandling to its callerhandle_qmp_command().Signed-off-by Markus Armbruster <[email protected]>--- monitor.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
PATCH 12/20 monitor: Inline monitor_has_error() into its only caller
In reply to Markus Armbruster--- monitor.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)
monitor.c +1 -6
@@ -377,11 +377,6 @@ static int GCC_FMT_ATTR(2, 3) monitor_fprintf(FILE *stream,return 0 ; } -static inline int monitor_has_error (const Monitor *mon) -{ -return mon->error !=NULL ; -} -static void monitor_json_emitter (Monitor *mon,const QObject *data) {QString *json;
@@ -5031,7 +5026,7 @@ static void handle_qmp_command(JSONMessageParser *parser, QList *tokens)if (cmd->mhandler.cmd_new (mon, args, &data)) {/* Command failed... */ -if (!monitor_has_error (mon)) { +if (!mon->error) {/* ... without setting an error, so make one up */ qerror_report (QERR_UNDEFINED_ERROR ); }
Signature · 1 line
1.9.3
Re: [Qemu-devel] [PATCH 12/20] monitor: Inline monitor_has_error() into its only caller
Quoted 10 lines · Markus Armbruster
Signed-off-by Markus Armbruster <[email protected]>--- monitor.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)monitor.c +0
-0
monitor.c static int GCC_FMT_ATTR(2, 3) monitor_fprintf(FILE *stream, · line 382
Quoted 6 lines
monitor.c +0
-3@@ -377,11 +377,6 @@ static int GCC_FMT_ATTR(2, 3) monitor_fprintf(FILE *stream,return 0 ; } -static inline int monitor_has_error (const Monitor *mon) -{ -return mon->error !=NULL ;
Yay - gets rid of a function returning 0 or 1 instead of a proper bool :)
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
PATCH 13/20 monitor: Limit QError use to command handlers
In reply to Markus ArmbrusterThe previous commits narrowed use of QError to handle_qmp_command() and its helpers monitor_protocol_emitter(), build_qmp_error_dict(). Narrow it further to just the command handler call: instead of converting Error to QError throughout handle_qmp_command(), convert the QError gotten from the command handler to Error, and switch the helpers from QError to Error.
--- monitor.c | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-)
monitor.c +14 -12
@@ -391,19 +391,19 @@ static void monitor_json_emitter(Monitor *mon, const QObject *data)QDECREF (json); } -static QDict *build_qmp_error_dict (const QError *err) +static QDict *build_qmp_error_dict (Error *err) {QObject *obj; - obj =qobject_from_jsonf ("{ 'error': { 'class': %s, 'desc': %p } }" , - ErrorClass_lookup[err->err_class], -qerror_human (err)); + obj =qobject_from_jsonf ("{ 'error': { 'class': %s, 'desc': %s } }" , + ErrorClass_lookup[error_get_class (err)], +error_get_pretty (err));return qobject_to_qdict (obj); }static void monitor_protocol_emitter (Monitor *mon,QObject *data, -QError *err) +Error *err) {QDict *qmp;
@@ -4984,13 +4984,12 @@ static void handle_qmp_command(JSONMessageParser *parser, QList *tokens) obj =json_parser_parse (tokens,NULL );if (!obj) {// FIXME: should be triggered in json_parser_parse() -qerror_report (QERR_JSON_PARSING ); +error_set (&local_err,QERR_JSON_PARSING ); goto err_out; } input =qmp_check_input_obj (obj, &local_err);if (!input) { -qerror_report_err (local_err);qobject_decref (obj); goto err_out; }
@@ -5002,8 +5001,8 @@ static void handle_qmp_command(JSONMessageParser *parser, QList *tokens)trace_handle_qmp_command (mon, cmd_name); cmd =qmp_find_cmd (cmd_name);if (!cmd) { -qerror_report (ERROR_CLASS_COMMAND_NOT_FOUND , -"The command %s has not been found" , cmd_name); +error_set (&local_err,ERROR_CLASS_COMMAND_NOT_FOUND , +"The command %s has not been found" , cmd_name); goto err_out; }if (invalid_qmp_mode (mon, cmd)) {
@@ -5020,7 +5019,6 @@ static void handle_qmp_command(JSONMessageParser *parser, QList *tokens)qmp_check_client_args (cmd, args, &local_err);if (local_err) { -qerror_report_err (local_err); goto err_out; }
@@ -5028,12 +5026,16 @@ static void handle_qmp_command(JSONMessageParser *parser, QList *tokens)/* Command failed... */ if (!mon->error) {/* ... without setting an error, so make one up */ -qerror_report (QERR_UNDEFINED_ERROR ); +error_set (&local_err,QERR_UNDEFINED_ERROR ); } } +if (mon->error) { +error_set (&local_err, mon->error->err_class,"%s" , + mon->error->err_msg); + } err_out: -monitor_protocol_emitter (mon, data, mon->error); +monitor_protocol_emitter (mon, data, local_err);qobject_decref (data);QDECREF (mon->error); mon->error =NULL ;
Signature · 1 line
1.9.3
Re: [Qemu-devel] [PATCH 13/20] monitor: Limit QError use to command handlers
Quoted 21 lines · Markus Armbruster
The previous commits narrowed use of QError to
handle_qmp_command()and its helpersmonitor_protocol_emitter(),build_qmp_error_dict(). Narrow it further to just the command handler call: instead of converting Error to QError throughouthandle_qmp_command(), convert the QError gotten from the command handler to Error, and switch the helpers from QError to Error.Signed-off-by Markus Armbruster <[email protected]>--- monitor.c | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-)monitor.c +0
-1@@ -391,19 +391,19 @@ static void monitor_json_emitter(Monitor *mon, const QObject *data)QDECREF (json); } -static QDict *build_qmp_error_dict (const QError *err)
monitor.c static void monitor_json_emitter(Monitor *mon, const QObject *data) · line 400
Quoted 10 lines
monitor.c +4
-3+static QDict *build_qmp_error_dict (Error *err) {QObject *obj; - obj =qobject_from_jsonf ("{ 'error': { 'class': %s, 'desc': %p } }" , - ErrorClass_lookup[err->err_class], -qerror_human (err)); + obj =qobject_from_jsonf ("{ 'error': { 'class': %s, 'desc': %s } }" , + ErrorClass_lookup[error_get_class (err)], +error_get_pretty (err));
You're getting rid of one of three uses of '_jsonf.*%p' in the tree (two in monitor.c, one in tests/check-qjson.c) [I didn't check for multi-line instances where the format string was on a different line than the function call]. Is it worth completely getting rid of the %p formatter, and simplifying qobject/json-*.c accordingly, in a later patch?
[oh, and I hate that I could not find documentation on the format strings accepted by qobject_from_jsonf; it took me forever to track down that %i correlated to bool, when I was doing my series for "qobject" Use 'bool' for qbool]
monitor.c static void handle_qmp_command(JSONMessageParser *parser, QList *tokens) · line 4988
Quoted 7 lines
monitor.c +1
-1@@ -4984,13 +4984,12 @@ static void handle_qmp_command(JSONMessageParser *parser, QList *tokens) obj =json_parser_parse (tokens,NULL );if (!obj) {// FIXME: should be triggered in json_parser_parse() -qerror_report (QERR_JSON_PARSING ); +error_set (&local_err,QERR_JSON_PARSING ); goto err_out;
Is this FIXME still valid, or are we reaching the point where it could be replaced by an assert(obj) in a later patch?
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Re: [Qemu-devel] [PATCH 13/20] monitor: Limit QError use to command handlers
Quoted 26 lines · Eric Blake
Quoted 25 lines · Markus Armbruster
The previous commits narrowed use of QError to
handle_qmp_command()and its helpersmonitor_protocol_emitter(),build_qmp_error_dict(). Narrow it further to just the command handler call: instead of converting Error to QError throughouthandle_qmp_command(), convert the QError gotten from the command handler to Error, and switch the helpers from QError to Error.Signed-off-by Markus Armbruster <[email protected]>--- monitor.c | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-)monitor.c +1
-1@@ -391,19 +391,19 @@ static void monitor_json_emitter(Monitor *mon, const QObject *data)QDECREF (json); } -static QDict *build_qmp_error_dict (const QError *err) +static QDict *build_qmp_error_dict (Error *err) {QObject *obj;
Quoted 12 lines
monitor.c static void monitor_json_emitter(Monitor *mon, const QObject *data) · line 400
Quoted 6 lines
monitor.c +3
-3- obj =qobject_from_jsonf ("{ 'error': { 'class': %s, 'desc': %p } }" , - ErrorClass_lookup[err->err_class], -qerror_human (err)); + obj =qobject_from_jsonf ("{ 'error': { 'class': %s, 'desc': %s } }" , + ErrorClass_lookup[error_get_class (err)], +error_get_pretty (err));You're getting rid of one of three uses of '_jsonf.*%p' in the tree (two in
monitor.c, one intests/check-qjson.c) [I didn't check for multi-line instances where the format string was on a different line than the function call]. Is it worth completely getting rid of the %p formatter, and simplifyingqobject/json-*.caccordingly, in a later patch?
No idea :)
Here, %p removal simply falls out of the QError -> Error conversion. I don't know how it's used elsewhere, and whether the uses are worth the code backing it.
Quoted 4 lines
[oh, and I hate that I could not find documentation on the format strings accepted by
qobject_from_jsonf; it took me forever to track down that %i correlated to bool, when I was doing my series for "qobject" Use 'bool' for qbool]
Valid complaint. Pretty much everything from that particular hacking bout is underdocumented, but the undocumented conversion specifiers are particularly egregious.
Quoted 10 lines
monitor.c static void handle_qmp_command(JSONMessageParser *parser, QList *tokens) · line 4988
Quoted 7 lines
monitor.c +1
-1@@ -4984,13 +4984,12 @@ static void handle_qmp_command(JSONMessageParser *parser, QList *tokens) obj =json_parser_parse (tokens,NULL );if (!obj) {// FIXME: should be triggered in json_parser_parse() -qerror_report (QERR_JSON_PARSING ); +error_set (&local_err,QERR_JSON_PARSING ); goto err_out;Is this FIXME still valid, or are we reaching the point where it could be replaced by an assert(obj) in a later patch?
Yes, it's still valid. Watch this:
{"QMP": {"version": {"qemu": {"micro": 94, "minor": 2, "major": 2}, "package": ""}, "capabilities": []}}
{ "execute": "\xxxxxxxxx" }
{"error": {"class": "GenericError", "desc": "Invalid JSON syntax"}}
{"error": {"class": "GenericError", "desc": "Invalid JSON syntax"}}
{ "execute": "qmp_capabilities" }
{"error": {"class": "GenericError", "desc": "Expected 'object' in QMP input"}}
{"error": {"class": "GenericError", "desc": "Invalid JSON syntax"}}
{"error": {"class": "GenericError", "desc": "Expected 'object' in QMP input"}}
{"error": {"class": "GenericError", "desc": "Invalid JSON syntax"}}
{"error": {"class": "GenericError", "desc": "Invalid JSON syntax"}}
{"error": {"class": "GenericError", "desc": "Invalid JSON syntax"}}This does enter json-parser.c's
parse_error(ctxt, token,
"invalid hex escape sequence in string");and parse_error() duly creates an Error object with error_setg(), but the object then gets lost somewhere on the way.
Also visible: multiple error replies, which is wrong, and an apparent inability to recover from syntax error.
Not just the documentation sucks. But then half-assed documentation has always been a pretty reliable indicator of half-assed code.
Quoted 1 line
Reviewed-by Eric Blake <[email protected]>
Thanks!
PATCH 14/20 monitor: Rename handle_user_command() to handle_hmp_command()
In reply to Markus Armbruster--- monitor.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
monitor.c +5 -5
@@ -574,7 +574,7 @@ static int do_qmp_capabilities(Monitor *mon, const QDict *params,return 0 ; } -static void handle_user_command (Monitor *mon,const char *cmdline); +static void handle_hmp_command (Monitor *mon,const char *cmdline);static void monitor_data_init (Monitor *mon) {
@@ -613,7 +613,7 @@ char *qmp_human_monitor_command(const char *command_line, bool has_cpu_index, } } -handle_user_command (&hmp, command_line); +handle_hmp_command (&hmp, command_line); cur_mon = old_mon;qemu_mutex_lock (&hmp.out_lock);
@@ -4025,7 +4025,7 @@ void monitor_set_error(Monitor *mon, QError *qerror) } } -static void handle_user_command (Monitor *mon,const char *cmdline) +static void handle_hmp_command (Monitor *mon,const char *cmdline) {QDict *qdict;const mon_cmd_t *cmd;
@@ -5071,7 +5071,7 @@ static void monitor_read(void *opaque, const uint8_t *buf, int size)if (size ==0 || buf[size -1 ] !=0 )monitor_printf (cur_mon,"corrupted command\n" );else -handle_user_command (cur_mon, (char *)buf); +handle_hmp_command (cur_mon, (char *)buf); } cur_mon = old_mon;
@@ -5083,7 +5083,7 @@ static void monitor_command_cb(void *opaque, const char *cmdline,Monitor *mon = opaque;monitor_suspend (mon); -handle_user_command (mon, cmdline); +handle_hmp_command (mon, cmdline);monitor_resume (mon); }
Signature · 1 line
1.9.3
Re: [Qemu-devel] [PATCH 14/20] monitor: Rename handle_user_command() to handle_hmp_command()
Quoted 5 lines · Markus Armbruster
Signed-off-by Markus Armbruster <[email protected]>--- monitor.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
PATCH 15/20 monitor: Rename monitor_control_read(), monitor_control_event()
In reply to Markus Armbruster... to monitor_qmp_read(), monitor_qmp_event().
--- monitor.c | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-)
monitor.c +4 -10
@@ -5043,10 +5043,7 @@ err_out:QDECREF (args); } -/** -* monitor_control_read(): Read and handle QMP input -*/ -static void monitor_control_read (void *opaque,const uint8_t *buf,int size) +static void monitor_qmp_read (void *opaque,const uint8_t *buf,int size) {Monitor *old_mon = cur_mon;
@@ -5111,10 +5108,7 @@ static QObject *get_qmp_greeting(void)return qobject_from_jsonf ("{'QMP':{'version': %p,'capabilities': []}}" ,ver); } -/** -* monitor_control_event(): Print QMP gretting -*/ -static void monitor_control_event (void *opaque,int event) +static void monitor_qmp_event (void *opaque,int event) {QObject *data;Monitor *mon = opaque;
@@ -5264,8 +5258,8 @@ void monitor_init(CharDriverState *chr, int flags)if (monitor_ctrl_mode (mon)) { mon->mc =g_malloc0 (sizeof (MonitorControl));/* Control mode requires special handlers */ -qemu_chr_add_handlers (chr, monitor_can_read, monitor_control_read, - monitor_control_event, mon); +qemu_chr_add_handlers (chr, monitor_can_read, monitor_qmp_read, + monitor_qmp_event, mon);qemu_chr_fe_set_echo (chr, true);json_message_parser_init (&mon->mc->parser, handle_qmp_command);
Signature · 1 line
1.9.3
Re: [Qemu-devel] [PATCH 15/20] monitor: Rename monitor_control_read(), monitor_control_event()
Quoted 6 lines · Markus Armbruster
... to
monitor_qmp_read(),monitor_qmp_event().Signed-off-by Markus Armbruster <[email protected]>--- monitor.c | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-)
Not a trivial rename based on diffstat, but...
Quoted 6 lines
monitor.c +0
-0
monitor.c err_out: · line 5046
Quoted 8 lines
monitor.c +1
-4@@ -5043,10 +5043,7 @@ err_out:QDECREF (args); } -/** -* monitor_control_read(): Read and handle QMP input -*/ -static void monitor_control_read (void *opaque,const uint8_t *buf,int size) +static void monitor_qmp_read (void *opaque,const uint8_t *buf,int size)
Drops a comment, but the comment didn't add much.
monitor.c static QObject *get_qmp_greeting(void) · line 5111
Quoted 12 lines
monitor.c +1
-4{Monitor *old_mon = cur_mon;@@ -5111,10 +5108,7 @@ static QObject *get_qmp_greeting(void)return qobject_from_jsonf ("{'QMP':{'version': %p,'capabilities': []}}" ,ver); } -/** -* monitor_control_event(): Print QMP gretting -*/ -static void monitor_control_event (void *opaque,int event) +static void monitor_qmp_event (void *opaque,int event)
Drops a typo in the otherwise useless comment. (I'm surprised numerous scans for spelling cleanups haven't yet flagged "gretting" as unusual).
...so I'm okay with it.
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
PATCH 16/20 monitor: Unbox Monitor member mc and rename to qmp
In reply to Markus ArmbrusterWhile there, rename its type as well, from MonitorControl to MonitorQMP.
--- monitor.c | 35 ++++++++++++++++------------------- 1 file changed, 16 insertions(+), 19 deletions(-)
monitor.c +16 -19
@@ -161,11 +161,11 @@ struct MonFdset {QLIST_ENTRY (MonFdset) next; }; -typedef struct MonitorControl { +typedef struct {QObject *id;JSONMessageParser parser;int command_mode; -}MonitorControl ; +}MonitorQMP ;/* * To prevent flooding clients, events can be throttled. The
@@ -195,7 +195,7 @@ struct Monitor {int mux_out;ReadLineState *rs; -MonitorControl *mc; +MonitorQMP qmp;CPUState *mon_cpu;BlockCompletionFunc *password_completion_cb;void *password_opaque;
@@ -228,7 +228,7 @@ static void monitor_command_cb(void *opaque, const char *cmdline,static inline int qmp_cmd_mode (const Monitor *mon) { -return (mon->mc ? mon->mc->command_mode :0 ); +return mon->qmp.command_mode; }/* Return true if in control mode, false otherwise */
@@ -424,9 +424,9 @@ static void monitor_protocol_emitter(Monitor *mon, QObject *data, qmp =build_qmp_error_dict (err); } -if (mon->mc->id) { -qdict_put_obj (qmp,"id" , mon->mc->id); - mon->mc->id =NULL ; +if (mon->qmp.id) { +qdict_put_obj (qmp,"id" , mon->qmp.id); + mon->qmp.id =NULL ; }monitor_json_emitter (mon,QOBJECT (qmp));
@@ -568,7 +568,7 @@ static int do_qmp_capabilities(Monitor *mon, const QDict *params, {/* Will setup QMP capabilities in the future */ if (monitor_ctrl_mode (mon)) { - mon->mc->command_mode =1 ; + mon->qmp.command_mode =1 ; }return 0 ;
@@ -4994,8 +4994,8 @@ static void handle_qmp_command(JSONMessageParser *parser, QList *tokens) goto err_out; } - mon->mc->id =qdict_get (input,"id" ); -qobject_incref (mon->mc->id); + mon->qmp.id =qdict_get (input,"id" ); +qobject_incref (mon->qmp.id); cmd_name =qdict_get_str (input,"execute" );trace_handle_qmp_command (mon, cmd_name);
@@ -5049,7 +5049,7 @@ static void monitor_qmp_read(void *opaque, const uint8_t *buf, int size) cur_mon = opaque; -json_message_parser_feed (&cur_mon->mc->parser, (const char *) buf, size); +json_message_parser_feed (&cur_mon->qmp.parser, (const char *) buf, size); cur_mon = old_mon; }
@@ -5115,15 +5115,15 @@ static void monitor_qmp_event(void *opaque, int event)switch (event) {case CHR_EVENT_OPENED : - mon->mc->command_mode =0 ; + mon->qmp.command_mode =0 ; data =get_qmp_greeting ();monitor_json_emitter (mon, data);qobject_decref (data); mon_refcount++;break ;case CHR_EVENT_CLOSED : -json_message_parser_destroy (&mon->mc->parser); -json_message_parser_init (&mon->mc->parser, handle_qmp_command); +json_message_parser_destroy (&mon->qmp.parser); +json_message_parser_init (&mon->qmp.parser, handle_qmp_command); mon_refcount--;monitor_fdsets_cleanup ();break ;
@@ -5255,14 +5255,11 @@ void monitor_init(CharDriverState *chr, int flags)monitor_read_command (mon,0 ); } -if (monitor_ctrl_mode (mon)) { - mon->mc =g_malloc0 (sizeof (MonitorControl)); -/* Control mode requires special handlers */ +if (flags &MONITOR_USE_CONTROL ) {qemu_chr_add_handlers (chr, monitor_can_read, monitor_qmp_read, monitor_qmp_event, mon);qemu_chr_fe_set_echo (chr, true); - -json_message_parser_init (&mon->mc->parser, handle_qmp_command); +json_message_parser_init (&mon->qmp.parser, handle_qmp_command); }else {qemu_chr_add_handlers (chr, monitor_can_read, monitor_read, monitor_event, mon);
Signature · 1 line
1.9.3
Re: [Qemu-devel] [PATCH 16/20] monitor: Unbox Monitor member mc and rename to qmp
Quoted 10 lines · Markus Armbruster
While there, rename its type as well, from MonitorControl to MonitorQMP.
Signed-off-by Markus Armbruster <[email protected]>--- monitor.c | 35 ++++++++++++++++------------------- 1 file changed, 16 insertions(+), 19 deletions(-)
monitor.c void monitor_init(CharDriverState *chr, int flags) · line 5258
Quoted 7 lines
monitor.c +1
-3@@ -5255,14 +5255,11 @@ void monitor_init(CharDriverState *chr, int flags)monitor_read_command (mon,0 ); } -if (monitor_ctrl_mode (mon)) { - mon->mc =g_malloc0 (sizeof (MonitorControl)); -/* Control mode requires special handlers */ +if (flags &MONITOR_USE_CONTROL ) {
You also inlined this use of monitor_ctrl_mode; worth mentioning in the commit message? Or should it have been swapped to monitor_is_qmp() in patch 19?
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Re: [Qemu-devel] [PATCH 16/20] monitor: Unbox Monitor member mc and rename to qmp
Quoted 14 lines · Eric Blake
Quoted 13 lines · Markus Armbruster
While there, rename its type as well, from MonitorControl to MonitorQMP.
Signed-off-by Markus Armbruster <[email protected]>--- monitor.c | 35 ++++++++++++++++------------------- 1 file changed, 16 insertions(+), 19 deletions(-)monitor.c +0
-0@@ -5255,14 +5255,11 @@ void monitor_init(CharDriverState *chr, int flags)monitor_read_command (mon,0 ); }
Quoted 8 lines
monitor.c void monitor_init(CharDriverState *chr, int flags) · line 5258
Quoted 4 lines
monitor.c +1
-3-if (monitor_ctrl_mode (mon)) { - mon->mc =g_malloc0 (sizeof (MonitorControl)); -/* Control mode requires special handlers */ +if (flags &MONITOR_USE_CONTROL ) {You also inlined this use of
monitor_ctrl_mode; worth mentioning in the commit message? Or should it have been swapped tomonitor_is_qmp()in patch 19?
I'll do either one or the other.
Quoted 1 line
Reviewed-by Eric Blake <[email protected]>
Thanks!
PATCH 17/20 monitor: Drop do_qmp_capabilities()'s superfluous QMP check
In reply to Markus ArmbrusterSuperfluous since commit 30f5041 removed it from HMP.
--- monitor.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)
monitor.c +1 -5
@@ -566,11 +566,7 @@ static void monitor_qapi_event_init(void)static int do_qmp_capabilities (Monitor *mon,const QDict *params,QObject **ret_data) { -/* Will setup QMP capabilities in the future */ -if (monitor_ctrl_mode (mon)) { - mon->qmp.command_mode =1 ; - } - + mon->qmp.command_mode =1 ;return 0 ; }
Signature · 1 line
1.9.3
Re: [Qemu-devel] [PATCH 17/20] monitor: Drop do_qmp_capabilities()'s superfluous QMP check
Quoted 6 lines · Markus Armbruster
Superfluous since commit 30f5041 removed it from HMP.
Signed-off-by Markus Armbruster <[email protected]>--- monitor.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
PATCH 18/20 monitor: Turn int command_mode into bool in_command_mode
In reply to Markus Armbruster--- monitor.c | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-)
monitor.c +12 -11
@@ -164,7 +164,12 @@ struct MonFdset {typedef struct {QObject *id;JSONMessageParser parser; -int command_mode; +/* +* When a client connects, we're in capabilities negotiation mode. +* When command qmp_capabilities succeeds, we go into command +* mode. +*/ +bool in_command_mode;/* are we in command mode? */ }MonitorQMP ;/*
@@ -226,11 +231,6 @@ Monitor *default_mon;static void monitor_command_cb (void *opaque,const char *cmdline,void *readline_opaque); -static inline int qmp_cmd_mode (const Monitor *mon) -{ -return mon->qmp.command_mode; -} -/* Return true if in control mode, false otherwise */ static inline int monitor_ctrl_mode (const Monitor *mon) {
@@ -446,7 +446,7 @@ static void monitor_qapi_event_emit(QAPIEvent event, QObject *data)trace_monitor_protocol_event_emit (event, data); QLIST_FOREACH(mon, &mon_list, entry) { -if (monitor_ctrl_mode (mon) &&qmp_cmd_mode (mon)) { +if (monitor_ctrl_mode (mon) && mon->qmp.in_command_mode) {monitor_json_emitter (mon, data); } }
@@ -566,7 +566,7 @@ static void monitor_qapi_event_init(void)static int do_qmp_capabilities (Monitor *mon,const QDict *params,QObject **ret_data) { - mon->qmp.command_mode =1 ; + mon->qmp.in_command_mode = true;return 0 ; }
@@ -4701,13 +4701,14 @@ static int monitor_can_read(void *opaque)static bool invalid_qmp_mode (const Monitor *mon,const mon_cmd_t *cmd) {bool is_cap = cmd->mhandler.cmd_new == do_qmp_capabilities; -if (is_cap &&qmp_cmd_mode (mon)) { + +if (is_cap && mon->qmp.in_command_mode) {qerror_report (ERROR_CLASS_COMMAND_NOT_FOUND ,"Capabilities negotiation is already complete, command " "'%s' ignored" , cmd->name);return true; } -if (!is_cap && !qmp_cmd_mode (mon)) { +if (!is_cap && !mon->qmp.in_command_mode) {qerror_report (ERROR_CLASS_COMMAND_NOT_FOUND ,"Expecting capabilities negotiation with " "'qmp_capabilities' before command '%s'" , cmd->name);
@@ -5111,7 +5112,7 @@ static void monitor_qmp_event(void *opaque, int event)switch (event) {case CHR_EVENT_OPENED : - mon->qmp.command_mode =0 ; + mon->qmp.in_command_mode = false; data =get_qmp_greeting ();monitor_json_emitter (mon, data);qobject_decref (data);
Signature · 1 line
1.9.3
Re: [Qemu-devel] [PATCH 18/20] monitor: Turn int command_mode into bool in_command_mode
Quoted 5 lines · Markus Armbruster
Signed-off-by Markus Armbruster <[email protected]>--- monitor.c | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-)
Might be worth mentioning in the commit message that...
monitor.c Monitor *default_mon; · line 232
Quoted 5 lines
monitor.c +0
-4-static inline int qmp_cmd_mode (const Monitor *mon) -{ -return mon->qmp.command_mode; -}
...qmp_cmd_mode() is now inlined into its callers.
Nice, fixes up a mis-use of 'int' that I complained about when writing commit 2d5a8346
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Re: [Qemu-devel] [PATCH 18/20] monitor: Turn int command_mode into bool in_command_mode
Quoted 10 lines · Eric Blake
Quoted 5 lines · Markus Armbruster
Signed-off-by Markus Armbruster <[email protected]>--- monitor.c | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-)Might be worth mentioning in the commit message that...
Quoted 1 line
Quoted 6 lines
monitor.c Monitor *default_mon; · line 232
Quoted 4 lines
monitor.c +0
-4-static inline int qmp_cmd_mode (const Monitor *mon) -{ -return mon->qmp.command_mode; -}...qmp_cmd_mode() is now inlined into its callers.
Can do.
Quoted 4 lines
Nice, fixes up a mis-use of 'int' that I complained about when writing commit
2d5a8346Reviewed-by Eric Blake <[email protected]>
Thanks!
PATCH 19/20 monitor: Rename monitor_ctrl_mode() to monitor_is_qmp()
In reply to Markus Armbruster... and change return type to bool.
--- monitor.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)
monitor.c +7 -5
@@ -231,8 +231,10 @@ Monitor *default_mon;static void monitor_command_cb (void *opaque,const char *cmdline,void *readline_opaque); -/* Return true if in control mode, false otherwise */ -static inline int monitor_ctrl_mode (const Monitor *mon) +/** +* Is @mon a QMP monitor? +*/ +static inline bool monitor_is_qmp (const Monitor *mon) {return (mon->flags &MONITOR_USE_CONTROL ); }
@@ -240,7 +242,7 @@ static inline int monitor_ctrl_mode(const Monitor *mon)/* Return non-zero iff we have a current monitor, and it is in QMP mode. */ int monitor_cur_is_qmp (void ) { -return cur_mon &&monitor_ctrl_mode (cur_mon); +return cur_mon &&monitor_is_qmp (cur_mon); }void monitor_read_command (Monitor *mon,int show_prompt)
@@ -350,7 +352,7 @@ void monitor_vprintf(Monitor *mon, const char *fmt, va_list ap)if (!mon)return ; -if (monitor_ctrl_mode (mon)) { +if (monitor_is_qmp (mon)) {return ; }
@@ -446,7 +448,7 @@ static void monitor_qapi_event_emit(QAPIEvent event, QObject *data)trace_monitor_protocol_event_emit (event, data); QLIST_FOREACH(mon, &mon_list, entry) { -if (monitor_ctrl_mode (mon) && mon->qmp.in_command_mode) { +if (monitor_is_qmp (mon) && mon->qmp.in_command_mode) {monitor_json_emitter (mon, data); } }
Signature · 1 line
1.9.3
Re: [Qemu-devel] [PATCH 19/20] monitor: Rename monitor_ctrl_mode() to monitor_is_qmp()
Quoted 7 lines · Markus Armbruster
... and change return type to bool.
Signed-off-by Markus Armbruster <[email protected]>--- monitor.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)
See also my review on 16 about a usage in monitor_init().
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
PATCH 20/20 monitor: Change return type of monitor_cur_is_qmp() to bool
In reply to Markus Armbruster--- include/monitor/monitor.h | 2 +- monitor.c | 6 ++++-- stubs/mon-is-qmp.c | 2 +- 3 files changed, 6 insertions(+), 4 deletions(-)
include/monitor/monitor.h +1 -1
@@ -16,7 +16,7 @@ extern Monitor *default_mon;#define MONITOR_USE_CONTROL 0x04#define MONITOR_USE_PRETTY 0x08 -int monitor_cur_is_qmp (void ); +bool monitor_cur_is_qmp (void );void monitor_init (CharDriverState *chr,int flags);
monitor.c +4 -2
@@ -239,8 +239,10 @@ static inline bool monitor_is_qmp(const Monitor *mon)return (mon->flags &MONITOR_USE_CONTROL ); } -/* Return non-zero iff we have a current monitor, and it is in QMP mode. */ -int monitor_cur_is_qmp (void ) +/** +* Is the current monitor, if any, a QMP monitor? +*/ +bool monitor_cur_is_qmp (void ) {return cur_mon &&monitor_is_qmp (cur_mon); }
stubs/mon-is-qmp.c +1 -1
@@ -1,7 +1,7 @@#include "qemu-common.h" #include "monitor/monitor.h" -int monitor_cur_is_qmp (void ) +bool monitor_cur_is_qmp (void ) {return 0 ; }
Signature · 1 line
1.9.3
Re: [Qemu-devel] [PATCH 20/20] monitor: Change return type of monitor_cur_is_qmp() to bool
Quoted 10 lines · Markus Armbruster
Signed-off-by Markus Armbruster <[email protected]>--- include/monitor/monitor.h | 2 +- monitor.c | 6 ++++-- stubs/mon-is-qmp.c | 2 +- 3 files changed, 6 insertions(+), 4 deletions(-)+++
b/stubs/mon-is-qmp.c
stubs/mon-is-qmp.c line 6
Quoted 7 lines
stubs/mon-is-qmp.c +1
-1@@ -1,7 +1,7 @@#include "qemu-common.h" #include "monitor/monitor.h" -int monitor_cur_is_qmp (void ) +bool monitor_cur_is_qmp (void ) {return 0 ;
s/0/false/
with that change,
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Re: [Qemu-devel] [PATCH 20/20] monitor: Change return type of monitor_cur_is_qmp() to bool
Quoted 14 lines · Eric Blake
Quoted 13 lines · Markus Armbruster
Signed-off-by Markus Armbruster <[email protected]>--- include/monitor/monitor.h | 2 +- monitor.c | 6 ++++-- stubs/mon-is-qmp.c | 2 +- 3 files changed, 6 insertions(+), 4 deletions(-)+++
b/stubs/mon-is-qmp.cstubs/mon-is-qmp.c +0
-0@@ -1,7 +1,7 @@#include "qemu-common.h" #include "monitor/monitor.h"
Quoted 6 lines
stubs/mon-is-qmp.c line 6
Quoted 4 lines
stubs/mon-is-qmp.c +1
-1-int monitor_cur_is_qmp (void ) +bool monitor_cur_is_qmp (void ) {return 0 ;s/0/false/
Yes.
Quoted 2 lines
with that change,
Reviewed-by Eric Blake <[email protected]>
Thanks!