Re: [Qemu-devel] [PATCH v3] qemu, qmp: convert do_inject_nmi() to QObject, QError

[Markus Armbruster]

Luiz Capitulino <address@hidden> writes:

> On Wed, 15 Dec 2010 11:49:23 +0100
> Markus Armbruster <address@hidden> wrote:
>
>> Lai Jiangshan <address@hidden> writes:
>> 
>> > Convert do_inject_nmi() to QObject, QError, we need to use it(via libvirt).
>> >
>> > changed from v1
>> > Add document.
>> > Add error handling when the cpu index is invalid.
>> >
>> > changed from v2
>> > use QERR_INVALID_PARAMETER_VALUE as Markus suggest.
>> >
>> > Signed-off-by:  Lai Jiangshan <address@hidden>
>> 
>> A note on commit messages:
>> 
>> The commit message should describe the current version of the patch.
>> Don't repeat the subject line in the body.
>> 
>> Patch history is very useful for review, but usually uninteresting once
>> the patch is committed.  Thus, it's best to put it after the "---"
>> separator.
>> 
>> Subsystem tags in the subject line are helpful.  But "qemu" doesn't
>> provide any information there :)
>> 
>> 
>> Regarding the patch:
>> 
>> The conversion looks good.
>> 
>> The new QMP command is called "inject_nmi", while the existing human
>> monitor command is called "nmi".  Luiz asked for this name change.  I
>> don't mind.  But should we rename the human monitor command for
>> consistency?
>
> I don't think so, we don't need (and maybe don't even want) naming parity
> between QMP and HMP. Remember that one of our mistakes was to couple the two.

Human "nmi" and QMP "inject_nmi" are identical commands, aren't they?
Giving the same things the same name isn't coupling :)

The mistake that matters here was adopting existing human commands for
QMP uncritically.

> Also, Avi asked for more descriptive names in QMP and I agree with him, I
> would even be in favor of calling it inject-non-maskable-interrupt.

I like inject_nmi better than nmi.  inject-non-maskable-interrupt is too
long even for QMP.

>> Regardless, the differing command name is worth mentioning in the commit
>> message.