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.
>
>> + 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
