| 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 |
| 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 |
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",
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
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.
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 :(
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 --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])