[ovs-dev,v12,6/6] ofproto: Add JSON output for 'dpif/show' command.

From: Jakob Meng <code@jakobmeng.de>

The 'dpif/show' command now supports machine-readable JSON output in
addition to the plain-text output for humans. An example would be:

  ovs-appctl --format json dpif/show

Reported-at: https://bugzilla.redhat.com/1824861
Signed-off-by: Jakob Meng <code@jakobmeng.de>
---
 NEWS                   |   1 +
 ofproto/ofproto-dpif.c | 120 +++++++++++++++++++++++++++++++++++++----
 tests/pmd.at           |  23 ++++++++
 3 files changed, 133 insertions(+), 11 deletions(-)

On 5/16/24 17:41, jmeng@redhat.com wrote:
> From: Jakob Meng <code@jakobmeng.de>
> 
> The 'dpif/show' command now supports machine-readable JSON output in
> addition to the plain-text output for humans. An example would be:
> 
>   ovs-appctl --format json dpif/show
> 
> Reported-at: https://bugzilla.redhat.com/1824861
> Signed-off-by: Jakob Meng <code@jakobmeng.de>
> ---
>  NEWS                   |   1 +
>  ofproto/ofproto-dpif.c | 120 +++++++++++++++++++++++++++++++++++++----
>  tests/pmd.at           |  23 ++++++++
>  3 files changed, 133 insertions(+), 11 deletions(-)
> 
> diff --git a/NEWS b/NEWS
> index 479310d49..c434b1497 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -5,6 +5,7 @@ Post-v3.3.0
>         or 'text' (by default).
>       * Added new option [--pretty] to print JSON output in a readable fashion.
>       * 'list-commands' now supports output in JSON format.
> +     * 'dpif/show' now supports output in JSON format.

Might be better to squash above two entries into one.

