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

Signed-off-by:  Lai Jiangshan <laijs@cn.fujitsu.com>
---

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 Jiangshan<laijs@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",
>
>

On Wed, 23 Feb 2011 13:25:38 -0600
Anthony Liguori <anthony@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 Jiangshan<laijs@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",
> >
> >    
>

Anthony Liguori <anthony@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 Jiangshan<laijs@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.

On 02/24/2011 02:33 AM, Markus Armbruster wrote:
> Anthony Liguori<anthony@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 Jiangshan<laijs@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:

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

Anthony Liguori <aliguori@linux.vnet.ibm.com> writes:

> On 02/24/2011 02:33 AM, Markus Armbruster wrote:
>> Anthony Liguori<anthony@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 Jiangshan<laijs@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 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:
>
> 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)

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?

On 02/24/2011 10:20 AM, Markus Armbruster wrote:
> Anthony Liguori<aliguori@linux.vnet.ibm.com>  writes:
>
>    
>> On 02/24/2011 02:33 AM, Markus Armbruster wrote:
>>      
>>> Anthony Liguori<anthony@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 Jiangshan<laijs@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 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



>>>>                                         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:
>>
>> 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)
>>      
> 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?
>

Anthony Liguori <aliguori@linux.vnet.ibm.com> writes:

> On 02/24/2011 10:20 AM, Markus Armbruster wrote:
>> Anthony Liguori<aliguori@linux.vnet.ibm.com>  writes:
>>
>>    
>>> On 02/24/2011 02:33 AM, Markus Armbruster wrote:
>>>      
>>>> Anthony Liguori<anthony@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.

On 02/25/2011 03:54 AM, Markus Armbruster wrote:
> Anthony Liguori<aliguori@linux.vnet.ibm.com>  writes:
>
>    
>> On 02/24/2011 10:20 AM, Markus Armbruster wrote:
>>      
>>> Anthony Liguori<aliguori@linux.vnet.ibm.com>   writes:
>>>
>>>
>>>        
>>>> On 02/24/2011 02:33 AM, Markus Armbruster wrote:
>>>>
>>>>          
>>>>> Anthony Liguori<anthony@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:
>    

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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Anthony Liguori <anthony@codemonkey.ws> writes:

> On 02/25/2011 03:54 AM, Markus Armbruster wrote:
>> Anthony Liguori<aliguori@linux.vnet.ibm.com>  writes:
>>
>>    
>>> On 02/24/2011 10:20 AM, Markus Armbruster wrote:
>>>      
>>>> Anthony Liguori<aliguori@linux.vnet.ibm.com>   writes:
>>>>
>>>>
>>>>        
>>>>> On 02/24/2011 02:33 AM, Markus Armbruster wrote:
>>>>>
>>>>>          
>>>>>> Anthony Liguori<anthony@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:
>>    
>
> 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.