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

Markus Armbruster
2010-12-15T17:39:24Z

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Wed, 15 Dec 2010 11:49:23 +0100
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Lai Jiangshan <laijs@cn.fujitsu.com> 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 <laijs@cn.fujitsu.com>
>>
>> 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.