On 2 Apr 2026, at 14:28, Ilya Maximets wrote:
> 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?
Grouping makes sense, than current might not be needed, maybe max should be
maximum?
>>
>>
>> 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
