| Message ID | 20170405194741.18956-2-eblake@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Apr 05, 2017 at 02:47:29PM -0500, Eric Blake wrote: > It's simpler to just use a C struct than it is to bundle things > into a QDict in one function just to pull them back out in the > caller. Plus, doing this gets rid of one more user of dynamic > JSON through qobject_from_jsonf(), as well as a memory leak of > the QDict. > > While cleaning the code, fix things to report all errors (the > code was previously silently ignoring a failure of > pcie_aer_inject_error(), at a distance). > > Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: Michael S. Tsirkin <mst@redhat.com> > --- > v3: more cleanups suggested by Markus, drop R-b > --- > hw/pci/pcie_aer.c | 44 ++++++++++++++++++++++++++------------------ > 1 file changed, 26 insertions(+), 18 deletions(-) > > diff --git a/hw/pci/pcie_aer.c b/hw/pci/pcie_aer.c > index a8c1820..653af86 100644 > --- a/hw/pci/pcie_aer.c > +++ b/hw/pci/pcie_aer.c > @@ -44,6 +44,13 @@ > #define PCI_ERR_SRC_COR_OFFS 0 > #define PCI_ERR_SRC_UNCOR_OFFS 2 > > +typedef struct PCIEErrorDetails { > + const char *id; > + const char *root_bus; > + int bus; > + int devfn; > +} PCIEErrorDetails; > + > /* From 6.2.7 Error Listing and Rules. Table 6-2, 6-3 and 6-4 */ > static uint32_t pcie_aer_uncor_default_severity(uint32_t status) > { > @@ -942,8 +949,14 @@ static int pcie_aer_parse_error_string(const char *error_name, > return -EINVAL; > } > > +/* > + * Inject an error described by @qdict. > + * On success, set @details to show where error was sent. > + * Return negative errno if injection failed and a message was emitted. > + */ > static int do_pcie_aer_inject_error(Monitor *mon, > - const QDict *qdict, QObject **ret_data) > + const QDict *qdict, > + PCIEErrorDetails *details) > { > const char *id = qdict_get_str(qdict, "id"); > const char *error_name; > @@ -1005,33 +1018,28 @@ static int do_pcie_aer_inject_error(Monitor *mon, > err.prefix[3] = qdict_get_try_int(qdict, "prefix3", 0); > > ret = pcie_aer_inject_error(dev, &err); > - *ret_data = qobject_from_jsonf("{'id': %s, " > - "'root_bus': %s, 'bus': %d, 'devfn': %d, " > - "'ret': %d}", > - id, pci_root_bus_path(dev), > - pci_bus_num(dev->bus), dev->devfn, > - ret); > - assert(*ret_data); > + if (ret < 0) { > + monitor_printf(mon, "failed to inject error: %s\n", > + strerror(-ret)); > + return ret; > + } > + details->id = id; > + details->root_bus = pci_root_bus_path(dev); > + details->bus = pci_bus_num(dev->bus); > + details->devfn = dev->devfn; > > return 0; > } > > void hmp_pcie_aer_inject_error(Monitor *mon, const QDict *qdict) > { > - QObject *data; > - int devfn; > + PCIEErrorDetails data; > > if (do_pcie_aer_inject_error(mon, qdict, &data) < 0) { > return; > } > > - qdict = qobject_to_qdict(data); > - assert(qdict); > - > - 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)); > + data.id, data.root_bus, data.bus, > + PCI_SLOT(data.devfn), PCI_FUNC(data.devfn)); > } > -- > 2.9.3
On 04/05/2017 10:47 PM, Eric Blake wrote: > It's simpler to just use a C struct than it is to bundle things > into a QDict in one function just to pull them back out in the > caller. Plus, doing this gets rid of one more user of dynamic > JSON through qobject_from_jsonf(), as well as a memory leak of > the QDict. > > While cleaning the code, fix things to report all errors (the > code was previously silently ignoring a failure of > pcie_aer_inject_error(), at a distance). > > Signed-off-by: Eric Blake <eblake@redhat.com> > > --- > v3: more cleanups suggested by Markus, drop R-b > --- > hw/pci/pcie_aer.c | 44 ++++++++++++++++++++++++++------------------ > 1 file changed, 26 insertions(+), 18 deletions(-) > > diff --git a/hw/pci/pcie_aer.c b/hw/pci/pcie_aer.c > index a8c1820..653af86 100644 > --- a/hw/pci/pcie_aer.c > +++ b/hw/pci/pcie_aer.c > @@ -44,6 +44,13 @@ > #define PCI_ERR_SRC_COR_OFFS 0 > #define PCI_ERR_SRC_UNCOR_OFFS 2 > > +typedef struct PCIEErrorDetails { > + const char *id; > + const char *root_bus; > + int bus; > + int devfn; > +} PCIEErrorDetails; > + > /* From 6.2.7 Error Listing and Rules. Table 6-2, 6-3 and 6-4 */ > static uint32_t pcie_aer_uncor_default_severity(uint32_t status) > { > @@ -942,8 +949,14 @@ static int pcie_aer_parse_error_string(const char *error_name, > return -EINVAL; > } > > +/* > + * Inject an error described by @qdict. > + * On success, set @details to show where error was sent. > + * Return negative errno if injection failed and a message was emitted. > + */ > static int do_pcie_aer_inject_error(Monitor *mon, > - const QDict *qdict, QObject **ret_data) > + const QDict *qdict, > + PCIEErrorDetails *details) > { > const char *id = qdict_get_str(qdict, "id"); > const char *error_name; > @@ -1005,33 +1018,28 @@ static int do_pcie_aer_inject_error(Monitor *mon, > err.prefix[3] = qdict_get_try_int(qdict, "prefix3", 0); > > ret = pcie_aer_inject_error(dev, &err); > - *ret_data = qobject_from_jsonf("{'id': %s, " > - "'root_bus': %s, 'bus': %d, 'devfn': %d, " > - "'ret': %d}", > - id, pci_root_bus_path(dev), > - pci_bus_num(dev->bus), dev->devfn, > - ret); > - assert(*ret_data); > + if (ret < 0) { > + monitor_printf(mon, "failed to inject error: %s\n", > + strerror(-ret)); > + return ret; > + } > + details->id = id; > + details->root_bus = pci_root_bus_path(dev); > + details->bus = pci_bus_num(dev->bus); > + details->devfn = dev->devfn; > > return 0; > } > > void hmp_pcie_aer_inject_error(Monitor *mon, const QDict *qdict) > { > - QObject *data; > - int devfn; > + PCIEErrorDetails data; > > if (do_pcie_aer_inject_error(mon, qdict, &data) < 0) { > return; > } > > - qdict = qobject_to_qdict(data); > - assert(qdict); > - > - 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)); > + data.id, data.root_bus, data.bus, > + PCI_SLOT(data.devfn), PCI_FUNC(data.devfn)); > } > Reviewed-by: Marcel Apfelbaum <marcel@redhat.com> Thanks, Marcel
diff --git a/hw/pci/pcie_aer.c b/hw/pci/pcie_aer.c index a8c1820..653af86 100644 --- a/hw/pci/pcie_aer.c +++ b/hw/pci/pcie_aer.c @@ -44,6 +44,13 @@ #define PCI_ERR_SRC_COR_OFFS 0 #define PCI_ERR_SRC_UNCOR_OFFS 2 +typedef struct PCIEErrorDetails { + const char *id; + const char *root_bus; + int bus; + int devfn; +} PCIEErrorDetails; + /* From 6.2.7 Error Listing and Rules. Table 6-2, 6-3 and 6-4 */ static uint32_t pcie_aer_uncor_default_severity(uint32_t status) { @@ -942,8 +949,14 @@ static int pcie_aer_parse_error_string(const char *error_name, return -EINVAL; } +/* + * Inject an error described by @qdict. + * On success, set @details to show where error was sent. + * Return negative errno if injection failed and a message was emitted. + */ static int do_pcie_aer_inject_error(Monitor *mon, - const QDict *qdict, QObject **ret_data) + const QDict *qdict, + PCIEErrorDetails *details) { const char *id = qdict_get_str(qdict, "id"); const char *error_name; @@ -1005,33 +1018,28 @@ static int do_pcie_aer_inject_error(Monitor *mon, err.prefix[3] = qdict_get_try_int(qdict, "prefix3", 0); ret = pcie_aer_inject_error(dev, &err); - *ret_data = qobject_from_jsonf("{'id': %s, " - "'root_bus': %s, 'bus': %d, 'devfn': %d, " - "'ret': %d}", - id, pci_root_bus_path(dev), - pci_bus_num(dev->bus), dev->devfn, - ret); - assert(*ret_data); + if (ret < 0) { + monitor_printf(mon, "failed to inject error: %s\n", + strerror(-ret)); + return ret; + } + details->id = id; + details->root_bus = pci_root_bus_path(dev); + details->bus = pci_bus_num(dev->bus); + details->devfn = dev->devfn; return 0; } void hmp_pcie_aer_inject_error(Monitor *mon, const QDict *qdict) { - QObject *data; - int devfn; + PCIEErrorDetails data; if (do_pcie_aer_inject_error(mon, qdict, &data) < 0) { return; } - qdict = qobject_to_qdict(data); - assert(qdict); - - 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)); + data.id, data.root_bus, data.bus, + PCI_SLOT(data.devfn), PCI_FUNC(data.devfn)); }
It's simpler to just use a C struct than it is to bundle things into a QDict in one function just to pull them back out in the caller. Plus, doing this gets rid of one more user of dynamic JSON through qobject_from_jsonf(), as well as a memory leak of the QDict. While cleaning the code, fix things to report all errors (the code was previously silently ignoring a failure of pcie_aer_inject_error(), at a distance). Signed-off-by: Eric Blake <eblake@redhat.com> --- v3: more cleanups suggested by Markus, drop R-b --- hw/pci/pcie_aer.c | 44 ++++++++++++++++++++++++++------------------ 1 file changed, 26 insertions(+), 18 deletions(-)