diff mbox

[v2,1/6] pci: Use struct instead of QDict to pass back parameters

Message ID 20170118161653.19296-2-eblake@redhat.com
State New
Headers show

Commit Message

Eric Blake Jan. 18, 2017, 4:16 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().

Signed-off-by: Eric Blake <eblake@redhat.com>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/pci/pcie_aer.c | 36 +++++++++++++++++-------------------
 1 file changed, 17 insertions(+), 19 deletions(-)

Comments

Michael S. Tsirkin Jan. 18, 2017, 4:57 p.m. UTC | #1 
On Wed, Jan 18, 2017 at 10:16:48AM -0600, 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().
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> Acked-by: Michael S. Tsirkin <mst@redhat.com>

All this JSON was for the purpose of making it
easier to inject errors from unit tests or QMP.
If this doesn't help anymore, let's get rid of it.
Pls feel free to merge with the rest of the patchset
through whatever tree seems appropriate.

> ---
>  hw/pci/pcie_aer.c | 36 +++++++++++++++++-------------------
>  1 file changed, 17 insertions(+), 19 deletions(-)
> 
> diff --git a/hw/pci/pcie_aer.c b/hw/pci/pcie_aer.c
> index daf1f65..78fd2c3 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 PCIEErrorInject {
> +    const char *id;
> +    const char *root_bus;
> +    int bus;
> +    int devfn;
> +} PCIEErrorInject;
> +
>  /* 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)
>  {
> @@ -943,7 +950,8 @@ static int pcie_aer_parse_error_string(const char *error_name,
>  }
> 
>  static int do_pcie_aer_inject_error(Monitor *mon,
> -                                    const QDict *qdict, QObject **ret_data)
> +                                    const QDict *qdict,
> +                                    PCIEErrorInject *ret_data)
>  {
>      const char *id = qdict_get_str(qdict, "id");
>      const char *error_name;
> @@ -1004,34 +1012,24 @@ static int do_pcie_aer_inject_error(Monitor *mon,
>      err.prefix[2] = qdict_get_try_int(qdict, "prefix2", 0);
>      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);
> +    pcie_aer_inject_error(dev, &err);
> +    ret_data->id = id;
> +    ret_data->root_bus = pci_root_bus_path(dev);
> +    ret_data->bus = pci_bus_num(dev->bus);
> +    ret_data->devfn = dev->devfn;
> 
>      return 0;
>  }
> 
>  void hmp_pcie_aer_inject_error(Monitor *mon, const QDict *qdict)
>  {
> -    QObject *data;
> -    int devfn;
> +    PCIEErrorInject data;
> 
>      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));
> +                   data.id, data.root_bus, data.bus,
> +                   PCI_SLOT(data.devfn), PCI_FUNC(data.devfn));
>  }
> -- 
> 2.9.3
Marc-André Lureau Jan. 19, 2017, 9:08 a.m. UTC | #2 
On Wed, Jan 18, 2017 at 8:58 PM Eric Blake <eblake@redhat.com> 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().
>

and it looks like it removes a leak

>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> Acked-by: Michael S. Tsirkin <mst@redhat.com>
>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>


> ---
>  hw/pci/pcie_aer.c | 36 +++++++++++++++++-------------------
>  1 file changed, 17 insertions(+), 19 deletions(-)
>
> diff --git a/hw/pci/pcie_aer.c b/hw/pci/pcie_aer.c
> index daf1f65..78fd2c3 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 PCIEErrorInject {
> +    const char *id;
> +    const char *root_bus;
> +    int bus;
> +    int devfn;
> +} PCIEErrorInject;
> +
>  /* 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)
>  {
> @@ -943,7 +950,8 @@ static int pcie_aer_parse_error_string(const char
> *error_name,
>  }
>
>  static int do_pcie_aer_inject_error(Monitor *mon,
> -                                    const QDict *qdict, QObject
> **ret_data)
> +                                    const QDict *qdict,
> +                                    PCIEErrorInject *ret_data)
>  {
>      const char *id = qdict_get_str(qdict, "id");
>      const char *error_name;
> @@ -1004,34 +1012,24 @@ static int do_pcie_aer_inject_error(Monitor *mon,
>      err.prefix[2] = qdict_get_try_int(qdict, "prefix2", 0);
>      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);
> +    pcie_aer_inject_error(dev, &err);
> +    ret_data->id = id;
> +    ret_data->root_bus = pci_root_bus_path(dev);
> +    ret_data->bus = pci_bus_num(dev->bus);
> +    ret_data->devfn = dev->devfn;
>
>      return 0;
>  }
>
>  void hmp_pcie_aer_inject_error(Monitor *mon, const QDict *qdict)
>  {
> -    QObject *data;
> -    int devfn;
> +    PCIEErrorInject data;
>
>      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));
> +                   data.id, data.root_bus, data.bus,
> +                   PCI_SLOT(data.devfn), PCI_FUNC(data.devfn));
>  }
> --
> 2.9.3
>
>
> --
Marc-André Lureau
Markus Armbruster Jan. 19, 2017, 9:23 a.m. UTC | #3 
Eric Blake <eblake@redhat.com> writes:

> 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().
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> Acked-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  hw/pci/pcie_aer.c | 36 +++++++++++++++++-------------------
>  1 file changed, 17 insertions(+), 19 deletions(-)
>
> diff --git a/hw/pci/pcie_aer.c b/hw/pci/pcie_aer.c
> index daf1f65..78fd2c3 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 PCIEErrorInject {
> +    const char *id;
> +    const char *root_bus;
> +    int bus;
> +    int devfn;
> +} PCIEErrorInject;
> +
>  /* 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)
>  {

Aside: both pcie_aer_inject_error() and pcie_aer_msg() could be made
static.

> @@ -943,7 +950,8 @@ static int pcie_aer_parse_error_string(const char *error_name,
>  }
>
>  static int do_pcie_aer_inject_error(Monitor *mon,
> -                                    const QDict *qdict, QObject **ret_data)
> +                                    const QDict *qdict,
> +                                    PCIEErrorInject *ret_data)

Your chance to pick a better name than @ret_data, if you like.

>  {
>      const char *id = qdict_get_str(qdict, "id");
>      const char *error_name;
> @@ -1004,34 +1012,24 @@ static int do_pcie_aer_inject_error(Monitor *mon,
>      err.prefix[2] = qdict_get_try_int(qdict, "prefix2", 0);
>      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);
> +    pcie_aer_inject_error(dev, &err);

Ignores failure (assuming it can happen here).

Turns out this is no change: before, we passed the return code to the
caller, and ignored it there.  So this is an improvement of sorts.

Still, is ignoring errors correct?  For what it's worth, the other
caller of pcie_aer_inject_error() asserts it succeeds.

> +    ret_data->id = id;
> +    ret_data->root_bus = pci_root_bus_path(dev);
> +    ret_data->bus = pci_bus_num(dev->bus);
> +    ret_data->devfn = dev->devfn;
>
>      return 0;
>  }
>
>  void hmp_pcie_aer_inject_error(Monitor *mon, const QDict *qdict)
>  {
> -    QObject *data;
> -    int devfn;
> +    PCIEErrorInject data;

Your chance to pick a better name than @data, if you like.

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

Here's where we ignore the return code.

> +                   data.id, data.root_bus, data.bus,
> +                   PCI_SLOT(data.devfn), PCI_FUNC(data.devfn));
>  }

Since the fishy error ignore is pre-existing:
Reviewed-by: Markus Armbruster <armbru@redhat.com>
diff mbox

Patch

diff --git a/hw/pci/pcie_aer.c b/hw/pci/pcie_aer.c
index daf1f65..78fd2c3 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 PCIEErrorInject {
+    const char *id;
+    const char *root_bus;
+    int bus;
+    int devfn;
+} PCIEErrorInject;
+
 /* 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)
 {
@@ -943,7 +950,8 @@  static int pcie_aer_parse_error_string(const char *error_name,
 }

 static int do_pcie_aer_inject_error(Monitor *mon,
-                                    const QDict *qdict, QObject **ret_data)
+                                    const QDict *qdict,
+                                    PCIEErrorInject *ret_data)
 {
     const char *id = qdict_get_str(qdict, "id");
     const char *error_name;
@@ -1004,34 +1012,24 @@  static int do_pcie_aer_inject_error(Monitor *mon,
     err.prefix[2] = qdict_get_try_int(qdict, "prefix2", 0);
     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);
+    pcie_aer_inject_error(dev, &err);
+    ret_data->id = id;
+    ret_data->root_bus = pci_root_bus_path(dev);
+    ret_data->bus = pci_bus_num(dev->bus);
+    ret_data->devfn = dev->devfn;

     return 0;
 }

 void hmp_pcie_aer_inject_error(Monitor *mon, const QDict *qdict)
 {
-    QObject *data;
-    int devfn;
+    PCIEErrorInject data;

     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));
+                   data.id, data.root_bus, data.bus,
+                   PCI_SLOT(data.devfn), PCI_FUNC(data.devfn));
 }