On 18 Jan 2024, at 16:26, jm...@redhat.com wrote:
> From: Jakob Meng <c...@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 <c...@jakobmeng.de>
Some style comments below.
//Eelco
> ---
> ofproto/ofproto-dpif.c | 129 ++++++++++++++++++++++++++++++++++++-----
> tests/pmd.at | 3 +
> 2 files changed, 119 insertions(+), 13 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 46d6c5014..fe4b8f3e1 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -20,6 +20,7 @@
> #include "bond.h"
> #include "bundle.h"
> #include "byte-order.h"
> +#include "command-line.h"
> #include "connectivity.h"
> #include "connmgr.h"
> #include "coverage.h"
> @@ -6423,8 +6424,103 @@ done:
> return changed;
> }
>
> +static struct json *
> +dpif_show_backer_json(const struct dpif_backer *backer)
> +{
> + struct json *json_backer = json_object_create();
> +
> + /* Add dpif name to JSON. */
> + json_object_put_string(json_backer, "name", dpif_name(backer->dpif));
> +
> + /* Add dpif stats to JSON. */
> + struct dpif_dp_stats dp_stats;
> + dpif_get_dp_stats(backer->dpif, &dp_stats);
> + struct json *json_dp_stats = json_object_create();
Moved the code around a bit, so it's more clear what are declarations vs logic.
/* Add dpif stats to JSON. */
struct dpif_dp_stats dp_stats;
struct json *json_dp_stats = json_object_create();
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_hit", "%"PRIu64,
> dp_stats.n_hit);
> + json_object_put_format(json_dp_stats, "n_missed", "%"PRIu64,
> + dp_stats.n_missed);
> + json_object_put(json_backer, "stats", json_dp_stats);
> +
> + /* Add ofprotos to JSON. */
> + struct json *json_ofprotos = json_array_create_empty();
> + struct shash ofproto_shash;
> + shash_init(&ofproto_shash);
> + const struct shash_node **ofprotos = get_ofprotos(&ofproto_shash);
Maybe re-arrange the code a bit to have a more clear separation between
declarations and logic. I know new compilers are all fine with this
in-line definitions, but it looks more clear if at least they are grouped.
This goes for the entire function.
I'll let Ilya comment on this also, to see what his thoughts on this are.
I can make some more suggestions based on his feedback if needed.
> + for (size_t i = 0; i < shash_count(&ofproto_shash); i++) {
> + struct ofproto_dpif *ofproto = ofprotos[i]->data;
> +
> + if (ofproto->backer != backer) {
> + continue;
> + }
> +
> + struct json *json_ofproto = json_object_create();
> +
> + /* Add ofproto name to JSON array. */
> + json_object_put_string(json_ofproto, "name", ofproto->up.name);
> +
> + /* Add ofproto ports to JSON array. */
> + struct json *json_ofproto_ports = json_array_create_empty();
> + 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++) {
> + const struct shash_node *port = ports[j];
> + struct ofport *ofport = port->data;
> +
> + struct json * json_ofproto_port = json_object_create();
> + /* Add ofproto port netdev name to JSON sub array. */
> + json_object_put_string(json_ofproto_port, "netdev_name",
> + netdev_get_name(ofport->netdev));
> + /* Add ofproto port ofp port to JSON sub array. */
> + json_object_put_format(json_ofproto_port, "ofp_port", "%u",
> + ofport->ofp_port);
> +
> + /* Add ofproto port odp port to JSON sub array. */
> + 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, "odp_port",
> + "%"PRIu32, odp_port);
> + } else {
> + json_object_put_string(json_ofproto_port, "odp_port",
> "none");
> + }
> +
> + /* Add ofproto port netdev type to JSON sub array. */
> + json_object_put_string(json_ofproto_port, "netdev_type",
> + netdev_get_type(ofport->netdev));
> +
> + /* Add ofproto port config to JSON sub array. */
> + struct json *json_port_config = json_object_create();
> + struct smap config;
> + smap_init(&config);
> + 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);
> + json_object_put(json_ofproto_port, "netdev_config",
> + json_port_config);
> +
> + json_array_add(json_ofproto_ports, json_ofproto_port);
> + } /* End of ofproto port(s). */
> +
> + free(ports);
> + json_object_put(json_ofproto, "ports", json_ofproto_ports);
> +
> + json_array_add(json_ofprotos, json_ofproto);
> + } /* End of ofproto(s). */
> + shash_destroy(&ofproto_shash);
> + free(ofprotos);
> +
> + json_object_put(json_backer, "ofprotos", json_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;
> @@ -6490,21 +6586,27 @@ dpif_show_backer(const struct dpif_backer *backer,
> struct ds *ds)
> static void
> ofproto_unixctl_dpif_show(struct unixctl_conn *conn, int argc OVS_UNUSED,
> const char *argv[] OVS_UNUSED,
> - enum ovs_output_fmt fmt OVS_UNUSED,
> - void *aux OVS_UNUSED)
> + enum ovs_output_fmt fmt, void *aux OVS_UNUSED)
> {
> struct ds ds = DS_EMPTY_INITIALIZER;
> - const struct shash_node **backers;
> - int i;
>
> - backers = shash_sort(&all_dpif_backers);
> - for (i = 0; i < shash_count(&all_dpif_backers); i++) {
> - dpif_show_backer(backers[i]->data, &ds);
> - }
> - free(backers);
> + if (fmt == OVS_OUTPUT_FMT_JSON) {
> + struct json *backers = json_array_create_empty();
> + const struct shash_node *backer;
Add a new line between definitions and logic.
> + SHASH_FOR_EACH (backer, &all_dpif_backers) {
> + json_array_add(backers, dpif_show_backer_json(backer->data));
> + }
> + unixctl_command_reply_json(conn, backers);
> + } else {
> + const struct shash_node **backers = shash_sort(&all_dpif_backers);
Add a new line between definitions and logic.
> + for (int i = 0; i < shash_count(&all_dpif_backers); i++) {
> + dpif_show_backer_text(backers[i]->data, &ds);
> + }
> + free(backers);
>
> - unixctl_command_reply(conn, ds_cstr(&ds));
> - ds_destroy(&ds);
> + unixctl_command_reply(conn, ds_cstr(&ds));
> + ds_destroy(&ds);
> + }
> }
>
> static void
> @@ -6685,7 +6787,8 @@ ofproto_unixctl_init(void)
> ofproto_unixctl_mcast_snooping_show, NULL);
> unixctl_command_register("dpif/dump-dps", "", 0, 0, OVS_OUTPUT_FMT_TEXT,
> ofproto_unixctl_dpif_dump_dps, NULL);
> - unixctl_command_register("dpif/show", "", 0, 0, OVS_OUTPUT_FMT_TEXT,
> + unixctl_command_register("dpif/show", "", 0, 0,
> + OVS_OUTPUT_FMT_TEXT | OVS_OUTPUT_FMT_JSON,
> ofproto_unixctl_dpif_show, NULL);
> unixctl_command_register("dpif/show-dp-features", "bridge", 1, 1,
> OVS_OUTPUT_FMT_TEXT,
> diff --git a/tests/pmd.at b/tests/pmd.at
> index 35a44b4df..57660c407 100644
> --- a/tests/pmd.at
> +++ b/tests/pmd.at
> @@ -112,6 +112,9 @@ 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 dpif/show], [0], [dnl
> +[[{"ofprotos":[{"name":"br0","ports":[{"netdev_type":"dummy-internal","ofp_port":"65534","odp_port":"100","netdev_config":{},"netdev_name":"br0"},{"netdev_type":"dummy-pmd","ofp_port":"1","odp_port":"1","netdev_config":{"numa_id":"0","n_txq":"1","n_rxq":"1"},"netdev_name":"p0"}]}],"name":"dummy@ovs-dummy","stats":{"n_hit":"0","n_missed":"0"}}]]])
> +
> OVS_VSWITCHD_STOP
> AT_CLEANUP
>
> --
> 2.39.2
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
