On 7/2/26 7:26 PM, Takeru Hayasaka wrote:
> Previously fdb/show required exactly one bridge argument. This change
> lets it accept several bridge names in a single call, and show all
> bridges when no argument is given. This reduces the number of unixctl
> round-trips for clients that poll FDB entries across many bridges
> (e.g. EVPN agents managing multiple VNIs), while still allowing a
> client to query only the subset of bridges it cares about.
>
> When more than one bridge is shown, the entries are grouped under a
> "Bridge" column, sized to the widest bridge name shown, that prints
> each bridge name only on its first row; a bridge with no entries is
> listed as a name-only row so it stays visible. JSON output is now an
> object keyed by bridge name whose values are arrays of that bridge's
> entries, e.g. {"br0": [...], "br1": [...]}; this replaces the bare
> entries array previously returned for a single bridge. Single-bridge
> text output is unchanged.
>
> Signed-off-by: Takeru Hayasaka <[email protected]>
Thanks for the update! The patch looks good to me in general.
See a couple small comments below.
Best regards, Ilya Maximets.
> ---
> v4:
> - Rebased on current master; the NEWS entry now lives under the existing
> "ovs-appctl" section. No functional changes.
> v3:
> - No arguments now shows all bridges, matching commands like fdb/flush
> (suggested by Ilya Maximets); one or more bridge names still restricts
> output to just those bridges.
> - Replaced the v2 "bridge <name>" header line with a "Bridge" column,
> added only when more than one bridge is shown, so single-bridge output
> is byte-for-byte identical to before. This reverts the test churn
> introduced in v2 (including tests/stp.at).
> - The "Bridge" column is sized to the widest bridge name shown and
> groups each bridge's entries, printing the name only on the bridge's
> first row. A bridge with no entries is listed as a name-only row, so
> that (as in JSON) an existing-but-empty bridge stays visible in a
> multi-bridge listing. Added tests covering long names, a bridge with
> multiple entries, and an empty bridge.
> - JSON output is now an object keyed by bridge name ({"br0": [...]})
> rather than an array of {"bridge", "entries"} objects.
> - Trimmed the NEWS entry to a single item.
> - Updated the ovs-vswitchd(8) man page to match the above.
> v2:
> - Moved the "bridge <name>" prefixing into the per-bridge text and JSON
> helpers (suggested by Mike Pattrick).
> - Updated the unixctl usage hint and the ovs-vswitchd(8) man page.
> - Added tests for an empty-FDB JSON response and for the error path when
> a requested bridge does not exist.
>
> NEWS | 2 +
> ofproto/ofproto-dpif.c | 84 ++++++++++++++++---
> tests/ofproto-dpif.at | 162 +++++++++++++++++++++++++++++--------
> vswitchd/ovs-vswitchd.8.in | 10 ++-
> 4 files changed, 211 insertions(+), 47 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index c9331321c0ad..608da24ca928 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -16,6 +16,8 @@ Post-v3.7.0
> * 'dpif-netdev/pmd-stats-show' command was removed in favor of the more
> informative and better structured 'dpif-netdev/pmd-perf-show', which
> now also provides statistics for the "main" thread.
> + * "fdb/show" now accepts multiple bridge names, and shows all bridges
> + when no bridge is specified.
> - Python:
> * The ovstest Python module is now removed. It was only used by the
> ovs-test and ovs-l3ping utilities.
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 729bc6f1d4ac..0c970b55fd27 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -6165,11 +6165,11 @@ ofbundle_get_a_port(const struct ofbundle *bundle)
>
> static void
> ofproto_unixctl_fdb_show_text(const struct ofproto_dpif *ofproto,
> - struct ds *ds)
> + struct ds *ds, int name_width)
May be better to call it 'br_width', as we use 'name' for port names
inside the function.
> {
> const struct mac_entry *e;
> + bool first = true;
>
> - 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);
> @@ -6178,6 +6178,12 @@ ofproto_unixctl_fdb_show_text(const struct
> ofproto_dpif *ofproto,
>
> ofputil_port_to_string(ofbundle_get_a_port(bundle)->up.ofp_port,
> NULL, name, sizeof name);
> + if (name_width) {
> + /* Print the bridge name only on its first entry so that a
> + * bridge's rows read as a single group. */
> + ds_put_format(ds, " %-*s", name_width,
> + first ? ofproto->up.name : "");
> + }
> ds_put_format(ds, "%5s %4d "ETH_ADDR_FMT" ",
> name, e->vlan, ETH_ADDR_ARGS(e->mac));
> if (MAC_ENTRY_AGE_STATIC_ENTRY == age) {
> @@ -6185,6 +6191,12 @@ ofproto_unixctl_fdb_show_text(const struct
> ofproto_dpif *ofproto,
> } else {
> ds_put_format(ds, "%3d\n", age);
> }
> + first = false;
> + }
> + if (name_width && first) {
> + /* Bridge has no entries; still show its name so that it stays
> + * visible in a multi-bridge listing, matching JSON output. */
> + ds_put_format(ds, " %s\n", ofproto->up.name);
> }
> ovs_rwlock_unlock(&ofproto->ml->rwlock);
> }
> @@ -6233,28 +6245,74 @@ done_unlock:
> }
>
> static void
> -ofproto_unixctl_fdb_show(struct unixctl_conn *conn, int argc OVS_UNUSED,
> - const char *argv[] OVS_UNUSED, void *aux
> OVS_UNUSED)
> +ofproto_unixctl_fdb_show(struct unixctl_conn *conn, int argc,
> + const char *argv[], void *aux OVS_UNUSED)
> {
> - const struct ofproto_dpif *ofproto =
> ofproto_dpif_lookup_by_name(argv[1]);
> + const struct ofproto_dpif **ofprotos;
> + size_t n = 0;
>
> - if (!ofproto) {
> - unixctl_command_reply_error(conn, "no such bridge");
> - return;
> + if (argc > 1) {
> + /* Validate all requested bridges up front so that a bad name
> + * produces an error with no partial output. */
> + ofprotos = xmalloc((argc - 1) * sizeof *ofprotos);
> + for (int i = 1; i < argc; i++) {
> + const struct ofproto_dpif *ofproto =
> + ofproto_dpif_lookup_by_name(argv[i]);
> +
> + if (!ofproto) {
> + unixctl_command_reply_error(conn, "no such bridge");
Would be good to print out the argument that triggered the error, i.e.
which bridge doesn't exist.
> + free(ofprotos);
> + return;
> + }
> + ofprotos[n++] = ofproto;
> + }
> + } else {
> + /* No bridge specified: show all of them. */
> + struct ofproto_dpif *ofproto;
> +
> + ofprotos = xmalloc(hmap_count(&all_ofproto_dpifs_by_name)
> + * sizeof *ofprotos);
> + HMAP_FOR_EACH (ofproto, all_ofproto_dpifs_by_name_node,
> + &all_ofproto_dpifs_by_name) {
> + ofprotos[n++] = ofproto;
> + }
> }
>
> if (unixctl_command_get_output_format(conn) == UNIXCTL_OUTPUT_FMT_JSON) {
> - struct json *fdb_entries;
> + struct json *obj = json_object_create();
>
> - ofproto_unixctl_fdb_show_json(ofproto, &fdb_entries);
> - unixctl_command_reply_json(conn, fdb_entries);
> + for (size_t i = 0; i < n; i++) {
> + struct json *entries;
> +
> + ofproto_unixctl_fdb_show_json(ofprotos[i], &entries);
> + json_object_put(obj, ofprotos[i]->up.name, entries);
> + }
> +
> + unixctl_command_reply_json(conn, obj);
> } else {
> struct ds ds = DS_EMPTY_INITIALIZER;
> + int name_width = 0;
> +
> + if (n > 1) {
> + name_width = strlen("Bridge");
> + for (size_t i = 0; i < n; i++) {
> + int len = strlen(ofprotos[i]->up.name);
We don't really need this variable. The strlen can be put directly
into the MAX, saving 2 lines.
> +
> + name_width = MAX(name_width, len);
> + }
> + ds_put_format(&ds, " %-*s", name_width, "Bridge");
> + }
> + ds_put_cstr(&ds, " port VLAN MAC Age\n");
There are 2 spaces at least between column names in the header, but
there is only one reserved between the 'Bridge' and the 'port'. It
will look better if we have 2 here as well, e.g. by increasing the
name_width by one at the end.
It may be better to also convert 'port' into 'Port', so all the columns
look more similar.
> +
> + for (size_t i = 0; i < n; i++) {
> + ofproto_unixctl_fdb_show_text(ofprotos[i], &ds, name_width);
> + }
>
> - ofproto_unixctl_fdb_show_text(ofproto, &ds);
> unixctl_command_reply(conn, ds_cstr(&ds));
> ds_destroy(&ds);
> }
> +
> + free(ofprotos);
> }
>
> static void
> @@ -7122,7 +7180,7 @@ ofproto_unixctl_init(void)
> ofproto_unixctl_fdb_delete, NULL);
> unixctl_command_register("fdb/flush", "[bridge]", 0, 1,
> ofproto_unixctl_fdb_flush, NULL);
> - unixctl_command_register("fdb/show", "bridge", 1, 1,
> + unixctl_command_register("fdb/show", "[bridge...]", 0, INT_MAX,
> ofproto_unixctl_fdb_show, NULL);
> unixctl_command_register("fdb/stats-clear", "[bridge]", 0, 1,
> ofproto_unixctl_fdb_stats_clear, NULL);
> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
> index 9e64e8ee4bd7..56e3b8a65f97 100644
> --- a/tests/ofproto-dpif.at
> +++ b/tests/ofproto-dpif.at
> @@ -7543,6 +7543,12 @@ add_of_ports br0 1 2 3
>
>
> arp='eth_type(0x0806),arp(sip=192.168.0.1,tip=192.168.0.2,op=1,sha=50:54:00:00:00:05,tha=00:00:00:00:00:00)'
>
> +dnl Check json output for an empty FDB.
> +AT_CHECK([ovs-appctl --format json --pretty fdb/show br0], [0], [dnl
> +[{
> + "br0": []}]
> +])
> +
> # Trace an ARP packet arriving on p3, to create a MAC learning entry.
> OFPROTO_TRACE(
> [ovs-dummy],
> @@ -7574,17 +7580,18 @@ AT_CHECK_UNQUOTED([ovs-appctl fdb/show br0 | sed
> 's/[[0-9]]\{1,\}$/?/'], [0], [d
> 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}]]
> +[{
> + "br0": [
> + {
> + "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.
> @@ -7639,6 +7646,96 @@ AT_CHECK_UNQUOTED([ovs-appctl fdb/show br1 | sed
> 's/[[0-9]]\{1,\}$/?/'], [0], [d
> 5 0 50:54:00:00:00:07 ?
> ])
>
> +# Test fdb/show with multiple bridges. A "Bridge" column groups each
> +# bridge's entries, printing the name only on the bridge's first row.
> +AT_CHECK_UNQUOTED([ovs-appctl fdb/show br0 br1 | sed 's/[[0-9]]\{1,\}$/?/'],
> [0], [dnl
> + Bridge port VLAN MAC Age
> + br0 2 0 50:54:00:00:00:05 ?
> + br1 5 0 50:54:00:00:00:07 ?
> +])
> +
> +# Test fdb/show without arguments: show all bridges.
> +AT_CHECK_UNQUOTED([ovs-appctl fdb/show | sed 's/[[0-9]]\{1,\}$/?/' | sort],
> [0], [dnl
> + Bridge port VLAN MAC Age
> + br0 2 0 50:54:00:00:00:05 ?
> + br1 5 0 50:54:00:00:00:07 ?
> +])
> +
> +dnl Check json output with multiple bridges: keyed by bridge name.
> +AT_CHECK([ovs-appctl --format json --pretty fdb/show br0 br1 \
> + | sed 's/"age": [[0-9]]*/"age": ?/g'], [0], [dnl
> +[{
> + "br0": [
> + {
> + "age": ?,
> + "mac": "50:54:00:00:00:05",
> + "port": 2,
> + "vlan": 0}],
> + "br1": [
> + {
> + "age": ?,
> + "mac": "50:54:00:00:00:07",
> + "port": 5,
> + "vlan": 0}]}]
> +])
> +
> +# Test fdb/show with a non-existent bridge mixed in: must reply with an
> +# error and emit no partial output.
> +AT_CHECK([ovs-appctl fdb/show br0 nonexistent], [2], [],
> + [no such bridge
> +ovs-appctl: ovs-vswitchd: server returned an error
> +])
> +
> +OVS_VSWITCHD_STOP
> +AT_CLEANUP
> +
> +AT_SETUP([ofproto-dpif - fdb/show bridge column width])
> +OVS_VSWITCHD_START
> +AT_CHECK([ovs-vsctl -- add-br br-nexthop -- set bridge br-nexthop
> datapath-type=dummy])
> +add_of_ports br0 1 3
> +add_of_ports br-nexthop 2
> +
> +dnl br0 holds several MAC entries; br-nexthop holds one.
> +AT_CHECK([ovs-appctl fdb/add br0 p1 0 50:54:00:00:00:05])
> +AT_CHECK([ovs-appctl fdb/add br0 p3 0 50:54:00:00:00:06])
> +AT_CHECK([ovs-appctl fdb/add br-nexthop p2 0 50:54:00:00:00:07])
> +
> +dnl The "Bridge" column widens to the longest bridge name shown. A
> +dnl bridge with several MAC entries prints its name only on the first
> +dnl row; the remaining rows leave the column blank so the entries read
> +dnl as one group.
> +AT_CHECK([ovs-appctl fdb/show br0 br-nexthop], [0], [dnl
> + Bridge port VLAN MAC Age
> + br0 1 0 50:54:00:00:00:05 static
> + 3 0 50:54:00:00:00:06 static
> + br-nexthop 2 0 50:54:00:00:00:07 static
> +])
> +
> +dnl A bridge with no entries still shows a name-only row so it stays
> +dnl visible in a multi-bridge listing (matching JSON, which lists []).
> +AT_CHECK([ovs-vsctl -- add-br br-empty -- set bridge br-empty
> datapath-type=dummy])
> +AT_CHECK([ovs-appctl fdb/show br0 br-empty], [0], [dnl
> + Bridge port VLAN MAC Age
> + br0 1 0 50:54:00:00:00:05 static
> + 3 0 50:54:00:00:00:06 static
> + br-empty
> +])
> +AT_CHECK([ovs-appctl --format json --pretty fdb/show br0 br-empty], [0], [dnl
> +[{
> + "br-empty": [],
> + "br0": [
> + {
> + "mac": "50:54:00:00:00:05",
> + "port": 1,
> + "static": true,
> + "vlan": 0},
> + {
> + "mac": "50:54:00:00:00:06",
> + "port": 3,
> + "static": true,
> + "vlan": 0}]}]
> +])
> +
> OVS_VSWITCHD_STOP
> AT_CLEANUP
>
> @@ -8197,27 +8294,28 @@ AT_CHECK_UNQUOTED([ovs-appctl fdb/show br0 | sed 's/
> *[[0-9]]\{1,\}$//' | grep -
> 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}]]
> +[{
> + "br0": [
> + {
> + "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.
> diff --git a/vswitchd/ovs-vswitchd.8.in b/vswitchd/ovs-vswitchd.8.in
> index b21b1f08e893..3d80bceb655b 100644
> --- a/vswitchd/ovs-vswitchd.8.in
> +++ b/vswitchd/ovs-vswitchd.8.in
> @@ -176,10 +176,16 @@ Deletes \fImac\fR address from a \fIport\fR and
> \fIvlan\fR on a \fIbridge\fR.
> .IP "\fBfdb/flush\fR [\fIbridge\fR]"
> Flushes \fIbridge\fR MAC address learning table, or all learning tables
> if no \fIbridge\fR is given.
> -.IP "\fBfdb/show\fR \fIbridge\fR"
> +.IP "\fBfdb/show\fR [\fIbridge\fR...]"
> Lists each MAC address/VLAN pair learned by the specified \fIbridge\fR,
> along with the port on which it was learned and the age of the entry,
> -in seconds.
> +in seconds. Multiple bridge names may be given to query several bridges
> +in a single call, and if no bridge is given the tables of all bridges are
> +shown. When more than one bridge is shown, text output gains a leading
> +"Bridge" column that groups each bridge's entries, printing the bridge
> +name only on its first row; a bridge with no entries is listed as a
> +name-only row. JSON output is an object keyed by bridge name, whose
> +values are arrays of that bridge's entries.
> .IP "\fBfdb/stats-clear\fR [\fIbridge\fR]"
> Clear \fIbridge\fR MAC address learning table statistics, or all
> statistics if no \fIbridge\fR is given.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
