Re: [Qemu-devel] [PATCH V6 3/4] qmp, nmi: convert do_inject_nmi() to QObject

2011-02-27 Thread Markus Armbruster 
Anthony Liguori anth...@codemonkey.ws writes:

 On 02/25/2011 03:54 AM, Markus Armbruster wrote:
 Anthony Liguorialigu...@linux.vnet.ibm.com  writes:


 On 02/24/2011 10:20 AM, Markus Armbruster wrote:
  
 Anthony Liguorialigu...@linux.vnet.ibm.com   writes:



 On 02/24/2011 02:33 AM, Markus Armbruster wrote:

  
 Anthony Liguorianth...@codemonkey.wswrites:

 [...]

 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.



 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.


 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.
  
 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.

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.

 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.

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.
--
To unsubscribe from this list: send the line

Re: [Qemu-devel] [PATCH V6 3/4] qmp, nmi: convert do_inject_nmi() to QObject

2011-02-25 Thread Markus Armbruster 
Anthony Liguori aligu...@linux.vnet.ibm.com writes:

 On 02/24/2011 10:20 AM, Markus Armbruster wrote:
 Anthony Liguorialigu...@linux.vnet.ibm.com  writes:


 On 02/24/2011 02:33 AM, Markus Armbruster wrote:
  
 Anthony Liguorianth...@codemonkey.ws   writes:
[...]
 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.


 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.


 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.

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:

 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.
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
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

2011-02-25 Thread Anthony Liguori 

On 02/25/2011 03:54 AM, Markus Armbruster wrote:

Anthony Liguorialigu...@linux.vnet.ibm.com  writes:

   

On 02/24/2011 10:20 AM, Markus Armbruster wrote:
 

Anthony Liguorialigu...@linux.vnet.ibm.com   writes:


   

On 02/24/2011 02:33 AM, Markus Armbruster wrote:

 

Anthony Liguorianth...@codemonkey.wswrites:
   

[...]
   

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.


   

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.

   

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.
 

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.



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


--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
   


--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
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

2011-02-24 Thread Markus Armbruster 
Anthony Liguori anth...@codemonkey.ws writes:

 On 01/27/2011 02:20 AM, Lai Jiangshan wrote:
 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 Jiangshanla...@cn.fujitsu.com
 ---
 diff --git a/hmp-commands.hx b/hmp-commands.hx
 index ec1a4db..e763bf9 100644
 --- a/hmp-commands.hx
 +++ b/hmp-commands.hx
 @@ -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
 diff --git a/monitor.c b/monitor.c
 index 387b020..1b1c0ba 100644
 --- a/monitor.c
 +++ b/monitor.c
 @@ -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);
   else
   cpu_interrupt(env, CPU_INTERRUPT_NMI);
 -break;
 +return 0;
   }
 +
 +return -1;
   }
   #endif

 diff --git a/qmp-commands.hx b/qmp-commands.hx
 index 56c4d8b..a887dd5 100644
 --- a/qmp-commands.hx
 +++ b/qmp-commands.hx
 @@ -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: {} }


 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.

   Don't hide this command for
 !defined(TARGET_I386), instead have it throw an error in the
 implementation.

Works for me.

 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.
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
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

2011-02-24 Thread Anthony Liguori 

On 02/24/2011 02:33 AM, Markus Armbruster wrote:

Anthony Liguorianth...@codemonkey.ws  writes:

   

On 01/27/2011 02:20 AM, Lai Jiangshan wrote:
 

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 Jiangshanla...@cn.fujitsu.com
---
diff --git a/hmp-commands.hx b/hmp-commands.hx
index ec1a4db..e763bf9 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -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
diff --git a/monitor.c b/monitor.c
index 387b020..1b1c0ba 100644
--- a/monitor.c
+++ b/monitor.c
@@ -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);
   else
   cpu_interrupt(env, CPU_INTERRUPT_NMI);
-break;
+return 0;
   }
+
+return -1;
   }
   #endif

diff --git a/qmp-commands.hx b/qmp-commands.hx
index 56c4d8b..a887dd5 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -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: {} }

   

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.
   


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.



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.



   Don't hide this command for
!defined(TARGET_I386), instead have it throw an error in the
implementation.
 

Works for me.

   

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.
   


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:

Re: [Qemu-devel] [PATCH V6 3/4] qmp, nmi: convert do_inject_nmi() to QObject

