From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=43354 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PSvX8-00046h-Nl for qemu-devel@nongnu.org; Wed, 15 Dec 2010 12:53:00 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PSvWt-00028z-72 for qemu-devel@nongnu.org; Wed, 15 Dec 2010 12:52:45 -0500 Received: from mx1.redhat.com ([209.132.183.28]:2923) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PSvWs-00028n-Uo for qemu-devel@nongnu.org; Wed, 15 Dec 2010 12:52:43 -0500 Date: Wed, 15 Dec 2010 15:52:36 -0200 From: Luiz Capitulino Subject: Re: [Qemu-devel] [PATCH v3] qemu, qmp: convert do_inject_nmi() to QObject, QError Message-ID: <20101215155236.5f019f55@doriath> In-Reply-To: References: <4D088F27.8000909@cn.fujitsu.com> <20101215151411.0f52b692@doriath> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: avi@redhat.com, Lai Jiangshan , kvm@vger.kernel.org, qemu-devel@nongnu.org On Wed, 15 Dec 2010 18:39:07 +0100 Markus Armbruster wrote: > 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? At this point in time yes, but they might not be in the near future. Assuming they might be different is the safest thing to do. That's true for all existing commands. > Giving the same things the same name isn't coupling :) Expecting them to be the same in the future is. > The mistake that matters here was adopting existing human commands for > QMP uncritically. That's the protocol visible mistake, yes. > > 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. It's not supposed to be typed that much, but I'm not that strong about that. nitpick: I think we should be consistent in the use of "_" or "-", eg. we should pick inject-nmi or inject_nmi? > > >> Regardless, the differing command name is worth mentioning in the commit > >> message. >