>     - Python:
>       * Added support for different output formats like 'json' to appctl.py and
>         Python's unixctl classes.
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 32d037be6..db0405886 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -28,6 +28,7 @@
>  #include "fail-open.h"
>  #include "guarded-list.h"
>  #include "hmapx.h"
> +#include "json.h"
>  #include "lacp.h"
>  #include "learn.h"
>  #include "mac-learning.h"
> @@ -6519,8 +6520,99 @@ done:
>      return changed;
>  }
>  
> +static struct json *
> +dpif_show_backer_json(struct json *backers, const struct dpif_backer *backer)
> +{
> +    struct json *json_backer = json_object_create();
> +
> +    /* Add datapath as new JSON object using its name as key. */
> +    json_object_put(backers, dpif_name(backer->dpif), json_backer);
> +
> +    /* Add datapath's stats under "stats" key. */
> +    struct dpif_dp_stats dp_stats;
> +    struct json *json_dp_stats = json_object_create();

Reverse x-mass tree.

> +
> +    json_object_put(json_backer, "stats", json_dp_stats);
> +    dpif_get_dp_stats(backer->dpif, &dp_stats);
> +    json_object_put_format(json_dp_stats, "n_hit", "%"PRIu64, dp_stats.n_hit);
> +    json_object_put_format(json_dp_stats, "n_missed", "%"PRIu64,
> +                           dp_stats.n_missed);

It's better to build the json_dp_stats first, and then add it to json_backer.


> +
> +    /* Add datapath's bridges under "bridges" key. */
> +    struct json *json_dp_bridges = json_object_create();
> +    json_object_put(json_backer, "bridges", json_dp_bridges);

Move to the end.

> +
> +    struct shash ofproto_shash;
> +    shash_init(&ofproto_shash);

Use SHASH_INITIALIZER.

> +    const struct shash_node **ofprotos = get_ofprotos(&ofproto_shash);
> +    for (size_t i = 0; i < shash_count(&ofproto_shash); i++) {

But also, there is no need to sort things while adding to json objects.
The order will be lost and we'll re-sort on output.

So, we could just free(get_ofprotos(&ofproto_shash)); and then use normal
SHASH_FOR_EACH to iterate.

> +        struct ofproto_dpif *ofproto = ofprotos[i]->data;
> +
> +        if (ofproto->backer != backer) {
> +            continue;
> +        }
> +
> +        struct json *json_ofproto = json_object_create();
> +        /* Add bridge to "bridges" dictionary using its name as key. */
> +        json_object_put(json_dp_bridges, ofproto->up.name, json_ofproto);

Move to the end.

> +
> +        /* Add bridge ports to the current bridge dictionary. */
> +        const struct shash_node **ports;
> +        ports = shash_sort(&ofproto->up.port_by_name);
> +        for (size_t j = 0; j < shash_count(&ofproto->up.port_by_name); j++) {

Just iterate SHASH directly.

> +            const struct shash_node *port = ports[j];
> +            struct ofport *ofport = port->data;
> +
> +            struct json * json_ofproto_port = json_object_create();

struct json *json_ofproto_port

> +            /* Add bridge port to a bridge's dict using port name as key. */
> +            json_object_put(json_ofproto, netdev_get_name(ofport->netdev),
> +                            json_ofproto_port);

To the end.

> +            /* Add OpenFlow port associated with a bridge port. */
> +            json_object_put_format(json_ofproto_port, "ofport", "%u",

PRIu32

> +                                   ofport->ofp_port);
> +
> +            /* Add bridge port number. */
> +            odp_port_t odp_port = ofp_port_to_odp_port(ofproto,
> +                                                       ofport->ofp_port);
> +            if (odp_port != ODPP_NONE) {
> +                json_object_put_format(json_ofproto_port, "port_no",
> +                                       "%"PRIu32, odp_port);
> +            } else {
> +                json_object_put_string(json_ofproto_port, "port_no", "none");
> +            }
> +
> +            /* Add type of a bridge port. */
> +            json_object_put_string(json_ofproto_port, "type",
> +                                   netdev_get_type(ofport->netdev));
> +
> +            /* Add config entries for a bridge port. */
> +            struct json *json_port_config = json_object_create();
> +            struct smap config;
> +            smap_init(&config);

SMAP_INITIALIZER

> +
> +            json_object_put(json_ofproto_port, "config", json_port_config);

To the end.

> +            if (!netdev_get_config(ofport->netdev, &config)) {
> +                struct smap_node *node;
> +
> +                SMAP_FOR_EACH (node, &config) {
> +                    json_object_put_string(json_port_config, node->key,
> +                                           node->value);
> +                }
> +            }
> +            smap_destroy(&config);
> +
> +        } /* End of bridge port(s). */
> +
> +        free(ports);
> +    } /* End of bridge(s). */

An empty line would be nice.

> +    shash_destroy(&ofproto_shash);
> +    free(ofprotos);
> +
> +    return json_backer;
> +}
> +
>  static void
> -dpif_show_backer(const struct dpif_backer *backer, struct ds *ds)
> +dpif_show_backer_text(const struct dpif_backer *backer, struct ds *ds)
>  {
>      const struct shash_node **ofprotos;
>      struct dpif_dp_stats dp_stats;
> @@ -6587,18 +6679,24 @@ static void
>  ofproto_unixctl_dpif_show(struct unixctl_conn *conn, int argc OVS_UNUSED,
>                            const char *argv[] OVS_UNUSED, void *aux OVS_UNUSED)
>  {
> -    struct ds ds = DS_EMPTY_INITIALIZER;
> -    const struct shash_node **backers;
> -    int i;
> +    if (unixctl_command_get_output_format(conn) == UNIXCTL_OUTPUT_FMT_JSON) {
> +        struct json *backers = json_object_create();
> +        const struct shash_node *backer;

An empty line.

> +        SHASH_FOR_EACH (backer, &all_dpif_backers) {
> +            dpif_show_backer_json(backers, backer->data);
> +        }
> +        unixctl_command_reply_json(conn, backers);
> +    } else {
> +        struct ds ds = DS_EMPTY_INITIALIZER;
> +        const struct shash_node **backers = shash_sort(&all_dpif_backers);

An empty line.  Also, reverse x-mass tree.

> +        for (int i = 0; i < shash_count(&all_dpif_backers); i++) {
> +            dpif_show_backer_text(backers[i]->data, &ds);
> +        }
> +        free(backers);
>  
> -    backers = shash_sort(&all_dpif_backers);
> -    for (i = 0; i < shash_count(&all_dpif_backers); i++) {
> -        dpif_show_backer(backers[i]->data, &ds);
> +        unixctl_command_reply(conn, ds_cstr(&ds));
> +        ds_destroy(&ds);
>      }
> -    free(backers);
> -
> -    unixctl_command_reply(conn, ds_cstr(&ds));
> -    ds_destroy(&ds);
>  }
>  
>  static void
> diff --git a/tests/pmd.at b/tests/pmd.at
> index 35a44b4df..189138f81 100644
> --- a/tests/pmd.at
> +++ b/tests/pmd.at

pmd.at is not a right place for a test.  There should be a test
in ofproto-dpif.at instead.  We already have one there for this
particular appctl command.

> @@ -112,6 +112,29 @@ dummy@ovs-dummy: hit:0 missed:0
>      p0 1/1: (dummy-pmd: n_rxq=1, n_txq=1, numa_id=0)
>  ])
>  
> +AT_CHECK([ovs-appctl --format json --pretty dpif/show], [0], [dnl
> +[{
> +  "dummy@ovs-dummy": {
> +    "bridges": {
> +      "br0": {
> +        "br0": {
> +          "config": {
> +            },
> +          "ofport": "65534",
> +          "port_no": "100",
> +          "type": "dummy-internal"},
> +        "p0": {
> +          "config": {
> +            "n_rxq": "1",
> +            "n_txq": "1",
> +            "numa_id": "0"},
> +          "ofport": "1",
> +          "port_no": "1",
> +          "type": "dummy-pmd"}}},
> +    "stats": {
> +      "n_hit": "0",
> +      "n_missed": "0"}}}]])

Normal command doesn't have 'n_' prefixes.

> +
>  OVS_VSWITCHD_STOP
>  AT_CLEANUP
>