diff mbox series

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

Message ID 20250223080519.2721441-1-roid@nvidia.com
State Changes Requested
Headers show
Series [ovs-dev,v5] ofproto: Add JSON output for 'fdb/show' command. | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/cirrus-robot fail cirrus build: failed
ovsrobot/github-robot-_Build_and_Test success github build: passed

Commit Message

Roi Dayan Feb. 23, 2025, 8:05 a.m. UTC
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
  [
    {
      "static": true,
      "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:
    v5
    - Forgot to remove the wrong string output of age static.
    - Added test case for static entry. The two testsuite case numbers are 1279 1282.
    
    v4
    - Make sure not to set more entries than allocated in json output.
    - Fix json output to have either boolean static or integer age and not mix the types.
    
    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 | 89 ++++++++++++++++++++++++++++++++++--------
 tests/ofproto-dpif.at  | 42 ++++++++++++++++++++
 2 files changed, 115 insertions(+), 16 deletions(-)

Comments

Roi Dayan Feb. 23, 2025, 1:06 p.m. UTC | #1
On 23/02/2025 10:05, 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
>   [
>     {
>       "static": true,
>       "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:
>     v5
>     - Forgot to remove the wrong string output of age static.
>     - Added test case for static entry. The two testsuite case numbers are 1279 1282.
>     
>     v4
>     - Make sure not to set more entries than allocated in json output.
>     - Fix json output to have either boolean static or integer age and not mix the types.
>     
>     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 | 89 ++++++++++++++++++++++++++++++++++--------
>  tests/ofproto-dpif.at  | 42 ++++++++++++++++++++
>  2 files changed, 115 insertions(+), 16 deletions(-)
> 
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index bf43d5d4bc59..5d485fc6859c 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,82 @@ 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);
> +    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(json_entry, "static", json_boolean_create(true));
> +        } else {
> +            json_object_put(json_entry, "age", json_integer_create(age));
> +        }
> +        json_entries[i++] = json_entry;
> +
> +        if (i >= num_entries) {
> +            break;
> +        }
> +    }
> +    ovs_rwlock_unlock(&ofproto->ml->rwlock);
> +done:
> +    *fdb_entries = json_array_create(json_entries, i);
> +}
> +
> +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..a0675cd55c66 100644
> --- a/tests/ofproto-dpif.at
> +++ b/tests/ofproto-dpif.at
> @@ -7100,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 that updates the first learned MAC entry.
>  OFPROTO_TRACE(
>    [ovs-dummy],
> @@ -7708,6 +7724,32 @@ AT_CHECK_UNQUOTED([ovs-appctl fdb/show br0 | sed 's/ *[[0-9]]\{1,\}$//' | grep -
>      2     0  50:54:00:00:02:02  static
>  ])
>  
> +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:01",
> +    "port": 1,
> +    "vlan": 0},
> +  {
> +    "age": ?,
> +    "mac": "50:54:00:00:00:02",
> +    "port": 2,
> +    "vlan": 0},
> +  {
> +    "mac": "50:54:00:00:01:01",
> +    "port": 1,
> +    "static": true,
> +    "vlan": 0},
> +  {
> +    "mac": "50:54:00:00:02:02",
> +    "port": 2,
> +    "static": true,
> +    "vlan": 0}]]
> +])
> +
>  dnl Remove static mac entry.
>  AT_CHECK([ovs-appctl fdb/del br0 0 50:54:00:00:01:01])
>  

the bot failed but not sure why the sed not working for it as it works locally.

https://cirrus-ci.com/task/6174704781754368?logs=check#L3226


+++ /tmp/cirrus-ci-build/tests/testsuite.dir/at-groups/1282/stdout	2025-02-23 08:22:16.532524000 +0000
@@ -1,11 +1,11 @@
 [
   {
-    "age": ?,
+    "age": 0,
     "mac": "50:54:00:00:00:01",
Eelco Chaudron Feb. 24, 2025, 4:01 p.m. UTC | #2
On 23 Feb 2025, at 9:05, 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
>   [
>     {
>       "static": true,
>       "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:
>     v5
>     - Forgot to remove the wrong string output of age static.
>     - Added test case for static entry. The two testsuite case numbers are 1279 1282.
>
>     v4
>     - Make sure not to set more entries than allocated in json output.
>     - Fix json output to have either boolean static or integer age and not mix the types.
>
>     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 | 89 ++++++++++++++++++++++++++++++++++--------
>  tests/ofproto-dpif.at  | 42 ++++++++++++++++++++
>  2 files changed, 115 insertions(+), 16 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index bf43d5d4bc59..5d485fc6859c 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,82 @@ 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);

The hmap_count() needs to be called under a lock, as it’s a non-atomic variable. In addition to this, some cleanups in the diff below. Let me know what you think.

In addition, I did try testing, but I could not get it to fail the sed on a local machine (1000+ tries) or on GitHub (https://github.com/chaudron/ovs/actions/runs/13498345963). Maybe just send it with a v6 and let's see.


diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 5d485fc68..25b1d9322 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -6180,25 +6180,30 @@ 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);
     struct json **json_entries = NULL;
     const struct mac_entry *entry;
+    size_t num_entries;
     int i = 0;

+    ovs_rwlock_rdlock(&ofproto->ml->rwlock);
+
+    num_entries = hmap_count(&ofproto->ml->table);
     if (!num_entries) {
-        goto done;
+        goto done_unlock;
     }

     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 ofport_dpif *port = ofbundle_get_a_port(bundle);
         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));
+        ovs_assert(i < num_entries);
+        json_object_put(json_entry, "port",
+                        json_integer_create(
+                            (OVS_FORCE long long) port->up.ofp_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));
@@ -6208,13 +6213,9 @@ ofproto_unixctl_fdb_show_json(const struct ofproto_dpif *ofproto,
             json_object_put(json_entry, "age", json_integer_create(age));
         }
         json_entries[i++] = json_entry;
-
-        if (i >= num_entries) {
-            break;
-        }
     }
