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 <[email protected]> >>> >>> 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 <[email protected]> >>> Acked-by: Roi Dayan <[email protected]> >>> --- >>> >>> 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 >> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
