On 13 Mar 2026, at 21:50, Timothy Redaelli via dev wrote:
> When --format json is passed to ovs-appctl, fdb/stats-show returns a
> flat JSON object with the bridge name and all counters from the MAC
> learning table rather than the existing text representation.
>
> Example output:
> {"bridge":"br0","current_entries":17,"max_entries":8192,
> "static_entries":17,"total_expired":0,"total_evicted":0,
> "total_learned":17,"total_moved":0}
>
> Reported-at: https://issues.redhat.com/browse/FDP-2444
> Signed-off-by: Timothy Redaelli <[email protected]>
Hi Timothy,
Thanks for the series. I have a few comments on this patch below.
Cheers,
Eelco
> ---
> ofproto/ofproto-dpif.c | 52 +++++++++++++++++++++++++++++++-----------
> tests/ofproto-dpif.at | 9 ++++++++
> 2 files changed, 48 insertions(+), 13 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index a02afe8ef..add77ca4c 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -6388,7 +6388,6 @@ static void
> ofproto_unixctl_fdb_stats_show(struct unixctl_conn *conn, int argc
> OVS_UNUSED,
> const char *argv[], void *aux OVS_UNUSED)
> {
> - struct ds ds = DS_EMPTY_INITIALIZER;
> const struct ofproto_dpif *ofproto;
> ofproto = ofproto_dpif_lookup_by_name(argv[1]);
> if (!ofproto) {
> @@ -6396,27 +6395,54 @@ ofproto_unixctl_fdb_stats_show(struct unixctl_conn
> *conn, int argc OVS_UNUSED,
> return;
> }
>
> - ds_put_format(&ds, "Statistics for bridge \"%s\":\n", argv[1]);
> ovs_rwlock_rdlock(&ofproto->ml->rwlock);
> + if (unixctl_command_get_output_format(conn) == UNIXCTL_OUTPUT_FMT_JSON) {
The current output looks like this:
{
"bridge": "br0",
"current_entries": 17,
"max_entries": 8192,
"static_entries": 17,
"total_evicted": 0,
"total_expired": 0,
"total_learned": 18,
"total_moved": 0}
Where it should look something like this, this clearly shows the data is
related to br0:
"br0": {
"current_entries": 17,
"max_entries": 8192,
"static_entries": 17,
"total_evicted": 0,
"total_expired": 0,
"total_learned": 18,
"total_moved": 0}}
In addition can we split up the functions, rather than inlining it.
It's cleaner this way. Here's a quick diff:
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index add77ca4c..f62bff605 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -6385,68 +6385,87 @@ ofproto_unixctl_fdb_stats_clear(struct unixctl_conn
*conn, int argc,
}
static void
-ofproto_unixctl_fdb_stats_show(struct unixctl_conn *conn, int argc OVS_UNUSED,
- const char *argv[], void *aux OVS_UNUSED)
+ofproto_unixctl_fdb_stats_text(const struct ofproto_dpif *ofproto,
+ struct ds *ds)
{
- const struct ofproto_dpif *ofproto;
- ofproto = ofproto_dpif_lookup_by_name(argv[1]);
- if (!ofproto) {
- unixctl_command_reply_error(conn, "no such bridge");
- return;
- }
-
ovs_rwlock_rdlock(&ofproto->ml->rwlock);
- if (unixctl_command_get_output_format(conn) == UNIXCTL_OUTPUT_FMT_JSON) {
- struct json *json = json_object_create();
-
- json_object_put_string(json, "bridge", argv[1]);
- json_object_put(json, "current_entries",
- json_integer_create(
- hmap_count(&ofproto->ml->table)));
- json_object_put(json, "max_entries",
- json_integer_create(ofproto->ml->max_entries));
- json_object_put(json, "static_entries",
- json_integer_create(ofproto->ml->static_entries));
- json_object_put(json, "total_learned",
- json_integer_create(ofproto->ml->total_learned));
- json_object_put(json, "total_expired",
- json_integer_create(ofproto->ml->total_expired));
- json_object_put(json, "total_evicted",
- json_integer_create(ofproto->ml->total_evicted));
- json_object_put(json, "total_moved",
- json_integer_create(ofproto->ml->total_moved));
-
- ovs_rwlock_unlock(&ofproto->ml->rwlock);
- unixctl_command_reply_json(conn, json);
- return;
- }
- struct ds ds = DS_EMPTY_INITIALIZER;
-
- ds_put_format(&ds, "Statistics for bridge \"%s\":\n", argv[1]);
- ds_put_format(&ds, " Current/maximum MAC entries in the table: %"
+ ds_put_format(ds, "Statistics for bridge \"%s\":\n", ofproto->up.name);
+ ds_put_format(ds, " Current/maximum MAC entries in the table: %"
PRIuSIZE"/%"PRIuSIZE"\n",
hmap_count(&ofproto->ml->table),
ofproto->ml->max_entries);
- ds_put_format(&ds,
+ ds_put_format(ds,
" Current static MAC entries in the table : %"
PRIuSIZE"\n", ofproto->ml->static_entries);
- ds_put_format(&ds,
+ ds_put_format(ds,
" Total number of learned MAC entries : %"
PRIu64"\n", ofproto->ml->total_learned);
- ds_put_format(&ds,
+ ds_put_format(ds,
" Total number of expired MAC entries : %"
PRIu64"\n", ofproto->ml->total_expired);
- ds_put_format(&ds,
+ ds_put_format(ds,
" Total number of evicted MAC entries : %"
PRIu64"\n", ofproto->ml->total_evicted);
- ds_put_format(&ds,
+ ds_put_format(ds,
" Total number of port moved MAC entries : %"
PRIu64"\n", ofproto->ml->total_moved);
ovs_rwlock_unlock(&ofproto->ml->rwlock);
- unixctl_command_reply(conn, ds_cstr(&ds));
- ds_destroy(&ds);
+}
+
+static void
+ofproto_unixctl_fdb_stats_json(const struct ofproto_dpif *ofproto,
+ struct json *fdb_stats)
+{
+
+ struct json *json = json_object_create();
+
+ json_object_put(fdb_stats, ofproto->up.name, json);
+
+ ovs_rwlock_rdlock(&ofproto->ml->rwlock);
+
+ json_object_put(json, "current_entries",
+ json_integer_create(hmap_count(&ofproto->ml->table)));
+ json_object_put(json, "max_entries",
+ json_integer_create(ofproto->ml->max_entries));
+ json_object_put(json, "static_entries",
+ json_integer_create(ofproto->ml->static_entries));
+ json_object_put(json, "total_learned",
+ json_integer_create(ofproto->ml->total_learned));
+ json_object_put(json, "total_expired",
+ json_integer_create(ofproto->ml->total_expired));
+ json_object_put(json, "total_evicted",
+ json_integer_create(ofproto->ml->total_evicted));
+ json_object_put(json, "total_moved",
+ json_integer_create(ofproto->ml->total_moved));
+
+ ovs_rwlock_unlock(&ofproto->ml->rwlock);
+}
+
+static void
+ofproto_unixctl_fdb_stats_show(struct unixctl_conn *conn, int argc OVS_UNUSED,
+ const char *argv[], void *aux OVS_UNUSED)
+{
+ const struct ofproto_dpif *ofproto;
+ ofproto = ofproto_dpif_lookup_by_name(argv[1]);
+ if (!ofproto) {
+ unixctl_command_reply_error(conn, "no such bridge");
+ return;
+ }
+
+ if (unixctl_command_get_output_format(conn) == UNIXCTL_OUTPUT_FMT_JSON) {
+ struct json *fdb_stats = json_object_create();
+
+ ofproto_unixctl_fdb_stats_json(ofproto, fdb_stats);
+ unixctl_command_reply_json(conn, fdb_stats);
+ } else {
+ struct ds ds = DS_EMPTY_INITIALIZER;
+
+ ofproto_unixctl_fdb_stats_text(ofproto, &ds);
+ unixctl_command_reply(conn, ds_cstr(&ds));
+ ds_destroy(&ds);
+ }
}
static void
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index 9dfdfe29b..b66938212 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -8249,12 +8249,16 @@ AT_CHECK_UNQUOTED([ovs-appctl fdb/stats-show br0 | grep
static], [0], [dnl
])
dnl Check JSON output.
-AT_CHECK([ovs-appctl --format json --pretty fdb/stats-show br0 | dnl
- grep '"bridge"\|entries'], [0], [dnl
- "bridge": "br0",
- "current_entries": 17,
- "max_entries": 8192,
- "static_entries": 17,
+AT_CHECK([ovs-appctl --format json --pretty fdb/stats-show br0], [0], [dnl
+{
+ "br0": {
+ "current_entries": 17,
+ "max_entries": 8192,
+ "static_entries": 17,
+ "total_evicted": 0,
+ "total_expired": 0,
+ "total_learned": 18,
+ "total_moved": 0}}
])
OVS_VSWITCHD_STOP
> + struct json *json = json_object_create();
> +
> + json_object_put_string(json, "bridge", argv[1]);
> + json_object_put(json, "current_entries",
> + json_integer_create(
> + hmap_count(&ofproto->ml->table)));
> + json_object_put(json, "max_entries",
> + json_integer_create(ofproto->ml->max_entries));
> + json_object_put(json, "static_entries",
> + json_integer_create(ofproto->ml->static_entries));
> + json_object_put(json, "total_learned",
> + json_integer_create(ofproto->ml->total_learned));
> + json_object_put(json, "total_expired",
> + json_integer_create(ofproto->ml->total_expired));
> + json_object_put(json, "total_evicted",
> + json_integer_create(ofproto->ml->total_evicted));
> + json_object_put(json, "total_moved",
> + json_integer_create(ofproto->ml->total_moved));
> +
> + ovs_rwlock_unlock(&ofproto->ml->rwlock);
> + unixctl_command_reply_json(conn, json);
> + return;
> + }
>
> + struct ds ds = DS_EMPTY_INITIALIZER;
> +
> + ds_put_format(&ds, "Statistics for bridge \"%s\":\n", argv[1]);
> ds_put_format(&ds, " Current/maximum MAC entries in the table: %"
> PRIuSIZE"/%"PRIuSIZE"\n",
> - hmap_count(&ofproto->ml->table), ofproto->ml->max_entries);
> + hmap_count(&ofproto->ml->table),
> + ofproto->ml->max_entries);
> ds_put_format(&ds,
> - " Current static MAC entries in the table :
> %"PRIuSIZE"\n",
> - ofproto->ml->static_entries);
> + " Current static MAC entries in the table : %"
> + PRIuSIZE"\n", ofproto->ml->static_entries);
> ds_put_format(&ds,
> - " Total number of learned MAC entries : %"PRIu64"\n",
> - ofproto->ml->total_learned);
> + " Total number of learned MAC entries : %"
> + PRIu64"\n", ofproto->ml->total_learned);
> ds_put_format(&ds,
> - " Total number of expired MAC entries : %"PRIu64"\n",
> - ofproto->ml->total_expired);
> + " Total number of expired MAC entries : %"
> + PRIu64"\n", ofproto->ml->total_expired);
> ds_put_format(&ds,
> - " Total number of evicted MAC entries : %"PRIu64"\n",
> - ofproto->ml->total_evicted);
> + " Total number of evicted MAC entries : %"
> + PRIu64"\n", ofproto->ml->total_evicted);
> ds_put_format(&ds,
> - " Total number of port moved MAC entries : %"PRIu64"\n",
> - ofproto->ml->total_moved);
> + " Total number of port moved MAC entries : %"
> + PRIu64"\n", ofproto->ml->total_moved);
>
> ovs_rwlock_unlock(&ofproto->ml->rwlock);
> unixctl_command_reply(conn, ds_cstr(&ds));
> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
> index 39e43d376..9dfdfe29b 100644
> --- a/tests/ofproto-dpif.at
> +++ b/tests/ofproto-dpif.at
> @@ -8248,6 +8248,15 @@ AT_CHECK_UNQUOTED([ovs-appctl fdb/stats-show br0 |
> grep static], [0], [dnl
> Current static MAC entries in the table : 17
> ])
>
> +dnl Check JSON output.
> +AT_CHECK([ovs-appctl --format json --pretty fdb/stats-show br0 | dnl
> + grep '"bridge"\|entries'], [0], [dnl
I would not re-format the output here, just use the raw output of the --pretty
option.
AT_CHECK([ovs-appctl --format json --pretty fdb/stats-show br0], [0], [dnl
{
"br0": {
"current_entries": 17,
"max_entries": 8192,
"static_entries": 17,
"total_evicted": 0,
"total_expired": 0,
"total_learned": 18,
"total_moved": 0}}
])
Looking at the naming, I think static_entries should be renamed to
current_static_entries to make its meaning clearer. Thoughts?
The unfiltered output and split of the show functions should be true for all
other patches also.
> + "bridge": "br0",
> + "current_entries": 17,
> + "max_entries": 8192,
> + "static_entries": 17,
> +])
> +
> OVS_VSWITCHD_STOP
> AT_CLEANUP
>
> --
> 2.53.0
>
> _______________________________________________
> dev mailing list
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
