qmp,nmi: convert do_inject_nmi() to QObject
PATCH v6 3/4 qmp,nmi: convert do_inject_nmi() to QObject
Make we can inject NMI via qemu-monitor-protocol.
We use "inject-nmi" for the qmp command name, the meaning is clearer.
---
hmp-commands.hx +2 -1
@@ -725,7 +725,8 @@ ETEXI .params = "[cpu]", .help = "Inject an NMI on all CPUs if no argument is given, " "otherwise inject it on the specified CPU", - .mhandler.cmd = do_inject_nmi, + .user_print = monitor_user_noop, + .mhandler.cmd_new = do_inject_nmi, }, #endif STEXI
monitor.c +5 -3
@@ -2542,7 +2542,7 @@ static void do_wav_capture(Monitor *mon, const QDict *qdict)#endif #if defined(TARGET_I386 ) -static void do_inject_nmi (Monitor *mon,const QDict *qdict) +static int do_inject_nmi (Monitor *mon,const QDict *qdict,QObject **ret_data) {CPUState *env;int cpu_index;
@@ -2550,7 +2550,7 @@ static void do_inject_nmi(Monitor *mon, const QDict *qdict)if (!qdict_haskey (qdict,"cpu-index" )) {for (env = first_cpu; env !=NULL ; env = env->next_cpu)cpu_interrupt (env,CPU_INTERRUPT_NMI ); -return ; +return 0 ; } cpu_index =qdict_get_int (qdict,"cpu-index" );
@@ -2560,8 +2560,10 @@ static void do_inject_nmi(Monitor *mon, const QDict *qdict)kvm_inject_interrupt (env,CPU_INTERRUPT_NMI ); elsecpu_interrupt (env ,CPU_INTERRUPT_NMI ); - break; + return 0; } + +return -1 ; }#endif
qmp-commands.hx +28 -0
@@ -429,6 +429,34 @@ Example: EQMP +#if defined(TARGET_I386) + { + .name = "inject-nmi", + .args_type = "cpu-index:i?", + .params = "[cpu]", + .help = "Inject an NMI on all CPUs if no argument is given, " + "otherwise inject it on the specified CPU", + .user_print = monitor_user_noop, + .mhandler.cmd_new = do_inject_nmi, + }, +#endif +SQMP +inject-nmi +---------- + +Inject an NMI on all CPUs or the given CPU (x86 only). + +Arguments: + +- "cpu-index": the index of the CPU to be injected NMI (json-int, optional) + +Example: + +-> { "execute": "inject-nmi", "arguments": { "cpu-index": 0 } } +<- { "return": {} } + +EQMP + { .name = "migrate", .args_type = "detach:-d,blk:-b,inc:-i,uri:s",
Re: [Qemu-devel] [PATCH V6 3/4] qmp, nmi: convert do_inject_nmi() to QObject
Quoted 77 lines · Lai Jiangshan
Make we can inject NMI via qemu-monitor-protocol.
We use "inject-nmi" for the qmp command name, the meaning is clearer.Signed-off-by Lai Jiangshan<[email protected]>---
hmp-commands.hx +2
-1@@ -725,7 +725,8 @@ ETEXI .params = "[cpu]", .help = "Inject an NMI on all CPUs if no argument is given, " "otherwise inject it on the specified CPU", - .mhandler.cmd = do_inject_nmi, + .user_print = monitor_user_noop, + .mhandler.cmd_new = do_inject_nmi, }, #endif STEXImonitor.c +5
-3@@ -2542,7 +2542,7 @@ static void do_wav_capture(Monitor *mon, const QDict *qdict)#endif #if defined(TARGET_I386 ) -static void do_inject_nmi (Monitor *mon,const QDict *qdict) +static int do_inject_nmi (Monitor *mon,const QDict *qdict,QObject **ret_data) {CPUState *env;int cpu_index;@@ -2550,7 +2550,7 @@ static void do_inject_nmi(Monitor *mon, const QDict *qdict)if (!qdict_haskey (qdict,"cpu-index" )) {for (env = first_cpu; env !=NULL ; env = env->next_cpu)cpu_interrupt (env,CPU_INTERRUPT_NMI ); -return ; +return 0 ; } cpu_index =qdict_get_int (qdict,"cpu-index" );@@ -2560,8 +2560,10 @@ static void do_inject_nmi(Monitor *mon, const QDict *qdict)kvm_inject_interrupt (env,CPU_INTERRUPT_NMI ); elsecpu_interrupt (env ,CPU_INTERRUPT_NMI ); - break; + return 0; } + +return -1 ; }#endif qmp-commands.hx +16
-0@@ -429,6 +429,34 @@ Example: EQMP +#if defined(TARGET_I386) + { + .name = "inject-nmi", + .args_type = "cpu-index:i?", + .params = "[cpu]", + .help = "Inject an NMI on all CPUs if no argument is given, " + "otherwise inject it on the specified CPU", + .user_print = monitor_user_noop, + .mhandler.cmd_new = do_inject_nmi, + }, +#endif +SQMP +inject-nmi +---------- + +Inject an NMI on all CPUs or the given CPU (x86 only).
qmp-commands.hx Example: · line 457
Quoted 10 lines
qmp-commands.hx +9
-0+ +Arguments: + +- "cpu-index": the index of the CPU to be injected NMI (json-int, optional) + +Example: + +-> { "execute": "inject-nmi", "arguments": { "cpu-index": 0 } } +<- { "return": {} }
Please describe all expected errors. Don't hide this command for !defined(TARGET_I386), instead have it throw an error in the implementation.
Don't have commands that multiple behavior based on the presence or absence of arguments. Make it take a list of cpus if you want the ability to inject the NMI to more than one CPU.
Regards,
Anthony Liguori
Quoted 8 lines
qmp-commands.hx +3
-0+ +EQMP + { .name = "migrate",.args_type = "detach:-d,blk:-b,inc:-i,uri:s",
Re: [PATCH V6 3/4] qmp, nmi: convert do_inject_nmi() to QObject
Quoted 92 lines · On Wed, 23 Feb 2011 13:25:38 -0600 Anthony Liguori <[email protected]>…
Quoted 77 lines · Lai Jiangshan
Make we can inject NMI via qemu-monitor-protocol.
We use "inject-nmi" for the qmp command name, the meaning is clearer.Signed-off-by Lai Jiangshan<[email protected]>---
hmp-commands.hx +2
-1@@ -725,7 +725,8 @@ ETEXI .params = "[cpu]", .help = "Inject an NMI on all CPUs if no argument is given, " "otherwise inject it on the specified CPU", - .mhandler.cmd = do_inject_nmi, + .user_print = monitor_user_noop, + .mhandler.cmd_new = do_inject_nmi, }, #endif STEXImonitor.c +5
-3@@ -2542,7 +2542,7 @@ static void do_wav_capture(Monitor *mon, const QDict *qdict)#endif #if defined(TARGET_I386 ) -static void do_inject_nmi (Monitor *mon,const QDict *qdict) +static int do_inject_nmi (Monitor *mon,const QDict *qdict,QObject **ret_data) {CPUState *env;int cpu_index;@@ -2550,7 +2550,7 @@ static void do_inject_nmi(Monitor *mon, const QDict *qdict)if (!qdict_haskey (qdict,"cpu-index" )) {for (env = first_cpu; env !=NULL ; env = env->next_cpu)cpu_interrupt (env,CPU_INTERRUPT_NMI ); -return ; +return 0 ; } cpu_index =qdict_get_int (qdict,"cpu-index" );@@ -2560,8 +2560,10 @@ static void do_inject_nmi(Monitor *mon, const QDict *qdict)kvm_inject_interrupt (env,CPU_INTERRUPT_NMI ); elsecpu_interrupt (env ,CPU_INTERRUPT_NMI ); - break; + return 0; } + +return -1 ; }#endif qmp-commands.hx +16
-0@@ -429,6 +429,34 @@ Example: EQMP +#if defined(TARGET_I386) + { + .name = "inject-nmi", + .args_type = "cpu-index:i?", + .params = "[cpu]", + .help = "Inject an NMI on all CPUs if no argument is given, " + "otherwise inject it on the specified CPU", + .user_print = monitor_user_noop, + .mhandler.cmd_new = do_inject_nmi, + }, +#endif +SQMP +inject-nmi +---------- + +Inject an NMI on all CPUs or the given CPU (x86 only).qmp-commands.hx Example: · line 457
Quoted 10 lines
qmp-commands.hx +9
-0+ +Arguments: + +- "cpu-index": the index of the CPU to be injected NMI (json-int, optional) + +Example: + +-> { "execute": "inject-nmi", "arguments": { "cpu-index": 0 } } +<- { "return": {} }Please describe all expected errors. Don't hide this command for !defined(TARGET_I386), instead have it throw an error in the implementation.
Quoted 3 lines
Don't have commands that multiple behavior based on the presence or absence of arguments. Make it take a list of cpus if you want the ability to inject the NMI to more than one CPU.
We had this exactly same discussion last year:
http://lists.gnu.org/archive/html/qemu-devel/2010-12/msg01285.html
Quoted 14 lines
Regards,
Anthony Liguori
Quoted 8 lines
qmp-commands.hx +3
-0+ +EQMP + { .name = "migrate",.args_type = "detach:-d,blk:-b,inc:-i,uri:s",
Re: [Qemu-devel] [PATCH V6 3/4] qmp, nmi: convert do_inject_nmi() to QObject
In reply to Anthony LiguoriQuoted 82 lines · Anthony Liguori
Quoted 81 lines · Lai Jiangshan
Make we can inject NMI via qemu-monitor-protocol.
We use "inject-nmi" for the qmp command name, the meaning is clearer.Signed-off-by Lai Jiangshan<[email protected]>---
hmp-commands.hx +2
-1@@ -725,7 +725,8 @@ ETEXI .params = "[cpu]", .help = "Inject an NMI on all CPUs if no argument is given, " "otherwise inject it on the specified CPU", - .mhandler.cmd = do_inject_nmi, + .user_print = monitor_user_noop, + .mhandler.cmd_new = do_inject_nmi, }, #endif STEXImonitor.c +5
-3@@ -2542,7 +2542,7 @@ static void do_wav_capture(Monitor *mon, const QDict *qdict)#endif #if defined(TARGET_I386 ) -static void do_inject_nmi (Monitor *mon,const QDict *qdict) +static int do_inject_nmi (Monitor *mon,const QDict *qdict,QObject **ret_data) {CPUState *env;int cpu_index;@@ -2550,7 +2550,7 @@ static void do_inject_nmi(Monitor *mon, const QDict *qdict)if (!qdict_haskey (qdict,"cpu-index" )) {for (env = first_cpu; env !=NULL ; env = env->next_cpu)cpu_interrupt (env,CPU_INTERRUPT_NMI ); -return ; +return 0 ; } cpu_index =qdict_get_int (qdict,"cpu-index" );@@ -2560,8 +2560,10 @@ static void do_inject_nmi(Monitor *mon, const QDict *qdict)kvm_inject_interrupt (env,CPU_INTERRUPT_NMI ); elsecpu_interrupt (env ,CPU_INTERRUPT_NMI ); - break; + return 0; } + +return -1 ; }#endif qmp-commands.hx +20
-0@@ -429,6 +429,34 @@ Example: EQMP +#if defined(TARGET_I386) + { + .name = "inject-nmi", + .args_type = "cpu-index:i?", + .params = "[cpu]", + .help = "Inject an NMI on all CPUs if no argument is given, " + "otherwise inject it on the specified CPU", + .user_print = monitor_user_noop, + .mhandler.cmd_new = do_inject_nmi, + }, +#endif +SQMP +inject-nmi +---------- + +Inject an NMI on all CPUs or the given CPU (x86 only). + +Arguments: + +- "cpu-index": the index of the CPU to be injected NMI (json-int, optional)
Quoted 8 lines
qmp-commands.hx Example:
Quoted 6 lines
qmp-commands.hx +5
-0+ +Example: + +-> { "execute": "inject-nmi", "arguments": { "cpu-index": 0 } } +<- { "return": {} }Please describe all expected errors.
Quoting qmp-commands.hx:
3. Errors, in special, are not documented. Applications should NOT check for specific errors classes or data (it's strongly recommended to only check for the "error" key)
Indeed, not a single error is documented there. This is intentional.
Once we have an error design in place that has a reasonable hope to stand the test of time, and have errors documented for at least some of the commands here, we can start to require proper error documentation for new commands. But not now.
Quoted 3 lines
Don't hide this command for
!defined(TARGET_I386), instead have it throw an error in the implementation.
Works for me.
Quoted 3 lines
Don't have commands that multiple behavior based on the presence or absence of arguments. Make it take a list of cpus if you want the ability to inject the NMI to more than one CPU.
Having optional arguments is fine. It's good taste to give them "default semantics", i.e. "no argument" is shorthand for one specific argument value.
Luiz already pointed to the thread where we discussed this command before. Executive summary:
* Real hardware's NMI button injects all CPUs. This is the primary use case.
* Lai said injecting a single CPU can be useful for debugging. Was deemed acceptable as secondary use case.
Lai also pointed out that the human monitor's nmi command injects a single CPU. That was dismissed as irrelevant for QMP.
* No other use cases have been presented.
Therefore, the "list of CPUs" idea was shot down as overly general.
Re: [Qemu-devel] [PATCH V6 3/4] qmp, nmi: convert do_inject_nmi() to QObject
Quoted 95 lines · Markus Armbruster
Quoted 83 lines · Anthony Liguori
Quoted 81 lines · Lai Jiangshan
Make we can inject NMI via qemu-monitor-protocol.
We use "inject-nmi" for the qmp command name, the meaning is clearer.Signed-off-by Lai Jiangshan<[email protected]>---
hmp-commands.hx +2
-1@@ -725,7 +725,8 @@ ETEXI .params = "[cpu]", .help = "Inject an NMI on all CPUs if no argument is given, " "otherwise inject it on the specified CPU", - .mhandler.cmd = do_inject_nmi, + .user_print = monitor_user_noop, + .mhandler.cmd_new = do_inject_nmi, }, #endif STEXImonitor.c +5
-3@@ -2542,7 +2542,7 @@ static void do_wav_capture(Monitor *mon, const QDict *qdict)#endif #if defined(TARGET_I386 ) -static void do_inject_nmi (Monitor *mon,const QDict *qdict) +static int do_inject_nmi (Monitor *mon,const QDict *qdict,QObject **ret_data) {CPUState *env;int cpu_index;@@ -2550,7 +2550,7 @@ static void do_inject_nmi(Monitor *mon, const QDict *qdict)if (!qdict_haskey (qdict,"cpu-index" )) {for (env = first_cpu; env !=NULL ; env = env->next_cpu)cpu_interrupt (env,CPU_INTERRUPT_NMI ); -return ; +return 0 ; } cpu_index =qdict_get_int (qdict,"cpu-index" );@@ -2560,8 +2560,10 @@ static void do_inject_nmi(Monitor *mon, const QDict *qdict)kvm_inject_interrupt (env,CPU_INTERRUPT_NMI ); elsecpu_interrupt (env ,CPU_INTERRUPT_NMI ); - break; + return 0; } + +return -1 ; }#endif qmp-commands.hx +20
-0@@ -429,6 +429,34 @@ Example: EQMP +#if defined(TARGET_I386) + { + .name = "inject-nmi", + .args_type = "cpu-index:i?", + .params = "[cpu]", + .help = "Inject an NMI on all CPUs if no argument is given, " + "otherwise inject it on the specified CPU", + .user_print = monitor_user_noop, + .mhandler.cmd_new = do_inject_nmi, + }, +#endif +SQMP +inject-nmi +---------- + +Inject an NMI on all CPUs or the given CPU (x86 only). + +Arguments: + +- "cpu-index": the index of the CPU to be injected NMI (json-int, optional)Quoted 9 lines
qmp-commands.hx Example:
Quoted 7 lines
qmp-commands.hx +5
-0+ +Example: + +-> { "execute": "inject-nmi", "arguments": { "cpu-index": 0 } } +<- { "return": {} }Please describe all expected errors.
Quoted 8 lines
Quoting qmp-commands.hx:
3. Errors, in special, are not documented. Applications should NOT check for specific errors classes or data (it's strongly recommended to only check for the "error" key)
Indeed, not a single error is documented there. This is intentional.
Yeah, but we're not 0.14 anymore and for 0.15, we need to document errors. If you are suggesting I send a patch to remove that section, I'm more than happy to.
Quoted 5 lines
Once we have an error design in place that has a reasonable hope to stand the test of time, and have errors documented for at least some of the commands here, we can start to require proper error documentation for new commands. But not now.
I'm quite happy with the error design we have today. The only problem is that we don't propagate errors in a sane way but I've got that all but fixed in my qapi tree.
Quoted 24 lines
Quoted 4 lines
Don't hide this command for
!defined(TARGET_I386), instead have it throw an error in the implementation.Works for me.
Quoted 4 lines
Don't have commands that multiple behavior based on the presence or absence of arguments. Make it take a list of cpus if you want the ability to inject the NMI to more than one CPU.
Having optional arguments is fine. It's good taste to give them "default semantics", i.e. "no argument" is shorthand for one specific argument value.
Luiz already pointed to the thread where we discussed this command before. Executive summary:
* Real hardware's NMI button injects all CPUs. This is the primary use case.
* Lai said injecting a single CPU can be useful for debugging. Was deemed acceptable as secondary use case.
Quoted 7 lines
Lai also pointed out that the human monitor's nmi command injects a single CPU. That was dismissed as irrelevant for QMP.
* No other use cases have been presented.
Therefore, the "list of CPUs" idea was shot down as overly general.
That's fine, then we should do two commands. Think of it from the perspective of the client. This appears as:
in C:
qmp_inject_nmi(sess, false, 0, &err);
in Python:
sess.inject_nmi()
The first example doesn't tell you at all what's happening. The second API does look really nice until you see the following later:
sess.inject_nmi(0)
What's the difference between these two functions? You might say this is bad form and that an explicit named argument should be given but I wouldn't count on it.
Having two commands, nmi_inject and nmi_inject_on_cpu, would result in a much more readable API in both C and Python:
in C:
qmp_nmi_inject(sess, &err);qmp_nmi_inject_on_cpu(sess, 3, &err);
in Python:
sess.nmi_inject()sess.nmi_inject_on_cpu(3)
Regards,
Anthony Liguori
Re: [Qemu-devel] [PATCH V6 3/4] qmp, nmi: convert do_inject_nmi() to QObject
Quoted 102 lines · Anthony Liguori
Quoted 101 lines · Markus Armbruster
Quoted 83 lines · Anthony Liguori
Quoted 81 lines · Lai Jiangshan
Make we can inject NMI via qemu-monitor-protocol.
We use "inject-nmi" for the qmp command name, the meaning is clearer.Signed-off-by Lai Jiangshan<[email protected]>---
hmp-commands.hx +2
-1@@ -725,7 +725,8 @@ ETEXI .params = "[cpu]", .help = "Inject an NMI on all CPUs if no argument is given, " "otherwise inject it on the specified CPU", - .mhandler.cmd = do_inject_nmi, + .user_print = monitor_user_noop, + .mhandler.cmd_new = do_inject_nmi, }, #endif STEXImonitor.c +5
-3@@ -2542,7 +2542,7 @@ static void do_wav_capture(Monitor *mon, const QDict *qdict)#endif #if defined(TARGET_I386 ) -static void do_inject_nmi (Monitor *mon,const QDict *qdict) +static int do_inject_nmi (Monitor *mon,const QDict *qdict,QObject **ret_data) {CPUState *env;int cpu_index;@@ -2550,7 +2550,7 @@ static void do_inject_nmi(Monitor *mon, const QDict *qdict)if (!qdict_haskey (qdict,"cpu-index" )) {for (env = first_cpu; env !=NULL ; env = env->next_cpu)cpu_interrupt (env,CPU_INTERRUPT_NMI ); -return ; +return 0 ; } cpu_index =qdict_get_int (qdict,"cpu-index" );@@ -2560,8 +2560,10 @@ static void do_inject_nmi(Monitor *mon, const QDict *qdict)kvm_inject_interrupt (env,CPU_INTERRUPT_NMI ); elsecpu_interrupt (env ,CPU_INTERRUPT_NMI ); - break; + return 0; } + +return -1 ; }#endif qmp-commands.hx +20
-0@@ -429,6 +429,34 @@ Example: EQMP +#if defined(TARGET_I386) + { + .name = "inject-nmi", + .args_type = "cpu-index:i?", + .params = "[cpu]", + .help = "Inject an NMI on all CPUs if no argument is given, " + "otherwise inject it on the specified CPU", + .user_print = monitor_user_noop, + .mhandler.cmd_new = do_inject_nmi, + }, +#endif +SQMP +inject-nmi +---------- + +Inject an NMI on all CPUs or the given CPU (x86 only). + +Arguments: + +- "cpu-index": the index of the CPU to be injected NMI (json-int, optional)Quoted 9 lines
qmp-commands.hx Example:
Quoted 7 lines
qmp-commands.hx +5
-0+ +Example: + +-> { "execute": "inject-nmi", "arguments": { "cpu-index": 0 } } +<- { "return": {} }Please describe all expected errors.
Quoting qmp-commands.hx:
3. Errors, in special, are not documented. Applications should NOT check for specific errors classes or data (it's strongly recommended to only check for the "error" key)
Quoted 6 lines
Quoted 2 lines
Indeed, not a single error is documented there. This is intentional.
Yeah, but we're not 0.14 anymore and for 0.15, we need to document errors. If you are suggesting I send a patch to remove that section, I'm more than happy to.
Two separate issues here: 1. Are we ready to commit to the current design of errors, and 2. Is it fair to reject Lai's patch now because he doesn't document his errors.
I'm not commenting on 1. here.
Regarding 2.: rejecting a patch because it doesn't document an aspect that current master intentionally leaves undocumented is not how you treat contributors. At least not if you want any other than certified masochists who enjoy pain, and professionals who get adequately compensated for it.
Lead by example, not by fiat.
Quoted 9 lines
Quoted 5 lines
Once we have an error design in place that has a reasonable hope to stand the test of time, and have errors documented for at least some of the commands here, we can start to require proper error documentation for new commands. But not now.
I'm quite happy with the error design we have today. The only problem is that we don't propagate errors in a sane way but I've got that all but fixed in my qapi tree.
I don't think error propagation is the only problem we have with QError.
QError makes it way too hard to emit error messages fit for human consumption. The consequence is that we get errors unfit for humans.
Quoted 57 lines
Quoted 24 lines
Quoted 4 lines
Don't hide this command for
!defined(TARGET_I386), instead have it throw an error in the implementation.Works for me.
Quoted 4 lines
Don't have commands that multiple behavior based on the presence or absence of arguments. Make it take a list of cpus if you want the ability to inject the NMI to more than one CPU.
Having optional arguments is fine. It's good taste to give them "default semantics", i.e. "no argument" is shorthand for one specific argument value.
Luiz already pointed to the thread where we discussed this command before. Executive summary:
* Real hardware's NMI button injects all CPUs. This is the primary use case.
* Lai said injecting a single CPU can be useful for debugging. Was deemed acceptable as secondary use case.
Quoted 7 lines
Lai also pointed out that the human monitor's nmi command injects a single CPU. That was dismissed as irrelevant for QMP.
* No other use cases have been presented.
Therefore, the "list of CPUs" idea was shot down as overly general.
That's fine, then we should do two commands. Think of it from the perspective of the client. This appears as:
in C:
qmp_inject_nmi(sess, false, 0, &err);in Python:
sess.inject_nmi()The first example doesn't tell you at all what's happening. The second API does look really nice until you see the following later:
sess.inject_nmi(0)What's the difference between these two functions? You might say this is bad form and that an explicit named argument should be given but I wouldn't count on it.
Having two commands,
nmi_injectandnmi_inject_on_cpu, would result in a much more readable API in both C and Python:in C:
Quoted 7 lines
qmp_nmi_inject(sess, &err);qmp_nmi_inject_on_cpu(sess, 3, &err);in Python:
sess.nmi_inject()sess.nmi_inject_on_cpu(3)
I'm fine with Lai's patch. I'd be just as fine with Lai's patch less the "inject on one CPU" feature. And I'd be fine with your interface as well.
Lai has been trying to get this simple feature in for almost three months. And we're still here, debating which way he should change his arguments, and whether we should require him to document his errors before anyone else. For crying out loud, what's wrong with us?
Re: [Qemu-devel] [PATCH V6 3/4] qmp, nmi: convert do_inject_nmi() to QObject
Quoted 123 lines · Markus Armbruster
Quoted 107 lines · Anthony Liguori
Quoted 105 lines · Markus Armbruster
Quoted 85 lines · Anthony Liguori
Quoted 82 lines · Lai Jiangshan
Make we can inject NMI via qemu-monitor-protocol.
We use "inject-nmi" for the qmp command name, the meaning is clearer.Signed-off-by Lai Jiangshan<[email protected]>---
hmp-commands.hx +2
-1@@ -725,7 +725,8 @@ ETEXI .params = "[cpu]", .help = "Inject an NMI on all CPUs if no argument is given, " "otherwise inject it on the specified CPU", - .mhandler.cmd = do_inject_nmi, + .user_print = monitor_user_noop, + .mhandler.cmd_new = do_inject_nmi, }, #endif STEXImonitor.c +5
-3@@ -2542,7 +2542,7 @@ static void do_wav_capture(Monitor *mon, const QDict *qdict)#endif #if defined(TARGET_I386 ) -static void do_inject_nmi (Monitor *mon,const QDict *qdict) +static int do_inject_nmi (Monitor *mon,const QDict *qdict,QObject **ret_data) {CPUState *env;int cpu_index;@@ -2550,7 +2550,7 @@ static void do_inject_nmi(Monitor *mon, const QDict *qdict)if (!qdict_haskey (qdict,"cpu-index" )) {for (env = first_cpu; env !=NULL ; env = env->next_cpu)cpu_interrupt (env,CPU_INTERRUPT_NMI ); -return ; +return 0 ; } cpu_index =qdict_get_int (qdict,"cpu-index" );@@ -2560,8 +2560,10 @@ static void do_inject_nmi(Monitor *mon, const QDict *qdict)kvm_inject_interrupt (env,CPU_INTERRUPT_NMI ); elsecpu_interrupt (env ,CPU_INTERRUPT_NMI ); - break; + return 0; } + +return -1 ; }#endif qmp-commands.hx +21
-0@@ -429,6 +429,34 @@ Example: EQMP +#if defined(TARGET_I386) + { + .name = "inject-nmi", + .args_type = "cpu-index:i?", + .params = "[cpu]", + .help = "Inject an NMI on all CPUs if no argument is given, " + "otherwise inject it on the specified CPU", + .user_print = monitor_user_noop, + .mhandler.cmd_new = do_inject_nmi, + }, +#endif +SQMP +inject-nmi +---------- + +Inject an NMI on all CPUs or the given CPU (x86 only). + +Arguments: + +- "cpu-index": the index of the CPU to be injected NMI (json-int, optional) +Quoted 10 lines
qmp-commands.hx Example:
Quoted 7 lines
qmp-commands.hx +4
-0+Example: + +-> { "execute": "inject-nmi", "arguments": { "cpu-index": 0 } } +<- { "return": {} }Please describe all expected errors.
Quoting qmp-commands.hx:
3. Errors, in special, are not documented. Applications should NOT check for specific errors classes or data (it's strongly recommended to only check for the "error" key)
Quoted 7 lines
Quoted 3 lines
Indeed, not a single error is documented there. This is intentional.
Yeah, but we're not 0.14 anymore and for 0.15, we need to document errors. If you are suggesting I send a patch to remove that section, I'm more than happy to.
Two separate issues here: 1. Are we ready to commit to the current design of errors, and 2. Is it fair to reject Lai's patch now because he doesn't document his errors.
I'm not commenting on 1. here.
Quoted 8 lines
Regarding 2.: rejecting a patch because it doesn't document an aspect that current master intentionally leaves undocumented is not how you treat contributors. At least not if you want any other than certified masochists who enjoy pain, and professionals who get adequately compensated for it.
Lead by example, not by fiat.
http://repo.or.cz/w/qemu/aliguori.git/blob/refs/heads/glib:/qmp-schema.json
I am in the process of documenting the errors of every command. It's a royal pain but I'm going to document everything we have right now. It's actually the last bit of work I need to finish before sending QAPI out.
So for new commands being added, it would be hugely helpful for the authors to document the errors such that I don't have to reverse engineer all of the possible error conditions.
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Once we have an error design in place that has a reasonable hope to stand the test of time, and have errors documented for at least some of the commands here, we can start to require proper error documentation for new commands. But not now.
Quoted 9 lines
Quoted 4 lines
I'm quite happy with the error design we have today. The only problem is that we don't propagate errors in a sane way but I've got that all but fixed in my qapi tree.
I don't think error propagation is the only problem we have with QError.
QError makes it way too hard to emit error messages fit for human consumption. The consequence is that we get errors unfit for humans.
Take a look at http://repo.or.cz/w/qemu/aliguori.git/blob/refs/heads/glib:/hmp.c#l64
Automatically generating an error string based on an error and no additional context is problematic. Given the context of the caller and the right fields in the error, very high quality error messages can be constructed.
Regards,
Anthony Liguori
Quoted 77 lines
Quoted 60 lines
Quoted 27 lines
Quoted 5 lines
Don't hide this command for
!defined(TARGET_I386), instead have it throw an error in the implementation.Works for me.
Quoted 5 lines
Don't have commands that multiple behavior based on the presence or absence of arguments. Make it take a list of cpus if you want the ability to inject the NMI to more than one CPU.
Having optional arguments is fine. It's good taste to give them "default semantics", i.e. "no argument" is shorthand for one specific argument value.
Luiz already pointed to the thread where we discussed this command before. Executive summary:
* Real hardware's NMI button injects all CPUs. This is the primary use case.
* Lai said injecting a single CPU can be useful for debugging. Was deemed acceptable as secondary use case.
Quoted 8 lines
Lai also pointed out that the human monitor's nmi command injects a single CPU. That was dismissed as irrelevant for QMP.
* No other use cases have been presented.
Therefore, the "list of CPUs" idea was shot down as overly general.
That's fine, then we should do two commands. Think of it from the perspective of the client. This appears as:
in C:
qmp_inject_nmi(sess, false, 0,&err);in Python:
sess.inject_nmi()The first example doesn't tell you at all what's happening. The second API does look really nice until you see the following later:
sess.inject_nmi(0)What's the difference between these two functions? You might say this is bad form and that an explicit named argument should be given but I wouldn't count on it.
Having two commands,
nmi_injectandnmi_inject_on_cpu, would result in a much more readable API in both C and Python:in C:
Quoted 8 lines
qmp_nmi_inject(sess,&err);qmp_nmi_inject_on_cpu(sess, 3,&err);in Python:
sess.nmi_inject()sess.nmi_inject_on_cpu(3)I'm fine with Lai's patch. I'd be just as fine with Lai's patch less the "inject on one CPU" feature. And I'd be fine with your interface as well.
Lai has been trying to get this simple feature in for almost three months. And we're still here, debating which way he should change his arguments, and whether we should require him to document his errors before anyone else. For crying out loud, what's wrong with us?
Re: [Qemu-devel] [PATCH V6 3/4] qmp, nmi: convert do_inject_nmi() to QObject
Quoted 7 lines · Anthony Liguori
Quoted 6 lines · Markus Armbruster
Quoted 3 lines · Anthony Liguori
Quoted 1 line · Markus Armbruster
Anthony Liguori<[email protected]> writes:
[...]
Quoted 33 lines
Quoted 22 lines
Quoted 9 lines
Quoted 9 lines
Quoted 3 lines
Please describe all expected errors.
Quoting qmp-commands.hx:
3. Errors, in special, are not documented. Applications should NOT check for specific errors classes or data (it's strongly recommended to only check for the "error" key)
Quoted 7 lines
Quoted 3 lines
Indeed, not a single error is documented there. This is intentional.
Yeah, but we're not 0.14 anymore and for 0.15, we need to document errors. If you are suggesting I send a patch to remove that section, I'm more than happy to.
Two separate issues here: 1. Are we ready to commit to the current design of errors, and 2. Is it fair to reject Lai's patch now because he doesn't document his errors.
I'm not commenting on 1. here.
Quoted 8 lines
Regarding 2.: rejecting a patch because it doesn't document an aspect that current master intentionally leaves undocumented is not how you treat contributors. At least not if you want any other than certified masochists who enjoy pain, and professionals who get adequately compensated for it.
Lead by example, not by fiat.
http://repo.or.cz/w/qemu/aliguori.git/blob/refs/heads/glib:/qmp-schema.json
Quoted 8 lines
I am in the process of documenting the errors of every command. It's a royal pain but I'm going to document everything we have right now. It's actually the last bit of work I need to finish before sending QAPI out.
So for new commands being added, it would be hugely helpful for the authors to document the errors such that I don't have to reverse engineer all of the possible error conditions.
The moment this lands in master, you can begin to demand error descriptions from contributors. Until then, I'll NAK error descriptions in qmp-commands.txt. We left them undocumented there for good reasons:
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Once we have an error design in place that has a reasonable hope to stand the test of time, and have errors documented for at least some of the commands here, we can start to require proper error documentation for new commands. But not now.
I won't NAK non-normative error descriptions, say in commit messages, or in comments. And I won't object to you asking for them. But I feel you really shouldn't make it a condition for committing patches. Especially not for simple patches that have been on list for months.
Re: [Qemu-devel] [PATCH V6 3/4] qmp, nmi: convert do_inject_nmi() to QObject
Quoted 57 lines · Markus Armbruster
Quoted 11 lines · Anthony Liguori
Quoted 9 lines · Markus Armbruster
Quoted 5 lines · Anthony Liguori
Quoted 2 lines · Markus Armbruster
Anthony Liguori<[email protected]> writes:
[...]
Quoted 41 lines
Quoted 25 lines
Quoted 10 lines
Quoted 10 lines
Quoted 4 lines
Please describe all expected errors.
Quoting qmp-commands.hx:
3. Errors, in special, are not documented. Applications should NOT check for specific errors classes or data (it's strongly recommended to only check for the "error" key)
Quoted 9 lines
Quoted 4 lines
Indeed, not a single error is documented there. This is intentional.
Yeah, but we're not 0.14 anymore and for 0.15, we need to document errors. If you are suggesting I send a patch to remove that section, I'm more than happy to.
Two separate issues here: 1. Are we ready to commit to the current design of errors, and 2. Is it fair to reject Lai's patch now because he doesn't document his errors.
I'm not commenting on 1. here.
Quoted 9 lines
Regarding 2.: rejecting a patch because it doesn't document an aspect that current master intentionally leaves undocumented is not how you treat contributors. At least not if you want any other than certified masochists who enjoy pain, and professionals who get adequately compensated for it.
Lead by example, not by fiat.
http://repo.or.cz/w/qemu/aliguori.git/blob/refs/heads/glib:/qmp-schema.json
I am in the process of documenting the errors of every command. It's a royal pain but I'm going to document everything we have right now. It's actually the last bit of work I need to finish before sending QAPI out.
Quoted 8 lines
Quoted 4 lines
So for new commands being added, it would be hugely helpful for the authors to document the errors such that I don't have to reverse engineer all of the possible error conditions.
The moment this lands in master, you can begin to demand error descriptions from contributors. Until then, I'll NAK error descriptions in qmp-commands.txt. We left them undocumented there for good reasons:
No, it was always a bad reason. Good documentation is necessary to build good commands. Errors are a huge part of the semantics of a command. We cannot properly assess a command unless it's behavior in error conditions is well defined.
Quoted 10 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Once we have an error design in place that has a reasonable hope to stand the test of time, and have errors documented for at least some of the commands here, we can start to require proper error documentation for new commands. But not now.
I won't NAK non-normative error descriptions, say in commit messages, or in comments. And I won't object to you asking for them. But I feel you really shouldn't make it a condition for committing patches. Especially not for simple patches that have been on list for months.
I'm strongly committed to making QMP a first class interface in QEMU for 0.15. I feel documentation is a crucial part to making that happen.
I'm not asking for test cases even though that's something that we'll need for 0.15 because there's not enough infrastructure in master yet to do that in a reasonable way. I realize I'm likely going to have to write that test case and I'm happy to do that.
But there's no reason that we shouldn't require thorough documentation for all new QMP commands moving forward including error semantics. This is a critical part of having a first class API and no additional infrastructure is needed in master to do this.
Regards,
Anthony Liguori
Quoted 5 lines
--
To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Qemu-devel] [PATCH V6 3/4] qmp, nmi: convert do_inject_nmi() to QObject
Quoted 67 lines · Anthony Liguori
Quoted 57 lines · Markus Armbruster
Quoted 11 lines · Anthony Liguori
Quoted 9 lines · Markus Armbruster
Quoted 5 lines · Anthony Liguori
Quoted 2 lines · Markus Armbruster
Anthony Liguori<[email protected]> writes:
[...]
Quoted 41 lines
Quoted 25 lines
Quoted 10 lines
Quoted 10 lines
Quoted 4 lines
Please describe all expected errors.
Quoting qmp-commands.hx:
3. Errors, in special, are not documented. Applications should NOT check for specific errors classes or data (it's strongly recommended to only check for the "error" key)
Quoted 9 lines
Quoted 4 lines
Indeed, not a single error is documented there. This is intentional.
Yeah, but we're not 0.14 anymore and for 0.15, we need to document errors. If you are suggesting I send a patch to remove that section, I'm more than happy to.
Two separate issues here: 1. Are we ready to commit to the current design of errors, and 2. Is it fair to reject Lai's patch now because he doesn't document his errors.
I'm not commenting on 1. here.
Quoted 9 lines
Regarding 2.: rejecting a patch because it doesn't document an aspect that current master intentionally leaves undocumented is not how you treat contributors. At least not if you want any other than certified masochists who enjoy pain, and professionals who get adequately compensated for it.
Lead by example, not by fiat.
http://repo.or.cz/w/qemu/aliguori.git/blob/refs/heads/glib:/qmp-schema.json
I am in the process of documenting the errors of every command. It's a royal pain but I'm going to document everything we have right now. It's actually the last bit of work I need to finish before sending QAPI out.
Quoted 8 lines
Quoted 4 lines
So for new commands being added, it would be hugely helpful for the authors to document the errors such that I don't have to reverse engineer all of the possible error conditions.
The moment this lands in master, you can begin to demand error descriptions from contributors. Until then, I'll NAK error descriptions in qmp-commands.txt. We left them undocumented there for good reasons:
Quoted 4 lines
No, it was always a bad reason. Good documentation is necessary to build good commands. Errors are a huge part of the semantics of a command. We cannot properly assess a command unless it's behavior in error conditions is well defined.
The decision to declare QMP stable was made at the KVM Forum after quite some deliberation. We explictly excluded errors. We didn't do that because errors are not worthy of specification (that would be silly). We did it because there was too much unhappiness about the current state of errors to close the debate on them at that time. Luiz mindfully documented that decision in qmp-commands.txt:
3. Errors, in special, are not documented. Applications should NOT check for specific errors classes or data (it's strongly recommended to only check for the "error" key)
Nobody denies that errors need to be specified, and this clause needs to go. But I must insist on proper review. No sneaking in of error specification through the commit backdoor, please.
Quoted 20 lines
Quoted 10 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Once we have an error design in place that has a reasonable hope to stand the test of time, and have errors documented for at least some of the commands here, we can start to require proper error documentation for new commands. But not now.
I won't NAK non-normative error descriptions, say in commit messages, or in comments. And I won't object to you asking for them. But I feel you really shouldn't make it a condition for committing patches. Especially not for simple patches that have been on list for months.
I'm strongly committed to making QMP a first class interface in QEMU for 0.15. I feel documentation is a crucial part to making that happen.
I'm not asking for test cases even though that's something that we'll need for 0.15 because there's not enough infrastructure in master yet to do that in a reasonable way. I realize I'm likely going to have to write that test case and I'm happy to do that.
Quoted 4 lines
But there's no reason that we shouldn't require thorough documentation for all new QMP commands moving forward including error semantics. This is a critical part of having a first class API and no additional infrastructure is needed in master to do this.
Broken record time, again.
Nobody denies that errors need to be specified. Fair goal for 0.15. But I must insist on proper review. No sneaking in of error specification through the commit backdoor, please.
I won't NAK non-normative error descriptions, say in commit messages, or in comments. And I won't object to you asking for them. But I feel you really shouldn't make it a condition for committing patches. Especially not for simple patches that have been on list for months.