On 20/02/2025 12:04, Eelco Chaudron wrote:
>
>
> 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.
>
Hi sorry didn't fully understand the suggested change here :)
I don't think we need to assert, checking 'i' was better.
The hmap_count is just for allocating "enough" room to prepare the output.
Using the read look and limiting to the min between i and num_entries
should be enough. if item being removed/added during this the worst
is we miss an output of an item which will appear in next dump.
The text output also uses the same read lock.
Shouldn't this be enough ? so I don't think the write lock is needed?
Also just taking the hmap_count should be ok outside the read lock.
Let me know what I miss or if you still think we should replace to rw lock
and if to move hmap_count inside.
>>>
>>>> + 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
