From mboxrd@z Thu Jan 1 00:00:00 1970 From: Markus Armbruster Subject: Re: [Qemu-devel] [PATCH v3] qemu, qmp: convert do_inject_nmi() to QObject, QError Date: Wed, 15 Dec 2010 18:39:07 +0100 Message-ID: References: <4D088F27.8000909@cn.fujitsu.com> <20101215151411.0f52b692@doriath> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Lai Jiangshan , qemu-devel@nongnu.org, kvm@vger.kernel.org, avi@redhat.com To: Luiz Capitulino Return-path: Received: from mx1.redhat.com ([209.132.183.28]:22721 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751012Ab0LORjY (ORCPT ); Wed, 15 Dec 2010 12:39:24 -0500 In-Reply-To: <20101215151411.0f52b692@doriath> (Luiz Capitulino's message of "Wed, 15 Dec 2010 15:14:11 -0200") Sender: kvm-owner@vger.kernel.org List-ID: Luiz Capitulino writes: > On Wed, 15 Dec 2010 11:49:23 +0100 > Markus Armbruster wrote: > >> Lai Jiangshan 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 >> >> 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.