+done_unlock:
     ovs_rwlock_unlock(&ofproto->ml->rwlock);
-done:
     *fdb_entries = json_array_create(json_entries, i);
 }


> +    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(json_entry, "static", json_boolean_create(true));
> +        } else {
> +            json_object_put(json_entry, "age", json_integer_create(age));
> +        }
> +        json_entries[i++] = json_entry;
> +
> +        if (i >= num_entries) {
> +            break;
> +        }
> +    }
> +    ovs_rwlock_unlock(&ofproto->ml->rwlock);
> +done:
> +    *fdb_entries = json_array_create(json_entries, i);
> +}
> +
> +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..a0675cd55c66 100644
> --- a/tests/ofproto-dpif.at
> +++ b/tests/ofproto-dpif.at
> @@ -7100,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 that updates the first learned MAC entry.
>  OFPROTO_TRACE(
>    [ovs-dummy],
> @@ -7708,6 +7724,32 @@ AT_CHECK_UNQUOTED([ovs-appctl fdb/show br0 | sed 's/ *[[0-9]]\{1,\}$//' | grep -
>      2     0  50:54:00:00:02:02  static
>  ])
>
> +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:01",
> +    "port": 1,
> +    "vlan": 0},
> +  {
> +    "age": ?,
> +    "mac": "50:54:00:00:00:02",
> +    "port": 2,
> +    "vlan": 0},
> +  {
> +    "mac": "50:54:00:00:01:01",
> +    "port": 1,
> +    "static": true,
> +    "vlan": 0},
> +  {
> +    "mac": "50:54:00:00:02:02",
> +    "port": 2,
> +    "static": true,
> +    "vlan": 0}]]
> +])
> +
>  dnl Remove static mac entry.
>  AT_CHECK([ovs-appctl fdb/del br0 0 50:54:00:00:01:01])
>
> -- 
> 2.21.0
Ilya Maximets Feb. 24, 2025, 4:06 p.m. UTC | #3
On 2/24/25 17:01, Eelco Chaudron wrote:
> 
> 
> On 23 Feb 2025, at 9:05, 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
>>   [
>>     {
>>       "static": true,
>>       "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:
>>     v5
>>     - Forgot to remove the wrong string output of age static.
>>     - Added test case for static entry. The two testsuite case numbers are 1279 1282.
>>
>>     v4
>>     - Make sure not to set more entries than allocated in json output.
>>     - Fix json output to have either boolean static or integer age and not mix the types.
>>
>>     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 | 89 ++++++++++++++++++++++++++++++++++--------
>>  tests/ofproto-dpif.at  | 42 ++++++++++++++++++++
>>  2 files changed, 115 insertions(+), 16 deletions(-)
>>
>> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
>> index bf43d5d4bc59..5d485fc6859c 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,82 @@ 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);
> 
> The hmap_count() needs to be called under a lock, as it’s a non-atomic variable. In addition to this, some cleanups in the diff below. Let me know what you think.
> 
> In addition, I did try testing, but I could not get it to fail the sed on a local machine (1000+ tries) or on GitHub (https://github.com/chaudron/ovs/actions/runs/13498345963). Maybe just send it with a v6 and let's see.

sed fails on FreeBSD, because \+ is not portable and not supported
by POSIX sed.  Need to use * instead, should be fine in this case.
Running on linux with GNU sed will not trigger the issue.

Best regards, Ilya Maximets.
Eelco Chaudron Feb. 24, 2025, 4:28 p.m. UTC | #4
On 24 Feb 2025, at 17:06, Ilya Maximets wrote:

> On 2/24/25 17:01, Eelco Chaudron wrote:
>>
>>
>> On 23 Feb 2025, at 9:05, 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
>>>   [
>>>     {
>>>       "static": true,
>>>       "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:
>>>     v5
>>>     - Forgot to remove the wrong string output of age static.
>>>     - Added test case for static entry. The two testsuite case numbers are 1279 1282.
>>>
>>>     v4
>>>     - Make sure not to set more entries than allocated in json output.
>>>     - Fix json output to have either boolean static or integer age and not mix the types.
>>>
>>>     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 | 89 ++++++++++++++++++++++++++++++++++--------
>>>  tests/ofproto-dpif.at  | 42 ++++++++++++++++++++
>>>  2 files changed, 115 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
>>> index bf43d5d4bc59..5d485fc6859c 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,82 @@ 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);
>>
>> The hmap_count() needs to be called under a lock, as it’s a non-atomic variable. In addition to this, some cleanups in the diff below. Let me know what you think.
>>
>> In addition, I did try testing, but I could not get it to fail the sed on a local machine (1000+ tries) or on GitHub (https://github.com/chaudron/ovs/actions/runs/13498345963). Maybe just send it with a v6 and let's see.
>
> sed fails on FreeBSD, because \+ is not portable and not supported
> by POSIX sed.  Need to use * instead, should be fine in this case.
> Running on linux with GNU sed will not trigger the issue.

Thanks Ilya, I guess next time, I should check the specific test environment :(
Roi Dayan March 4, 2025, 10:50 a.m. UTC | #5
On 24/02/2025 18:28, Eelco Chaudron wrote:
> 
> 
> On 24 Feb 2025, at 17:06, Ilya Maximets wrote:
> 
>> On 2/24/25 17:01, Eelco Chaudron wrote:
>>>
>>>
>>> On 23 Feb 2025, at 9:05, 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
>>>>   [
>>>>     {
>>>>       "static": true,
>>>>       "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:
>>>>     v5
>>>>     - Forgot to remove the wrong string output of age static.
>>>>     - Added test case for static entry. The two testsuite case numbers are 1279 1282.
>>>>
>>>>     v4
>>>>     - Make sure not to set more entries than allocated in json output.
>>>>     - Fix json output to have either boolean static or integer age and not mix the types.
>>>>
>>>>     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 | 89 ++++++++++++++++++++++++++++++++++--------
>>>>  tests/ofproto-dpif.at  | 42 ++++++++++++++++++++
>>>>  2 files changed, 115 insertions(+), 16 deletions(-)
>>>>
>>>> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
>>>> index bf43d5d4bc59..5d485fc6859c 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,82 @@ 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);
>>>
>>> The hmap_count() needs to be called under a lock, as it’s a non-atomic variable. In addition to this, some cleanups in the diff below. Let me know what you think.
>>>
>>> In addition, I did try testing, but I could not get it to fail the sed on a local machine (1000+ tries) or on GitHub (https://github.com/chaudron/ovs/actions/runs/13498345963). Maybe just send it with a v6 and let's see.
>>
>> sed fails on FreeBSD, because \+ is not portable and not supported
>> by POSIX sed.  Need to use * instead, should be fine in this case.
>> Running on linux with GNU sed will not trigger the issue.
> 
> Thanks Ilya, I guess next time, I should check the specific test environment :(
> 

thanks for all the help and info.
I'll send v6 with the changes. also updated the test to use *.
diff mbox series

Patch

diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index bf43d5d4bc59..5d485fc6859c 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,82 @@  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);
+    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(json_entry, "static", json_boolean_create(true));
+        } else {
+            json_object_put(json_entry, "age", json_integer_create(age));
+        }
+        json_entries[i++] = json_entry;
+
+        if (i >= num_entries) {
+            break;
+        }
+    }
+    ovs_rwlock_unlock(&ofproto->ml->rwlock);
+done:
+    *fdb_entries = json_array_create(json_entries, i);
+}
+
+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..a0675cd55c66 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -7100,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 that updates the first learned MAC entry.
 OFPROTO_TRACE(
   [ovs-dummy],
@@ -7708,6 +7724,32 @@  AT_CHECK_UNQUOTED([ovs-appctl fdb/show br0 | sed 's/ *[[0-9]]\{1,\}$//' | grep -
     2     0  50:54:00:00:02:02  static
 ])
 
+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:01",
+    "port": 1,
+    "vlan": 0},
+  {
+    "age": ?,
+    "mac": "50:54:00:00:00:02",
+    "port": 2,
+    "vlan": 0},
+  {
+    "mac": "50:54:00:00:01:01",
+    "port": 1,
+    "static": true,
+    "vlan": 0},
+  {
+    "mac": "50:54:00:00:02:02",
+    "port": 2,
+    "static": true,
+    "vlan": 0}]]
+])
+
 dnl Remove static mac entry.
 AT_CHECK([ovs-appctl fdb/del br0 0 50:54:00:00:01:01])