From: Markus Armbruster <armbru@redhat.com>
References: <1432294585-5984-1-git-send-email-armbru@redhat.com>
	<1432294585-5984-8-git-send-email-armbru@redhat.com>
	<555FA834.2080506@redhat.com>
Date: Tue, 26 May 2015 11:25:17 +0200
In-Reply-To: <555FA834.2080506@redhat.com> (Eric Blake's message of "Fri, 22
	May 2015 16:05:40 -0600")
Message-ID: <87a8wrelmq.fsf@blackfin.pond.sub.org>
MIME-Version: 1.0
Content-Type: text/plain
Subject: Re: [Qemu-devel] [PATCH 07/20] monitor: Use trad. command interface
	for HMP pcie_aer_inject_error
To: Eric Blake <eblake@redhat.com>
Cc: qemu-devel@nongnu.org, lcapitulino@redhat.com

Eric Blake <eblake@redhat.com> writes:

> On 05/22/2015 05:36 AM, Markus Armbruster wrote:
>> All QMP commands use the "new" handler interface (mhandler.cmd_new).
>> Most HMP commands still use the traditional interface (mhandler.cmd),
>> but a few use the "new" one.  Complicates handle_user_command() for no
>> gain, so I'm converting these to the traditional interface.
>> 
>> pcie_aer_inject_error's implementation is split into the
>> hmp_pcie_aer_inject_error() and pcie_aer_inject_error_print().  The
>> former is a peculiar crossbreed between HMP and QMP handler.  On
>> success, it works like a QMP handler: store QDict through ret_data
>> parameter, return 0.  Printing the QDict is left to
>> pcie_aer_inject_error_print().  On failure, it works more like an HMP
>> handler: print error to monitor, return negative number.
>> 
>> To convert to the traditional interface, turn
>> pcie_aer_inject_error_print() into a command handler wrapping around
>> hmp_pcie_aer_inject_error().  By convention, this command handler
>> should be called hmp_pcie_aer_inject_error(), so rename the existing
>> hmp_pcie_aer_inject_error() to do_pcie_aer_inject_error().
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  hmp-commands.hx         |  3 +--
>>  hw/pci/pci-stub.c       | 14 +-------------
>>  hw/pci/pcie_aer.c       | 39 ++++++++++++++++++++++-----------------
>>  include/sysemu/sysemu.h |  4 +---
>>  4 files changed, 25 insertions(+), 35 deletions(-)
>> 
>
>> +
>> +    devfn = (int)qdict_get_int(qdict, "devfn");
>
> Code motion, so the problem is pre-existing, but this cast is unneeded.
>  Up to you if you want to clean it up now.

While I occasionally do fold cleanups into mechanical changes, e.g in
PATCH 15, I'm reluctant to fold them into code motion.

>> +    monitor_printf(mon, "OK id: %s root bus: %s, bus: %x devfn: %x.%x\n",
>> +                   qdict_get_str(qdict, "id"),
>> +                   qdict_get_str(qdict, "root_bus"),
>> +                   (int) qdict_get_int(qdict, "bus"),
>
> However, this one still is necessary, unless you tweak the format string
> (if you haven't guessed, I prefer avoiding casts when other mechanisms
> work equally well).

I share your preference, but I want to keep the changes in PCI code to a
trivial minimum, to keep this series firmly in the monitor subsystem.

> But that's minor; whether or not you clean things up,
>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!