On 4/2/26 11:19 AM, Eelco Chaudron via dev wrote:
>
>
> 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}
General note for all patches in the set:
1. Don't use underscores in JSON output. Always use dashes.
2. Not applicable to this patch, but in other patches, use full words
whenever possible, i.e. s/pkts/packets/, except for some things
like 'ms' for milliseconds, which are acceptable short names.
There is no point in seving a little bit of space in JSON.
3. All of the patches need a NEW entry.
>>
>> 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?
Not sure about that particular point, but I'd suggest we group the stats
here into two separate blocks:
"br0": {
"entries": {
"current": 17,
"max": 8192,
"static": 17
},
"events": {
"evicted": 0,
"expired": 0,
"learned": 18,
"moved": 0
}
}
WDYT?
>
>
> 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
