diff mbox

[v3,01/13] pci: Use struct instead of QDict to pass back parameters

Message ID 20170405194741.18956-2-eblake@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Eric Blake April 5, 2017, 7:47 p.m. UTC
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(-)

Comments

Michael S. Tsirkin April 5, 2017, 10:04 p.m. UTC | #1
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
Marcel Apfelbaum April 6, 2017, 5:41 a.m. UTC | #2
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 mbox

Patch

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));
 }