2011-02-24 Thread Markus Armbruster 
Anthony Liguori aligu...@linux.vnet.ibm.com writes:

 On 02/24/2011 02:33 AM, Markus Armbruster wrote:
 Anthony Liguorianth...@codemonkey.ws  writes:


 On 01/27/2011 02:20 AM, Lai Jiangshan wrote:
  
 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 Jiangshanla...@cn.fujitsu.com
 ---
 diff --git a/hmp-commands.hx b/hmp-commands.hx
 index ec1a4db..e763bf9 100644
 --- a/hmp-commands.hx
 +++ b/hmp-commands.hx
 @@ -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
 diff --git a/monitor.c b/monitor.c
 index 387b020..1b1c0ba 100644
 --- a/monitor.c
 +++ b/monitor.c
 @@ -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);
else
cpu_interrupt(env, CPU_INTERRUPT_NMI);
 -break;
 +return 0;
}
 +
 +return -1;
}
#endif

 diff --git a/qmp-commands.hx b/qmp-commands.hx
 index 56c4d8b..a887dd5 100644
 --- a/qmp-commands.hx
 +++ b/qmp-commands.hx
 @@ -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: {} }


 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.


 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.

 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.

Don't hide this command for
 !defined(TARGET_I386), instead have it throw an error in the
 implementation.
  
 Works for me.


 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

Re: [Qemu-devel] [PATCH V6 3/4] qmp, nmi: convert do_inject_nmi() to QObject

2011-02-24 Thread Anthony Liguori 

On 02/24/2011 10:20 AM, Markus Armbruster wrote:

Anthony Liguorialigu...@linux.vnet.ibm.com  writes:

   

On 02/24/2011 02:33 AM, Markus Armbruster wrote:
 

Anthony Liguorianth...@codemonkey.ws   writes:


   

On 01/27/2011 02:20 AM, Lai Jiangshan wrote:

 

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 Jiangshanla...@cn.fujitsu.com
---
diff --git a/hmp-commands.hx b/hmp-commands.hx
index ec1a4db..e763bf9 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -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
diff --git a/monitor.c b/monitor.c
index 387b020..1b1c0ba 100644
--- a/monitor.c
+++ b/monitor.c
@@ -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);
else
cpu_interrupt(env, CPU_INTERRUPT_NMI);
-break;
+return 0;
}
+
+return -1;
}
#endif

diff --git a/qmp-commands.hx b/qmp-commands.hx
index 56c4d8b..a887dd5 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -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: {} }


   

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.

   

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.
   


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.



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

Re: [Qemu-devel] [PATCH V6 3/4] qmp, nmi: convert do_inject_nmi() to QObject

2011-02-23 Thread Anthony Liguori 

On 01/27/2011 02:20 AM, Lai Jiangshan wrote:

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 Jiangshanla...@cn.fujitsu.com
---
diff --git a/hmp-commands.hx b/hmp-commands.hx
index ec1a4db..e763bf9 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -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
diff --git a/monitor.c b/monitor.c
index 387b020..1b1c0ba 100644
--- a/monitor.c
+++ b/monitor.c
@@ -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);
  else
  cpu_interrupt(env, CPU_INTERRUPT_NMI);
-break;
+return 0;
  }
+
+return -1;
  }
  #endif

diff --git a/qmp-commands.hx b/qmp-commands.hx
index 56c4d8b..a887dd5 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -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: {} }
   


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


+
+EQMP
+
  {
  .name   = migrate,
  .args_type  = detach:-d,blk:-b,inc:-i,uri:s,

   


--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
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

2011-02-23 Thread Luiz Capitulino 
On Wed, 23 Feb 2011 13:25:38 -0600
Anthony Liguori anth...@codemonkey.ws wrote:

 On 01/27/2011 02:20 AM, Lai Jiangshan wrote:
  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 Jiangshanla...@cn.fujitsu.com
  ---
  diff --git a/hmp-commands.hx b/hmp-commands.hx
  index ec1a4db..e763bf9 100644
  --- a/hmp-commands.hx
  +++ b/hmp-commands.hx
  @@ -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
  diff --git a/monitor.c b/monitor.c
  index 387b020..1b1c0ba 100644
  --- a/monitor.c
  +++ b/monitor.c
  @@ -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);
else
cpu_interrupt(env, CPU_INTERRUPT_NMI);
  -break;
  +return 0;
}
  +
  +return -1;
}
#endif
 
  diff --git a/qmp-commands.hx b/qmp-commands.hx
  index 56c4d8b..a887dd5 100644
  --- a/qmp-commands.hx
  +++ b/qmp-commands.hx
  @@ -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: {} }
 
 
 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.

We had this exactly same discussion last year:

 http://lists.gnu.org/archive/html/qemu-devel/2010-12/msg01285.html

 
 Regards,
 
 Anthony Liguori
 
  +
  +EQMP
  +
{
.name   = migrate,
.args_type  = detach:-d,blk:-b,inc:-i,uri:s,
 
 
 

--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html