| Message ID | 1432294585-5984-8-git-send-email-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
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. > + 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). But that's minor; whether or not you clean things up, Reviewed-by: Eric Blake <eblake@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!
On 05/26/2015 03:25 AM, Markus Armbruster wrote: > 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. >>> >> >>> + >>> + 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. I totally agree - mixing cleanups into code motion makes the motion harder to review. If it gets cleaned up, it should be as a separate patch, at which point it starts to be out of scope of the goal of this series, and better as a separate patch to qemu-trivial or to the PCI tree.
diff --git a/hmp-commands.hx b/hmp-commands.hx index 7776f16..0084114 100644 --- a/hmp-commands.hx +++ b/hmp-commands.hx @@ -1184,8 +1184,7 @@ ETEXI "<error_status> = error string or 32bit\n\t\t\t" "<tlb header> = 32bit x 4\n\t\t\t" "<tlb header prefix> = 32bit x 4", - .user_print = pcie_aer_inject_error_print, - .mhandler.cmd_new = hmp_pcie_aer_inject_error, + .mhandler.cmd = hmp_pcie_aer_inject_error, }, STEXI diff --git a/hw/pci/pci-stub.c b/hw/pci/pci-stub.c index 5e564c3..f8f237e 100644 --- a/hw/pci/pci-stub.c +++ b/hw/pci/pci-stub.c @@ -29,19 +29,7 @@ PciInfoList *qmp_query_pci(Error **errp) return NULL; } -static void pci_error_message(Monitor *mon) +void hmp_pcie_aer_inject_error(Monitor *mon, const QDict *qdict) { monitor_printf(mon, "PCI devices not supported\n"); } - -int hmp_pcie_aer_inject_error(Monitor *mon, - const QDict *qdict, QObject **ret_data) -{ - pci_error_message(mon); - return -ENOSYS; -} - -void pcie_aer_inject_error_print(Monitor *mon, const QObject *data) -{ - pci_error_message(mon); -} diff --git a/hw/pci/pcie_aer.c b/hw/pci/pcie_aer.c index b48c09c..c8dea8e 100644 --- a/hw/pci/pcie_aer.c +++ b/hw/pci/pcie_aer.c @@ -815,21 +815,6 @@ const VMStateDescription vmstate_pcie_aer_log = { } }; -void pcie_aer_inject_error_print(Monitor *mon, const QObject *data) -{ - QDict *qdict; - int devfn; - assert(qobject_type(data) == QTYPE_QDICT); - qdict = qobject_to_qdict(data); - - devfn = (int)qdict_get_int(qdict, "devfn"); - 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"), - PCI_SLOT(devfn), PCI_FUNC(devfn)); -} - typedef struct PCIEAERErrorName { const char *name; uint32_t val; @@ -962,8 +947,8 @@ static int pcie_aer_parse_error_string(const char *error_name, return -EINVAL; } -int hmp_pcie_aer_inject_error(Monitor *mon, - const QDict *qdict, QObject **ret_data) +static int do_pcie_aer_inject_error(Monitor *mon, + const QDict *qdict, QObject **ret_data) { const char *id = qdict_get_str(qdict, "id"); const char *error_name; @@ -1035,3 +1020,23 @@ int hmp_pcie_aer_inject_error(Monitor *mon, return 0; } + +void hmp_pcie_aer_inject_error(Monitor *mon, const QDict *qdict) +{ + QObject *data; + int devfn; + + if (do_pcie_aer_inject_error(mon, qdict, &data) < 0) { + return; + } + + assert(qobject_type(data) == QTYPE_QDICT); + qdict = qobject_to_qdict(data); + + devfn = (int)qdict_get_int(qdict, "devfn"); + 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"), + PCI_SLOT(devfn), PCI_FUNC(devfn)); +} diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h index 8a52934..e10c2c5 100644 --- a/include/sysemu/sysemu.h +++ b/include/sysemu/sysemu.h @@ -161,9 +161,7 @@ extern unsigned int nb_prom_envs; void hmp_drive_add(Monitor *mon, const QDict *qdict); /* pcie aer error injection */ -void pcie_aer_inject_error_print(Monitor *mon, const QObject *data); -int hmp_pcie_aer_inject_error(Monitor *mon, - const QDict *qdict, QObject **ret_data); +void hmp_pcie_aer_inject_error(Monitor *mon, const QDict *qdict); /* serial ports */
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(-)