On 17 Jun 2026, at 22:46, Timothy Redaelli via dev wrote:
> When --format json is passed to ovs-appctl, fdb/stats-show returns a
> JSON object keyed by bridge name. Each bridge contains two sub-objects:
> "entries" (with "current", "maximum", and "static" counts) and "events"
> (with "evicted", "expired", "learned", and "moved" counters).
>
> The implementation is split into separate text and JSON helper functions
> to keep the main handler simple.
>
> Example output:
> {"br0": {"entries": {"current": 17, "maximum": 8192, "static": 17},
> "events": {"evicted": 0, "expired": 0,
> "learned": 17, "moved": 0}}}
>
> Reported-at: https://issues.redhat.com/browse/FDP-2444
> Signed-off-by: Timothy Redaelli <[email protected]>
Thanks Timothy for sending out the v3. I have some small comments below.
Cheers,
Eelco
> ---
> ofproto/ofproto-dpif.c | 87 ++++++++++++++++++++++++++++++++----------
> tests/ofproto-dpif.at | 15 ++++++++
> 2 files changed, 81 insertions(+), 21 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index c40da62cf..d6f07a220 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -6369,11 +6369,67 @@ ofproto_unixctl_fdb_stats_clear(struct unixctl_conn
> *conn, int argc,
> unixctl_command_reply(conn, "statistics successfully cleared");
> }
>
> +static struct json *
> +ofproto_unixctl_fdb_stats_show_json(const struct ofproto_dpif *ofproto,
> + const char *bridge_name)
We do not need to pass in the bridge_name, is also here: ofproto->up.name
> +{
> + struct json *entries = json_object_create();
> + struct json *events = json_object_create();
> + struct json *bridge = json_object_create();
> + struct json *json = json_object_create();
> +
I would add the lock here.
> + json_object_put(entries, "current",
> + json_integer_create(hmap_count(&ofproto->ml->table)));
> + json_object_put(entries, "maximum",
> + json_integer_create(ofproto->ml->max_entries));
> + json_object_put(entries, "static",
> + json_integer_create(ofproto->ml->static_entries));
> + json_object_put(bridge, "entries", entries);
> +
> + json_object_put(events, "evicted",
> + json_integer_create(ofproto->ml->total_evicted));
> + json_object_put(events, "expired",
> + json_integer_create(ofproto->ml->total_expired));
> + json_object_put(events, "learned",
> + json_integer_create(ofproto->ml->total_learned));
> + json_object_put(events, "moved",
> + json_integer_create(ofproto->ml->total_moved));
> + json_object_put(bridge, "events", events);
> +
> + json_object_put(json, bridge_name, bridge);
Unlock here.
> + return json;
> +}
> +
> +static void
> +ofproto_unixctl_fdb_stats_show_text(const struct ofproto_dpif *ofproto,
> + const char *bridge_name, struct ds *ds)
See above about bridge name.
> +{
I would add the lock here.
> + ds_put_format(ds, "Statistics for bridge \"%s\":\n", bridge_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,
> + " 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);
> + ds_put_format(ds,
> + " 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);
> + ds_put_format(ds,
> + " Total number of port moved MAC entries : %"
> + PRIu64"\n", ofproto->ml->total_moved);
Unlock here.
> +}
> +
> 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;
Can you add a new line here?
We should have one between the declarations and the actual code.
> ofproto = ofproto_dpif_lookup_by_name(argv[1]);
> if (!ofproto) {
> @@ -6381,28 +6437,17 @@ 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) {
> + struct json *json = ofproto_unixctl_fdb_stats_show_json(ofproto,
> + argv[1]);
> + ovs_rwlock_unlock(&ofproto->ml->rwlock);
> + unixctl_command_reply_json(conn, json);
> + return;
> + }
A single unlock might look better, something like:
ovs_rwlock_rdlock(&ofproto->ml->rwlock);
if (unixctl_command_get_output_format(conn) == UNIXCTL_OUTPUT_FMT_JSON) {
struct json *json = ofproto_unixctl_fdb_stats_show_json(ofproto,
argv[1]);
unixctl_command_reply_json(conn, json);
} else {
struct ds ds = DS_EMPTY_INITIALIZER;
ofproto_unixctl_fdb_stats_show_text(ofproto, argv[1], &ds);
unixctl_command_reply(conn, ds_cstr(&ds));
ds_destroy(&ds);
}
ovs_rwlock_unlock(&ofproto->ml->rwlock);
In general, I would move the locking to the specific show function, but keep
the if/else here.
> - 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,
> - " 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);
> - ds_put_format(&ds,
> - " 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);
> - ds_put_format(&ds,
> - " Total number of port moved MAC entries : %"PRIu64"\n",
> - ofproto->ml->total_moved);
> -
> + struct ds ds = DS_EMPTY_INITIALIZER;
New line.
> + ofproto_unixctl_fdb_stats_show_text(ofproto, argv[1], &ds);
> ovs_rwlock_unlock(&ofproto->ml->rwlock);
> unixctl_command_reply(conn, ds_cstr(&ds));
> ds_destroy(&ds);
> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
> index 9e64e8ee4..b5df060b0 100644
> --- a/tests/ofproto-dpif.at
> +++ b/tests/ofproto-dpif.at
> @@ -8247,6 +8247,21 @@ 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], [0], [dnl
> +{
> + "br0": {
> + "entries": {
> + "current": 17,
> + "maximum": 8192,
> + "static": 17},
> + "events": {
> + "evicted": 0,
> + "expired": 0,
> + "learned": 18,
> + "moved": 0}}}
> +])
> +
> OVS_VSWITCHD_STOP
> AT_CLEANUP
>
> --
> 2.54.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
