[ovs-dev,v3] ofproto: Add JSON output for 'fdb/show' command.

From: Dima Chumak <dchumak@nvidia.com>

The 'fdb/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 --pretty fdb/show br-phy
  [
    {
      "age": "static",
      "mac": "e4:8c:07:08:00:02",
      "port": 2,
      "vlan": 0
    },
    {
      "age": 14,
      "mac": "e4:8c:07:08:00:03",
      "port": 3,
      "vlan": 0
    }
  ]

Signed-off-by: Dima Chumak <dchumak@nvidia.com>
Acked-by: Roi Dayan <roid@nvidia.com>
---

Notes:
    v3
    - Fix assert. No need to call json_destroy() after unixctl_command_reply_json() as it takes
      ownership of the allocated body.
    
    v2
    - Fix sparse check error.
    - Add case checking fdb/show json output.

 ofproto/ofproto-dpif.c | 85 ++++++++++++++++++++++++++++++++++--------
 tests/ofproto-dpif.at  | 10 +++++
 2 files changed, 79 insertions(+), 16 deletions(-)

On 18 Feb 2025, at 12:03, Roi Dayan wrote:

> From: Dima Chumak <dchumak@nvidia.com>
>
> The 'fdb/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 --pretty fdb/show br-phy
>   [
>     {
>       "age": "static",
>       "mac": "e4:8c:07:08:00:02",
>       "port": 2,
>       "vlan": 0
>     },
>     {
>       "age": 14,
>       "mac": "e4:8c:07:08:00:03",
>       "port": 3,
>       "vlan": 0
>     }
>   ]
>
> Signed-off-by: Dima Chumak <dchumak@nvidia.com>
> Acked-by: Roi Dayan <roid@nvidia.com>
> ---
>
> Notes:
>     v3
>     - Fix assert. No need to call json_destroy() after unixctl_command_reply_json() as it takes
>       ownership of the allocated body.
>
>     v2
>     - Fix sparse check error.
>     - Add case checking fdb/show json output.
>
>  ofproto/ofproto-dpif.c | 85 ++++++++++++++++++++++++++++++++++--------
>  tests/ofproto-dpif.at  | 10 +++++
>  2 files changed, 79 insertions(+), 16 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index bf43d5d4bc59..9926c016fb00 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -6151,20 +6151,12 @@ ofbundle_get_a_port(const struct ofbundle *bundle)
>  }
>
>  static void
> -ofproto_unixctl_fdb_show(struct unixctl_conn *conn, int argc OVS_UNUSED,
> -                         const char *argv[], void *aux OVS_UNUSED)
> +ofproto_unixctl_fdb_show_text(const struct ofproto_dpif *ofproto,
> +                              struct ds *ds)
>  {
> -    struct ds ds = DS_EMPTY_INITIALIZER;
> -    const struct ofproto_dpif *ofproto;
>      const struct mac_entry *e;
>
> -    ofproto = ofproto_dpif_lookup_by_name(argv[1]);
> -    if (!ofproto) {
> -        unixctl_command_reply_error(conn, "no such bridge");
> -        return;
> -    }
> -
> -    ds_put_cstr(&ds, " port  VLAN  MAC                Age\n");
> +    ds_put_cstr(ds, " port  VLAN  MAC                Age\n");
>      ovs_rwlock_rdlock(&ofproto->ml->rwlock);
>      LIST_FOR_EACH (e, lru_node, &ofproto->ml->lrus) {
>          struct ofbundle *bundle = mac_entry_get_port(ofproto->ml, e);
> @@ -6173,17 +6165,78 @@ ofproto_unixctl_fdb_show(struct unixctl_conn *conn, int argc OVS_UNUSED,
>
>          ofputil_port_to_string(ofbundle_get_a_port(bundle)->up.ofp_port,
>                  NULL, name, sizeof name);
> -        ds_put_format(&ds, "%5s  %4d  "ETH_ADDR_FMT"  ",
> +        ds_put_format(ds, "%5s  %4d  "ETH_ADDR_FMT"  ",
>                  name, e->vlan, ETH_ADDR_ARGS(e->mac));
>          if (MAC_ENTRY_AGE_STATIC_ENTRY == age) {
> -            ds_put_format(&ds, "static\n");
> +            ds_put_format(ds, "static\n");
>          } else {
> -            ds_put_format(&ds, "%3d\n", age);
> +            ds_put_format(ds, "%3d\n", age);
>          }
>      }
>      ovs_rwlock_unlock(&ofproto->ml->rwlock);
> -    unixctl_command_reply(conn, ds_cstr(&ds));
> -    ds_destroy(&ds);
> +}
> +
> +static void
> +ofproto_unixctl_fdb_show_json(const struct ofproto_dpif *ofproto,
> +                              struct json **fdb_entries)
> +{
> +    size_t num_entries = hmap_count(&ofproto->ml->table);

This needs to happen under the rw lock, or else entries could be deleted (added).

> +    struct json **json_entries = NULL;
> +    const struct mac_entry *entry;
> +    int i = 0;
> +
> +    if (!num_entries) {
> +        goto done;
> +    }
> +
> +    json_entries = xmalloc(num_entries * sizeof *json_entries);
> +    ovs_rwlock_rdlock(&ofproto->ml->rwlock);
> +    LIST_FOR_EACH (entry, lru_node, &ofproto->ml->lrus) {
> +        struct ofbundle *bundle = mac_entry_get_port(ofproto->ml, entry);
> +        struct json *json_entry = json_object_create();
> +        int age = mac_entry_age(ofproto->ml, entry);
> +        long long port;
> +
> +        port = (OVS_FORCE long long) ofbundle_get_a_port(bundle)->up.ofp_port;
> +        json_object_put(json_entry, "port", json_integer_create(port));
> +        json_object_put(json_entry, "vlan", json_integer_create(entry->vlan));
> +        json_object_put_format(json_entry, "mac", ETH_ADDR_FMT,
> +                               ETH_ADDR_ARGS(entry->mac));
> +        if (MAC_ENTRY_AGE_STATIC_ENTRY == age) {
> +            json_object_put_string(json_entry, "age", "static");
> +        } else {
> +            json_object_put(json_entry, "age", json_integer_create(age));
> +        }
> +        json_entries[i++] = json_entry;

Add a safety check here;
	if (i >= json_entries) {
		break;
    }

or an ovs_assert() at the beginning of the loop.

> +    }
> +    ovs_rwlock_unlock(&ofproto->ml->rwlock);
> +done:
> +    *fdb_entries = json_array_create(json_entries, num_entries);

For safety reasons, I would use ‘i’ here instead of ‘num_entries’ to make sure we have no invalid pointers if, for some reason, the number of entries added is less than the number initially calculated.

> +}
> +
> +static void
> +ofproto_unixctl_fdb_show(struct unixctl_conn *conn, int argc OVS_UNUSED,
> +                          const char *argv[] OVS_UNUSED, void *aux OVS_UNUSED)
> +{
> +    const struct ofproto_dpif *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_entries;
> +
> +        ofproto_unixctl_fdb_show_json(ofproto, &fdb_entries);
> +        unixctl_command_reply_json(conn, fdb_entries);
> +    } else {
> +        struct ds ds = DS_EMPTY_INITIALIZER;
> +
> +        ofproto_unixctl_fdb_show_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 fa5f148b4c28..8a975e6ffd9c 100644
> --- a/tests/ofproto-dpif.at
> +++ b/tests/ofproto-dpif.at
> @@ -7085,6 +7085,16 @@ AT_CHECK_UNQUOTED([ovs-appctl fdb/show br0 | sed 's/[[0-9]]\{1,\}$/?/'], [0], [d
>      3     0  50:54:00:00:00:05    ?
>  ])
>
> +dnl Check json output.
> +AT_CHECK([ovs-appctl --format json --pretty fdb/show br0], [0], [dnl
> +[[
> +  {
> +    "age": 0,
> +    "mac": "50:54:00:00:00:05",
> +    "port": 3,
> +    "vlan": 0}]]
> +])
> +

If you run the tests enough times, some times the age is 1, and the test fails. In addition I think we should move this to a place where we have at least two entries.
For example:

@@ -7110,6 +7100,22 @@ AT_CHECK_UNQUOTED([ovs-appctl fdb/show br0 | sed 's/[[0-9]]\{1,\}$/?/'], [0], [d
     1     0  50:54:00:00:00:06    ?
 ])

+dnl Check json output.
+AT_CHECK([ovs-appctl --format json --pretty fdb/show br0 \
+          | sed 's/"age": [[0-9]]\+/"age": ?/g'], [0], [dnl
+[[
+  {
+    "age": ?,
+    "mac": "50:54:00:00:00:05",
+    "port": 3,
+    "vlan": 0},
+  {
+    "age": ?,
+    "mac": "50:54:00:00:00:06",
+    "port": 1,
+    "vlan": 0}]]
+])
+

>  # Trace a packet arrival destined for the learned MAC.
>  # (This will also learn a MAC.)
>  OFPROTO_TRACE(
> -- 
> 2.21.0

On 18/02/2025 14:51, Eelco Chaudron wrote:
> 
> 
> On 18 Feb 2025, at 12:03, Roi Dayan wrote:
> 
>> From: Dima Chumak <dchumak@nvidia.com>
>>
>> The 'fdb/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 --pretty fdb/show br-phy
>>   [
>>     {
>>       "age": "static",
>>       "mac": "e4:8c:07:08:00:02",
>>       "port": 2,
>>       "vlan": 0
>>     },
>>     {
>>       "age": 14,
>>       "mac": "e4:8c:07:08:00:03",
>>       "port": 3,
>>       "vlan": 0
>>     }
>>   ]
>>
>> Signed-off-by: Dima Chumak <dchumak@nvidia.com>
>> Acked-by: Roi Dayan <roid@nvidia.com>
>> ---
>>
>> Notes:
>>     v3
>>     - Fix assert. No need to call json_destroy() after unixctl_command_reply_json() as it takes
>>       ownership of the allocated body.
>>
>>     v2
>>     - Fix sparse check error.
>>     - Add case checking fdb/show json output.
>>
>>  ofproto/ofproto-dpif.c | 85 ++++++++++++++++++++++++++++++++++--------
>>  tests/ofproto-dpif.at  | 10 +++++
>>  2 files changed, 79 insertions(+), 16 deletions(-)
>>
>> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
>> index bf43d5d4bc59..9926c016fb00 100644
>> --- a/ofproto/ofproto-dpif.c
>> +++ b/ofproto/ofproto-dpif.c
>> @@ -6151,20 +6151,12 @@ ofbundle_get_a_port(const struct ofbundle *bundle)
>>  }
>>
>>  static void
>> -ofproto_unixctl_fdb_show(struct unixctl_conn *conn, int argc OVS_UNUSED,
>> -                         const char *argv[], void *aux OVS_UNUSED)
>> +ofproto_unixctl_fdb_show_text(const struct ofproto_dpif *ofproto,
>> +                              struct ds *ds)
>>  {
>> -    struct ds ds = DS_EMPTY_INITIALIZER;
>> -    const struct ofproto_dpif *ofproto;
>>      const struct mac_entry *e;
>>
>> -    ofproto = ofproto_dpif_lookup_by_name(argv[1]);
>> -    if (!ofproto) {
>> -        unixctl_command_reply_error(conn, "no such bridge");
>> -        return;
>> -    }
>> -
>> -    ds_put_cstr(&ds, " port  VLAN  MAC                Age\n");
>> +    ds_put_cstr(ds, " port  VLAN  MAC                Age\n");
>>      ovs_rwlock_rdlock(&ofproto->ml->rwlock);
>>      LIST_FOR_EACH (e, lru_node, &ofproto->ml->lrus) {
>>          struct ofbundle *bundle = mac_entry_get_port(ofproto->ml, e);
>> @@ -6173,17 +6165,78 @@ ofproto_unixctl_fdb_show(struct unixctl_conn *conn, int argc OVS_UNUSED,
>>
>>          ofputil_port_to_string(ofbundle_get_a_port(bundle)->up.ofp_port,
>>                  NULL, name, sizeof name);
>> -        ds_put_format(&ds, "%5s  %4d  "ETH_ADDR_FMT"  ",
>> +        ds_put_format(ds, "%5s  %4d  "ETH_ADDR_FMT"  ",
>>                  name, e->vlan, ETH_ADDR_ARGS(e->mac));
>>          if (MAC_ENTRY_AGE_STATIC_ENTRY == age) {
>> -            ds_put_format(&ds, "static\n");
>> +            ds_put_format(ds, "static\n");
>>          } else {
>> -            ds_put_format(&ds, "%3d\n", age);
>> +            ds_put_format(ds, "%3d\n", age);
>>          }
>>      }
>>      ovs_rwlock_unlock(&ofproto->ml->rwlock);
>> -    unixctl_command_reply(conn, ds_cstr(&ds));
>> -    ds_destroy(&ds);
>> +}
>> +
>> +static void
>> +ofproto_unixctl_fdb_show_json(const struct ofproto_dpif *ofproto,
>> +                              struct json **fdb_entries)
>> +{
>> +    size_t num_entries = hmap_count(&ofproto->ml->table);
> 
> This needs to happen under the rw lock, or else entries could be deleted (added).

you mean the entire thing to be under rwlock instead of rdlock ?
but if we add what you suggested below with checking 'i' then isn't
it ok now? we fill the array with mini between 'i' and num_entries.

> 
>> +    struct json **json_entries = NULL;
>> +    const struct mac_entry *entry;
>> +    int i = 0;
>> +
>> +    if (!num_entries) {
>> +        goto done;
>> +    }
>> +
>> +    json_entries = xmalloc(num_entries * sizeof *json_entries);
>> +    ovs_rwlock_rdlock(&ofproto->ml->rwlock);
>> +    LIST_FOR_EACH (entry, lru_node, &ofproto->ml->lrus) {
>> +        struct ofbundle *bundle = mac_entry_get_port(ofproto->ml, entry);
>> +        struct json *json_entry = json_object_create();
>> +        int age = mac_entry_age(ofproto->ml, entry);
>> +        long long port;
>> +
>> +        port = (OVS_FORCE long long) ofbundle_get_a_port(bundle)->up.ofp_port;
>> +        json_object_put(json_entry, "port", json_integer_create(port));
>> +        json_object_put(json_entry, "vlan", json_integer_create(entry->vlan));
>> +        json_object_put_format(json_entry, "mac", ETH_ADDR_FMT,
>> +                               ETH_ADDR_ARGS(entry->mac));
>> +        if (MAC_ENTRY_AGE_STATIC_ENTRY == age) {
>> +            json_object_put_string(json_entry, "age", "static");
>> +        } else {
>> +            json_object_put(json_entry, "age", json_integer_create(age));
>> +        }
>> +        json_entries[i++] = json_entry;
> 
> Add a safety check here;
> 	if (i >= json_entries) {
> 		break;
>     }
> 

added. prefered than hitting an assert just for showing some result.

> or an ovs_assert() at the beginning of the loop.
> 
>> +    }
>> +    ovs_rwlock_unlock(&ofproto->ml->rwlock);
>> +done:
>> +    *fdb_entries = json_array_create(json_entries, num_entries);
> 
> For safety reasons, I would use ‘i’ here instead of ‘num_entries’ to make sure we have no invalid pointers if, for some reason, the number of entries added is less than the number initially calculated.
> 

updated.

>> +}
>> +
>> +static void
>> +ofproto_unixctl_fdb_show(struct unixctl_conn *conn, int argc OVS_UNUSED,
>> +                          const char *argv[] OVS_UNUSED, void *aux OVS_UNUSED)
>> +{
>> +    const struct ofproto_dpif *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_entries;
>> +
>> +        ofproto_unixctl_fdb_show_json(ofproto, &fdb_entries);
>> +        unixctl_command_reply_json(conn, fdb_entries);
>> +    } else {
>> +        struct ds ds = DS_EMPTY_INITIALIZER;
>> +
>> +        ofproto_unixctl_fdb_show_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 fa5f148b4c28..8a975e6ffd9c 100644
>> --- a/tests/ofproto-dpif.at
>> +++ b/tests/ofproto-dpif.at
>> @@ -7085,6 +7085,16 @@ AT_CHECK_UNQUOTED([ovs-appctl fdb/show br0 | sed 's/[[0-9]]\{1,\}$/?/'], [0], [d
>>      3     0  50:54:00:00:00:05    ?
>>  ])
>>
>> +dnl Check json output.
>> +AT_CHECK([ovs-appctl --format json --pretty fdb/show br0], [0], [dnl
>> +[[
>> +  {
>> +    "age": 0,
>> +    "mac": "50:54:00:00:00:05",
>> +    "port": 3,
>> +    "vlan": 0}]]
>> +])
>> +
> 
> If you run the tests enough times, some times the age is 1, and the test fails. In addition I think we should move this to a place where we have at least two entries.
> For example:
> 
> @@ -7110,6 +7100,22 @@ AT_CHECK_UNQUOTED([ovs-appctl fdb/show br0 | sed 's/[[0-9]]\{1,\}$/?/'], [0], [d
>      1     0  50:54:00:00:00:06    ?
>  ])
> 
> +dnl Check json output.
> +AT_CHECK([ovs-appctl --format json --pretty fdb/show br0 \
> +          | sed 's/"age": [[0-9]]\+/"age": ?/g'], [0], [dnl
> +[[
> +  {
> +    "age": ?,
> +    "mac": "50:54:00:00:00:05",
> +    "port": 3,
> +    "vlan": 0},
> +  {
> +    "age": ?,
> +    "mac": "50:54:00:00:00:06",
> +    "port": 1,
> +    "vlan": 0}]]
> +])
> +
> 

ok updated.

>>  # Trace a packet arrival destined for the learned MAC.
>>  # (This will also learn a MAC.)
>>  OFPROTO_TRACE(
>> -- 
>> 2.21.0
>

On 20 Feb 2025, at 10:55, Roi Dayan wrote:

> On 18/02/2025 14:51, Eelco Chaudron wrote:
>>
>>
>> On 18 Feb 2025, at 12:03, Roi Dayan wrote:
>>
>>> From: Dima Chumak <dchumak@nvidia.com>
>>>
>>> The 'fdb/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 --pretty fdb/show br-phy
>>>   [
>>>     {
>>>       "age": "static",
>>>       "mac": "e4:8c:07:08:00:02",
>>>       "port": 2,
>>>       "vlan": 0
>>>     },
>>>     {
>>>       "age": 14,
>>>       "mac": "e4:8c:07:08:00:03",
>>>       "port": 3,
>>>       "vlan": 0
>>>     }
>>>   ]
>>>
>>> Signed-off-by: Dima Chumak <dchumak@nvidia.com>
>>> Acked-by: Roi Dayan <roid@nvidia.com>
>>> ---
>>>
>>> Notes:
>>>     v3
>>>     - Fix assert. No need to call json_destroy() after unixctl_command_reply_json() as it takes
>>>       ownership of the allocated body.
>>>
>>>     v2
>>>     - Fix sparse check error.
>>>     - Add case checking fdb/show json output.
>>>
>>>  ofproto/ofproto-dpif.c | 85 ++++++++++++++++++++++++++++++++++--------
>>>  tests/ofproto-dpif.at  | 10 +++++
>>>  2 files changed, 79 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
>>> index bf43d5d4bc59..9926c016fb00 100644
>>> --- a/ofproto/ofproto-dpif.c
>>> +++ b/ofproto/ofproto-dpif.c
>>> @@ -6151,20 +6151,12 @@ ofbundle_get_a_port(const struct ofbundle *bundle)
>>>  }
>>>
>>>  static void
>>> -ofproto_unixctl_fdb_show(struct unixctl_conn *conn, int argc OVS_UNUSED,
>>> -                         const char *argv[], void *aux OVS_UNUSED)
>>> +ofproto_unixctl_fdb_show_text(const struct ofproto_dpif *ofproto,
>>> +                              struct ds *ds)
>>>  {
>>> -    struct ds ds = DS_EMPTY_INITIALIZER;
>>> -    const struct ofproto_dpif *ofproto;
>>>      const struct mac_entry *e;
>>>
>>> -    ofproto = ofproto_dpif_lookup_by_name(argv[1]);
>>> -    if (!ofproto) {
>>> -        unixctl_command_reply_error(conn, "no such bridge");
>>> -        return;
>>> -    }
>>> -
>>> -    ds_put_cstr(&ds, " port  VLAN  MAC                Age\n");
>>> +    ds_put_cstr(ds, " port  VLAN  MAC                Age\n");
>>>      ovs_rwlock_rdlock(&ofproto->ml->rwlock);
>>>      LIST_FOR_EACH (e, lru_node, &ofproto->ml->lrus) {
>>>          struct ofbundle *bundle = mac_entry_get_port(ofproto->ml, e);
>>> @@ -6173,17 +6165,78 @@ ofproto_unixctl_fdb_show(struct unixctl_conn *conn, int argc OVS_UNUSED,
>>>
>>>          ofputil_port_to_string(ofbundle_get_a_port(bundle)->up.ofp_port,
>>>                  NULL, name, sizeof name);
>>> -        ds_put_format(&ds, "%5s  %4d  "ETH_ADDR_FMT"  ",
>>> +        ds_put_format(ds, "%5s  %4d  "ETH_ADDR_FMT"  ",
>>>                  name, e->vlan, ETH_ADDR_ARGS(e->mac));
>>>          if (MAC_ENTRY_AGE_STATIC_ENTRY == age) {
>>> -            ds_put_format(&ds, "static\n");
>>> +            ds_put_format(ds, "static\n");
>>>          } else {
>>> -            ds_put_format(&ds, "%3d\n", age);
>>> +            ds_put_format(ds, "%3d\n", age);
>>>          }
>>>      }
>>>      ovs_rwlock_unlock(&ofproto->ml->rwlock);
>>> -    unixctl_command_reply(conn, ds_cstr(&ds));
>>> -    ds_destroy(&ds);
>>> +}
>>> +
>>> +static void
>>> +ofproto_unixctl_fdb_show_json(const struct ofproto_dpif *ofproto,
>>> +                              struct json **fdb_entries)
>>> +{
>>> +    size_t num_entries = hmap_count(&ofproto->ml->table);
>>
>> This needs to happen under the rw lock, or else entries could be deleted (added).
>
> you mean the entire thing to be under rwlock instead of rdlock ?
> but if we add what you suggested below with checking 'i' then isn't
> it ok now? we fill the array with mini between 'i' and num_entries.

Sorry, yes the entire thing between the read lock. The hmap_count() should be under the read lock anyway.
And if we assert, as suggested below, it should not be unlocked in between.

>>
>>> +    struct json **json_entries = NULL;
>>> +    const struct mac_entry *entry;
>>> +    int i = 0;
>>> +
>>> +    if (!num_entries) {
>>> +        goto done;
>>> +    }
>>> +
>>> +    json_entries = xmalloc(num_entries * sizeof *json_entries);
>>> +    ovs_rwlock_rdlock(&ofproto->ml->rwlock);
>>> +    LIST_FOR_EACH (entry, lru_node, &ofproto->ml->lrus) {
>>> +        struct ofbundle *bundle = mac_entry_get_port(ofproto->ml, entry);
>>> +        struct json *json_entry = json_object_create();
>>> +        int age = mac_entry_age(ofproto->ml, entry);
>>> +        long long port;
>>> +
>>> +        port = (OVS_FORCE long long) ofbundle_get_a_port(bundle)->up.ofp_port;
>>> +        json_object_put(json_entry, "port", json_integer_create(port));
>>> +        json_object_put(json_entry, "vlan", json_integer_create(entry->vlan));
>>> +        json_object_put_format(json_entry, "mac", ETH_ADDR_FMT,
>>> +                               ETH_ADDR_ARGS(entry->mac));
>>> +        if (MAC_ENTRY_AGE_STATIC_ENTRY == age) {
>>> +            json_object_put_string(json_entry, "age", "static");
>>> +        } else {
>>> +            json_object_put(json_entry, "age", json_integer_create(age));
>>> +        }
>>> +        json_entries[i++] = json_entry;
>>
>> Add a safety check here;
>> 	if (i >= json_entries) {
>> 		break;
>>     }
>>
>
> added. prefered than hitting an assert just for showing some result.
>
>> or an ovs_assert() at the beginning of the loop.
>>
>>> +    }
>>> +    ovs_rwlock_unlock(&ofproto->ml->rwlock);
>>> +done:
>>> +    *fdb_entries = json_array_create(json_entries, num_entries);
>>
>> For safety reasons, I would use ‘i’ here instead of ‘num_entries’ to make sure we have no invalid pointers if, for some reason, the number of entries added is less than the number initially calculated.
>>
>
> updated.
>
>>> +}
>>> +
>>> +static void
>>> +ofproto_unixctl_fdb_show(struct unixctl_conn *conn, int argc OVS_UNUSED,
>>> +                          const char *argv[] OVS_UNUSED, void *aux OVS_UNUSED)
>>> +{
>>> +    const struct ofproto_dpif *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_entries;
>>> +
>>> +        ofproto_unixctl_fdb_show_json(ofproto, &fdb_entries);
>>> +        unixctl_command_reply_json(conn, fdb_entries);
>>> +    } else {
>>> +        struct ds ds = DS_EMPTY_INITIALIZER;
>>> +
>>> +        ofproto_unixctl_fdb_show_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 fa5f148b4c28..8a975e6ffd9c 100644
>>> --- a/tests/ofproto-dpif.at
>>> +++ b/tests/ofproto-dpif.at
>>> @@ -7085,6 +7085,16 @@ AT_CHECK_UNQUOTED([ovs-appctl fdb/show br0 | sed 's/[[0-9]]\{1,\}$/?/'], [0], [d
>>>      3     0  50:54:00:00:00:05    ?
>>>  ])
>>>
>>> +dnl Check json output.
>>> +AT_CHECK([ovs-appctl --format json --pretty fdb/show br0], [0], [dnl
>>> +[[
>>> +  {
>>> +    "age": 0,
>>> +    "mac": "50:54:00:00:00:05",
>>> +    "port": 3,
>>> +    "vlan": 0}]]
>>> +])
>>> +
>>
>> If you run the tests enough times, some times the age is 1, and the test fails. In addition I think we should move this to a place where we have at least two entries.
>> For example:
>>
>> @@ -7110,6 +7100,22 @@ AT_CHECK_UNQUOTED([ovs-appctl fdb/show br0 | sed 's/[[0-9]]\{1,\}$/?/'], [0], [d
>>      1     0  50:54:00:00:00:06    ?
>>  ])
>>
>> +dnl Check json output.
>> +AT_CHECK([ovs-appctl --format json --pretty fdb/show br0 \
>> +          | sed 's/"age": [[0-9]]\+/"age": ?/g'], [0], [dnl
>> +[[
>> +  {
>> +    "age": ?,
>> +    "mac": "50:54:00:00:00:05",
>> +    "port": 3,
>> +    "vlan": 0},
>> +  {
>> +    "age": ?,
>> +    "mac": "50:54:00:00:00:06",
>> +    "port": 1,
>> +    "vlan": 0}]]
>> +])
>> +
>>
>
> ok updated.
>
>>>  # Trace a packet arrival destined for the learned MAC.
>>>  # (This will also learn a MAC.)
>>>  OFPROTO_TRACE(
>>> -- 
>>> 2.21.0
>>

On 2/18/25 12:03, Roi Dayan wrote:
> From: Dima Chumak <dchumak@nvidia.com>
> 
> The 'fdb/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 --pretty fdb/show br-phy
>   [
>     {
>       "age": "static",
>       "mac": "e4:8c:07:08:00:02",
>       "port": 2,
>       "vlan": 0
>     },
>     {
>       "age": 14,
>       "mac": "e4:8c:07:08:00:03",
>       "port": 3,
>       "vlan": 0
>     }
>   ]

Instead of returning different type values for the age, should we
perhaps return "static" as a separate boolean field and not report
the "age" in this case?

Alternative might be to report age as a string always.  But that
doesn't sound particularly great.

It may be very inconvenient to write parsers in some languages if
the type of the value can be both int and the string.

Best regards, Ilya Maximets.

On 20/02/2025 12:04, Eelco Chaudron wrote:
> 
> 
> On 20 Feb 2025, at 10:55, Roi Dayan wrote:
> 
>> On 18/02/2025 14:51, Eelco Chaudron wrote:
>>>
>>>
>>> On 18 Feb 2025, at 12:03, Roi Dayan wrote:
>>>
>>>> From: Dima Chumak <dchumak@nvidia.com>
>>>>
>>>> The 'fdb/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 --pretty fdb/show br-phy
>>>>   [
>>>>     {
>>>>       "age": "static",
>>>>       "mac": "e4:8c:07:08:00:02",
>>>>       "port": 2,
>>>>       "vlan": 0
>>>>     },
>>>>     {
>>>>       "age": 14,
>>>>       "mac": "e4:8c:07:08:00:03",
>>>>       "port": 3,
>>>>       "vlan": 0
>>>>     }
>>>>   ]
>>>>
>>>> Signed-off-by: Dima Chumak <dchumak@nvidia.com>
>>>> Acked-by: Roi Dayan <roid@nvidia.com>
>>>> ---
>>>>
>>>> Notes:
>>>>     v3
>>>>     - Fix assert. No need to call json_destroy() after unixctl_command_reply_json() as it takes
>>>>       ownership of the allocated body.
>>>>
>>>>     v2
>>>>     - Fix sparse check error.
>>>>     - Add case checking fdb/show json output.
>>>>
>>>>  ofproto/ofproto-dpif.c | 85 ++++++++++++++++++++++++++++++++++--------
>>>>  tests/ofproto-dpif.at  | 10 +++++
>>>>  2 files changed, 79 insertions(+), 16 deletions(-)
>>>>
>>>> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
>>>> index bf43d5d4bc59..9926c016fb00 100644
>>>> --- a/ofproto/ofproto-dpif.c
>>>> +++ b/ofproto/ofproto-dpif.c
>>>> @@ -6151,20 +6151,12 @@ ofbundle_get_a_port(const struct ofbundle *bundle)
>>>>  }
>>>>
>>>>  static void
>>>> -ofproto_unixctl_fdb_show(struct unixctl_conn *conn, int argc OVS_UNUSED,
>>>> -                         const char *argv[], void *aux OVS_UNUSED)
>>>> +ofproto_unixctl_fdb_show_text(const struct ofproto_dpif *ofproto,
>>>> +                              struct ds *ds)
>>>>  {
>>>> -    struct ds ds = DS_EMPTY_INITIALIZER;
>>>> -    const struct ofproto_dpif *ofproto;
>>>>      const struct mac_entry *e;
>>>>
>>>> -    ofproto = ofproto_dpif_lookup_by_name(argv[1]);
>>>> -    if (!ofproto) {
>>>> -        unixctl_command_reply_error(conn, "no such bridge");
>>>> -        return;
>>>> -    }
>>>> -
>>>> -    ds_put_cstr(&ds, " port  VLAN  MAC                Age\n");
>>>> +    ds_put_cstr(ds, " port  VLAN  MAC                Age\n");
>>>>      ovs_rwlock_rdlock(&ofproto->ml->rwlock);
>>>>      LIST_FOR_EACH (e, lru_node, &ofproto->ml->lrus) {
>>>>          struct ofbundle *bundle = mac_entry_get_port(ofproto->ml, e);
>>>> @@ -6173,17 +6165,78 @@ ofproto_unixctl_fdb_show(struct unixctl_conn *conn, int argc OVS_UNUSED,
>>>>
>>>>          ofputil_port_to_string(ofbundle_get_a_port(bundle)->up.ofp_port,
>>>>                  NULL, name, sizeof name);
>>>> -        ds_put_format(&ds, "%5s  %4d  "ETH_ADDR_FMT"  ",
>>>> +        ds_put_format(ds, "%5s  %4d  "ETH_ADDR_FMT"  ",
>>>>                  name, e->vlan, ETH_ADDR_ARGS(e->mac));
>>>>          if (MAC_ENTRY_AGE_STATIC_ENTRY == age) {
>>>> -            ds_put_format(&ds, "static\n");
>>>> +            ds_put_format(ds, "static\n");
>>>>          } else {
>>>> -            ds_put_format(&ds, "%3d\n", age);
>>>> +            ds_put_format(ds, "%3d\n", age);
>>>>          }
>>>>      }
>>>>      ovs_rwlock_unlock(&ofproto->ml->rwlock);
>>>> -    unixctl_command_reply(conn, ds_cstr(&ds));
>>>> -    ds_destroy(&ds);
>>>> +}
>>>> +
>>>> +static void
>>>> +ofproto_unixctl_fdb_show_json(const struct ofproto_dpif *ofproto,
>>>> +                              struct json **fdb_entries)
>>>> +{
>>>> +    size_t num_entries = hmap_count(&ofproto->ml->table);
>>>
>>> This needs to happen under the rw lock, or else entries could be deleted (added).
>>
>> you mean the entire thing to be under rwlock instead of rdlock ?
>> but if we add what you suggested below with checking 'i' then isn't
>> it ok now? we fill the array with mini between 'i' and num_entries.
> 
> Sorry, yes the entire thing between the read lock. The hmap_count() should be under the read lock anyway.
> And if we assert, as suggested below, it should not be unlocked in between.
> 

Hi sorry didn't fully understand the suggested change here :)
I don't think we need to assert, checking 'i' was better.
The hmap_count is just for allocating "enough" room to prepare the output.
Using the read look and limiting to the min between i and num_entries
should be enough. if item being removed/added during this the worst
is we miss an output of an item which will appear in next dump.
The text output also uses the same read lock.
Shouldn't this be enough ?  so I don't think the write lock is needed?
Also just taking the hmap_count should be ok outside the read lock.
Let me know what I miss or if you still think we should replace to rw lock
and if to move hmap_count inside.


>>>
>>>> +    struct json **json_entries = NULL;
>>>> +    const struct mac_entry *entry;
>>>> +    int i = 0;
>>>> +
>>>> +    if (!num_entries) {
>>>> +        goto done;
>>>> +    }
>>>> +
>>>> +    json_entries = xmalloc(num_entries * sizeof *json_entries);
>>>> +    ovs_rwlock_rdlock(&ofproto->ml->rwlock);
>>>> +    LIST_FOR_EACH (entry, lru_node, &ofproto->ml->lrus) {
>>>> +        struct ofbundle *bundle = mac_entry_get_port(ofproto->ml, entry);
>>>> +        struct json *json_entry = json_object_create();
>>>> +        int age = mac_entry_age(ofproto->ml, entry);
>>>> +        long long port;
>>>> +
>>>> +        port = (OVS_FORCE long long) ofbundle_get_a_port(bundle)->up.ofp_port;
>>>> +        json_object_put(json_entry, "port", json_integer_create(port));
>>>> +        json_object_put(json_entry, "vlan", json_integer_create(entry->vlan));
>>>> +        json_object_put_format(json_entry, "mac", ETH_ADDR_FMT,
>>>> +                               ETH_ADDR_ARGS(entry->mac));
>>>> +        if (MAC_ENTRY_AGE_STATIC_ENTRY == age) {
>>>> +            json_object_put_string(json_entry, "age", "static");
>>>> +        } else {
>>>> +            json_object_put(json_entry, "age", json_integer_create(age));
>>>> +        }
>>>> +        json_entries[i++] = json_entry;
>>>
>>> Add a safety check here;
>>> 	if (i >= json_entries) {
>>> 		break;
>>>     }
>>>
>>
>> added. prefered than hitting an assert just for showing some result.
>>
>>> or an ovs_assert() at the beginning of the loop.
>>>
>>>> +    }
>>>> +    ovs_rwlock_unlock(&ofproto->ml->rwlock);
>>>> +done:
>>>> +    *fdb_entries = json_array_create(json_entries, num_entries);
>>>
>>> For safety reasons, I would use ‘i’ here instead of ‘num_entries’ to make sure we have no invalid pointers if, for some reason, the number of entries added is less than the number initially calculated.
>>>
>>
>> updated.
>>
>>>> +}
>>>> +
>>>> +static void
>>>> +ofproto_unixctl_fdb_show(struct unixctl_conn *conn, int argc OVS_UNUSED,
>>>> +                          const char *argv[] OVS_UNUSED, void *aux OVS_UNUSED)
>>>> +{
>>>> +    const struct ofproto_dpif *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_entries;
>>>> +
>>>> +        ofproto_unixctl_fdb_show_json(ofproto, &fdb_entries);
>>>> +        unixctl_command_reply_json(conn, fdb_entries);
>>>> +    } else {
>>>> +        struct ds ds = DS_EMPTY_INITIALIZER;
>>>> +
>>>> +        ofproto_unixctl_fdb_show_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 fa5f148b4c28..8a975e6ffd9c 100644
>>>> --- a/tests/ofproto-dpif.at
>>>> +++ b/tests/ofproto-dpif.at
>>>> @@ -7085,6 +7085,16 @@ AT_CHECK_UNQUOTED([ovs-appctl fdb/show br0 | sed 's/[[0-9]]\{1,\}$/?/'], [0], [d
>>>>      3     0  50:54:00:00:00:05    ?
>>>>  ])
>>>>
>>>> +dnl Check json output.
>>>> +AT_CHECK([ovs-appctl --format json --pretty fdb/show br0], [0], [dnl
>>>> +[[
>>>> +  {
>>>> +    "age": 0,
>>>> +    "mac": "50:54:00:00:00:05",
>>>> +    "port": 3,
>>>> +    "vlan": 0}]]
>>>> +])
>>>> +
>>>
>>> If you run the tests enough times, some times the age is 1, and the test fails. In addition I think we should move this to a place where we have at least two entries.
>>> For example:
>>>
>>> @@ -7110,6 +7100,22 @@ AT_CHECK_UNQUOTED([ovs-appctl fdb/show br0 | sed 's/[[0-9]]\{1,\}$/?/'], [0], [d
>>>      1     0  50:54:00:00:00:06    ?
>>>  ])
>>>
>>> +dnl Check json output.
>>> +AT_CHECK([ovs-appctl --format json --pretty fdb/show br0 \
>>> +          | sed 's/"age": [[0-9]]\+/"age": ?/g'], [0], [dnl
>>> +[[
>>> +  {
>>> +    "age": ?,
>>> +    "mac": "50:54:00:00:00:05",
>>> +    "port": 3,
>>> +    "vlan": 0},
>>> +  {
>>> +    "age": ?,
>>> +    "mac": "50:54:00:00:00:06",
>>> +    "port": 1,
>>> +    "vlan": 0}]]
>>> +])
>>> +
>>>
>>
>> ok updated.
>>
>>>>  # Trace a packet arrival destined for the learned MAC.
>>>>  # (This will also learn a MAC.)
>>>>  OFPROTO_TRACE(
>>>> -- 
>>>> 2.21.0
>>>
>

On 20/02/2025 12:57, Ilya Maximets wrote:
> On 2/18/25 12:03, Roi Dayan wrote:
>> From: Dima Chumak <dchumak@nvidia.com>
>>
>> The 'fdb/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 --pretty fdb/show br-phy
>>   [
>>     {
>>       "age": "static",
>>       "mac": "e4:8c:07:08:00:02",
>>       "port": 2,
>>       "vlan": 0
>>     },
>>     {
>>       "age": 14,
>>       "mac": "e4:8c:07:08:00:03",
>>       "port": 3,
>>       "vlan": 0
>>     }
>>   ]
> 
> Instead of returning different type values for the age, should we
> perhaps return "static" as a separate boolean field and not report
> the "age" in this case?
> 
> Alternative might be to report age as a string always.  But that
> doesn't sound particularly great.
> 
> It may be very inconvenient to write parsers in some languages if
> the type of the value can be both int and the string.
> 
> Best regards, Ilya Maximets.

right. Fixed for v4.
thanks

On 23/02/2025 9:29, Roi Dayan wrote:
> 
> 
> On 20/02/2025 12:04, Eelco Chaudron wrote:
>>
>>
>> On 20 Feb 2025, at 10:55, Roi Dayan wrote:
>>
>>> On 18/02/2025 14:51, Eelco Chaudron wrote:
>>>>
>>>>
>>>> On 18 Feb 2025, at 12:03, Roi Dayan wrote:
>>>>
>>>>> From: Dima Chumak <dchumak@nvidia.com>
>>>>>
>>>>> The 'fdb/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 --pretty fdb/show br-phy
>>>>>   [
>>>>>     {
>>>>>       "age": "static",
>>>>>       "mac": "e4:8c:07:08:00:02",
>>>>>       "port": 2,
>>>>>       "vlan": 0
>>>>>     },
>>>>>     {
>>>>>       "age": 14,
>>>>>       "mac": "e4:8c:07:08:00:03",
>>>>>       "port": 3,
>>>>>       "vlan": 0
>>>>>     }
>>>>>   ]
>>>>>
>>>>> Signed-off-by: Dima Chumak <dchumak@nvidia.com>
>>>>> Acked-by: Roi Dayan <roid@nvidia.com>
>>>>> ---
>>>>>
>>>>> Notes:
>>>>>     v3
>>>>>     - Fix assert. No need to call json_destroy() after unixctl_command_reply_json() as it takes
>>>>>       ownership of the allocated body.
>>>>>
>>>>>     v2
>>>>>     - Fix sparse check error.
>>>>>     - Add case checking fdb/show json output.
>>>>>
>>>>>  ofproto/ofproto-dpif.c | 85 ++++++++++++++++++++++++++++++++++--------
>>>>>  tests/ofproto-dpif.at  | 10 +++++
>>>>>  2 files changed, 79 insertions(+), 16 deletions(-)
>>>>>
>>>>> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
>>>>> index bf43d5d4bc59..9926c016fb00 100644
>>>>> --- a/ofproto/ofproto-dpif.c
>>>>> +++ b/ofproto/ofproto-dpif.c
>>>>> @@ -6151,20 +6151,12 @@ ofbundle_get_a_port(const struct ofbundle *bundle)
>>>>>  }
>>>>>
>>>>>  static void
>>>>> -ofproto_unixctl_fdb_show(struct unixctl_conn *conn, int argc OVS_UNUSED,
>>>>> -                         const char *argv[], void *aux OVS_UNUSED)
>>>>> +ofproto_unixctl_fdb_show_text(const struct ofproto_dpif *ofproto,
>>>>> +                              struct ds *ds)
>>>>>  {
>>>>> -    struct ds ds = DS_EMPTY_INITIALIZER;
>>>>> -    const struct ofproto_dpif *ofproto;
>>>>>      const struct mac_entry *e;
>>>>>
>>>>> -    ofproto = ofproto_dpif_lookup_by_name(argv[1]);
>>>>> -    if (!ofproto) {
>>>>> -        unixctl_command_reply_error(conn, "no such bridge");
>>>>> -        return;
>>>>> -    }
>>>>> -
>>>>> -    ds_put_cstr(&ds, " port  VLAN  MAC                Age\n");
>>>>> +    ds_put_cstr(ds, " port  VLAN  MAC                Age\n");
>>>>>      ovs_rwlock_rdlock(&ofproto->ml->rwlock);
>>>>>      LIST_FOR_EACH (e, lru_node, &ofproto->ml->lrus) {
>>>>>          struct ofbundle *bundle = mac_entry_get_port(ofproto->ml, e);
>>>>> @@ -6173,17 +6165,78 @@ ofproto_unixctl_fdb_show(struct unixctl_conn *conn, int argc OVS_UNUSED,
>>>>>
>>>>>          ofputil_port_to_string(ofbundle_get_a_port(bundle)->up.ofp_port,
>>>>>                  NULL, name, sizeof name);
>>>>> -        ds_put_format(&ds, "%5s  %4d  "ETH_ADDR_FMT"  ",
>>>>> +        ds_put_format(ds, "%5s  %4d  "ETH_ADDR_FMT"  ",
>>>>>                  name, e->vlan, ETH_ADDR_ARGS(e->mac));
>>>>>          if (MAC_ENTRY_AGE_STATIC_ENTRY == age) {
>>>>> -            ds_put_format(&ds, "static\n");
>>>>> +            ds_put_format(ds, "static\n");
>>>>>          } else {
>>>>> -            ds_put_format(&ds, "%3d\n", age);
>>>>> +            ds_put_format(ds, "%3d\n", age);
>>>>>          }
>>>>>      }
>>>>>      ovs_rwlock_unlock(&ofproto->ml->rwlock);
>>>>> -    unixctl_command_reply(conn, ds_cstr(&ds));
>>>>> -    ds_destroy(&ds);
>>>>> +}
>>>>> +
>>>>> +static void
>>>>> +ofproto_unixctl_fdb_show_json(const struct ofproto_dpif *ofproto,
>>>>> +                              struct json **fdb_entries)
>>>>> +{
>>>>> +    size_t num_entries = hmap_count(&ofproto->ml->table);
>>>>
>>>> This needs to happen under the rw lock, or else entries could be deleted (added).
>>>
>>> you mean the entire thing to be under rwlock instead of rdlock ?
>>> but if we add what you suggested below with checking 'i' then isn't
>>> it ok now? we fill the array with mini between 'i' and num_entries.
>>
>> Sorry, yes the entire thing between the read lock. The hmap_count() should be under the read lock anyway.
>> And if we assert, as suggested below, it should not be unlocked in between.
>>
> 
> Hi sorry didn't fully understand the suggested change here :)
> I don't think we need to assert, checking 'i' was better.
> The hmap_count is just for allocating "enough" room to prepare the output.
> Using the read look and limiting to the min between i and num_entries
> should be enough. if item being removed/added during this the worst
> is we miss an output of an item which will appear in next dump.
> The text output also uses the same read lock.
> Shouldn't this be enough ?  so I don't think the write lock is needed?
> Also just taking the hmap_count should be ok outside the read lock.
> Let me know what I miss or if you still think we should replace to rw lock
> and if to move hmap_count inside.
> 
> 

I sent v4 with the changes about the safety check and ilya comment on the json output.
Can you please check and reply on v4 if you think the rw-lock change is still relevant
thanks

>>>>
>>>>> +    struct json **json_entries = NULL;
>>>>> +    const struct mac_entry *entry;
>>>>> +    int i = 0;
>>>>> +
>>>>> +    if (!num_entries) {
>>>>> +        goto done;
>>>>> +    }
>>>>> +
>>>>> +    json_entries = xmalloc(num_entries * sizeof *json_entries);
>>>>> +    ovs_rwlock_rdlock(&ofproto->ml->rwlock);
>>>>> +    LIST_FOR_EACH (entry, lru_node, &ofproto->ml->lrus) {
>>>>> +        struct ofbundle *bundle = mac_entry_get_port(ofproto->ml, entry);
>>>>> +        struct json *json_entry = json_object_create();
>>>>> +        int age = mac_entry_age(ofproto->ml, entry);
>>>>> +        long long port;
>>>>> +
>>>>> +        port = (OVS_FORCE long long) ofbundle_get_a_port(bundle)->up.ofp_port;
>>>>> +        json_object_put(json_entry, "port", json_integer_create(port));
>>>>> +        json_object_put(json_entry, "vlan", json_integer_create(entry->vlan));
>>>>> +        json_object_put_format(json_entry, "mac", ETH_ADDR_FMT,
>>>>> +                               ETH_ADDR_ARGS(entry->mac));
>>>>> +        if (MAC_ENTRY_AGE_STATIC_ENTRY == age) {
>>>>> +            json_object_put_string(json_entry, "age", "static");
>>>>> +        } else {
>>>>> +            json_object_put(json_entry, "age", json_integer_create(age));
>>>>> +        }
>>>>> +        json_entries[i++] = json_entry;
>>>>
>>>> Add a safety check here;
>>>> 	if (i >= json_entries) {
>>>> 		break;
>>>>     }
>>>>
>>>
>>> added. prefered than hitting an assert just for showing some result.
>>>
>>>> or an ovs_assert() at the beginning of the loop.
>>>>
>>>>> +    }
>>>>> +    ovs_rwlock_unlock(&ofproto->ml->rwlock);
>>>>> +done:
>>>>> +    *fdb_entries = json_array_create(json_entries, num_entries);
>>>>
>>>> For safety reasons, I would use ‘i’ here instead of ‘num_entries’ to make sure we have no invalid pointers if, for some reason, the number of entries added is less than the number initially calculated.
>>>>
>>>
>>> updated.
>>>
>>>>> +}
>>>>> +
>>>>> +static void
>>>>> +ofproto_unixctl_fdb_show(struct unixctl_conn *conn, int argc OVS_UNUSED,
>>>>> +                          const char *argv[] OVS_UNUSED, void *aux OVS_UNUSED)
>>>>> +{
>>>>> +    const struct ofproto_dpif *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_entries;
>>>>> +
>>>>> +        ofproto_unixctl_fdb_show_json(ofproto, &fdb_entries);
>>>>> +        unixctl_command_reply_json(conn, fdb_entries);
>>>>> +    } else {
>>>>> +        struct ds ds = DS_EMPTY_INITIALIZER;
>>>>> +
>>>>> +        ofproto_unixctl_fdb_show_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 fa5f148b4c28..8a975e6ffd9c 100644
>>>>> --- a/tests/ofproto-dpif.at
>>>>> +++ b/tests/ofproto-dpif.at
>>>>> @@ -7085,6 +7085,16 @@ AT_CHECK_UNQUOTED([ovs-appctl fdb/show br0 | sed 's/[[0-9]]\{1,\}$/?/'], [0], [d
>>>>>      3     0  50:54:00:00:00:05    ?
>>>>>  ])
>>>>>
>>>>> +dnl Check json output.
>>>>> +AT_CHECK([ovs-appctl --format json --pretty fdb/show br0], [0], [dnl
>>>>> +[[
>>>>> +  {
>>>>> +    "age": 0,
>>>>> +    "mac": "50:54:00:00:00:05",
>>>>> +    "port": 3,
>>>>> +    "vlan": 0}]]
>>>>> +])
>>>>> +
>>>>
>>>> If you run the tests enough times, some times the age is 1, and the test fails. In addition I think we should move this to a place where we have at least two entries.
>>>> For example:
>>>>
>>>> @@ -7110,6 +7100,22 @@ AT_CHECK_UNQUOTED([ovs-appctl fdb/show br0 | sed 's/[[0-9]]\{1,\}$/?/'], [0], [d
>>>>      1     0  50:54:00:00:00:06    ?
>>>>  ])
>>>>
>>>> +dnl Check json output.
>>>> +AT_CHECK([ovs-appctl --format json --pretty fdb/show br0 \
>>>> +          | sed 's/"age": [[0-9]]\+/"age": ?/g'], [0], [dnl
>>>> +[[
>>>> +  {
>>>> +    "age": ?,
>>>> +    "mac": "50:54:00:00:00:05",
>>>> +    "port": 3,
>>>> +    "vlan": 0},
>>>> +  {
>>>> +    "age": ?,
>>>> +    "mac": "50:54:00:00:00:06",
>>>> +    "port": 1,
>>>> +    "vlan": 0}]]
>>>> +])
>>>> +
>>>>
>>>
>>> ok updated.
>>>
>>>>>  # Trace a packet arrival destined for the learned MAC.
>>>>>  # (This will also learn a MAC.)
>>>>>  OFPROTO_TRACE(
>>>>> -- 
>>>>> 2.21.0
>>>>
>>
>