On 23/02/2025 9:29, Roi Dayan wrote:
>
>
> 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.
>
>
I sent v4 with the changes about the safety check and ilya comment on the json
output.
Can you please check and reply on v4 if you think the rw-lock change is still
relevant
thanks
>>>>
>>>>> + 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
