On 22/04/2025 10:12, Eelco Chaudron wrote:
>
>
> On 21 Apr 2025, at 9:08, Roi Dayan wrote:
>
>> On 17/04/2025 13:30, Eelco Chaudron wrote:
>>>
>>>
>>> On 7 Apr 2025, at 12:23, Roi Dayan via dev wrote:
>>>
>>>> From: Dima Chumak <dchu...@nvidia.com>
>>>>
>>>> The 'ovs/route/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 ovs/route/show
>>>> [
>>>> {
>>>> "is_local": false,
>>>> "nw_addr": "::1",
>>>> "output_netdev": "lo",
>>>> "plen": 128,
>>>> "priority": 128,
>>>> "src_addr": "::1"},
>>>> {
>>>> "is_local": true,
>>>> "nw_addr": "10.237.157.103",
>>>> "output_netdev": "eth1",
>>>> "plen": 128,
>>>> "priority": 192,
>>>> "src_addr": "10.237.157.103"},
>>>> {
>>>> "is_local": true,
>>>> "nw_addr": "fe80::42:67ff:fe28:188",
>>>> "output_netdev": "docker0",
>>>> "plen": 128,
>>>> "priority": 192,
>>>> "src_addr": "fe80::42:67ff:fe28:188"},
>>>> {
>>>> "gw": "192.168.121.1",
>>>> "is_local": false,
>>>> "nw_addr": "0.0.0.0",
>>>> "output_netdev": "eth0",
>>>> "plen": 96,
>>>> "priority": 96,
>>>> "src_addr": "192.168.121.203"}]
>>>>
>>>> Signed-off-by: Dima Chumak <dchu...@nvidia.com>
>>>> Reviewed-by: Roi Dayan <r...@nvidia.com>
>>>
>>> Hi Dima, thanks for the patch. See some comments below.
>>>
>>> Cheers,
>>>
>>> Eelco
>>>
>>>> ---
>>>> lib/ovs-router.c | 103 ++++++++++++++++++++++++++++++++++++--------
>>>> tests/ovs-router.at | 35 +++++++++++++++
>>>> 2 files changed, 119 insertions(+), 19 deletions(-)
>>>>
>>>> diff --git a/lib/ovs-router.c b/lib/ovs-router.c
>>>> index d955a3a543b8..eb04edece9cb 100644
>>>> --- a/lib/ovs-router.c
>>>> +++ b/lib/ovs-router.c
>>>> @@ -36,6 +36,7 @@
>>>> #include "dpif.h"
>>>> #include "fatal-signal.h"
>>>> #include "openvswitch/dynamic-string.h"
>>>> +#include "openvswitch/json.h"
>>>> #include "netdev.h"
>>>> #include "packets.h"
>>>> #include "seq.h"
>>>> @@ -509,44 +510,108 @@ ovs_router_del(struct unixctl_conn *conn, int argc
>>>> OVS_UNUSED,
>>>> }
>>>>
>>>> static void
>>>> -ovs_router_show(struct unixctl_conn *conn, int argc OVS_UNUSED,
>>>> - const char *argv[] OVS_UNUSED, void *aux OVS_UNUSED)
>>>> +ovs_router_show_json(struct json **routes)
>>>> {
>>>> + struct json **json_entries = NULL;
>>>> struct ovs_router_entry *rt;
>>>> - struct ds ds = DS_EMPTY_INITIALIZER;
>>>> + int i = 0;
>>>>
>>>> - ds_put_format(&ds, "Route Table:\n");
>>>> - CLS_FOR_EACH(rt, cr, &cls) {
>>>> + if (!cls.n_rules) {
>>>> + goto out;
>>>> + }
>>>> +
>>>> + json_entries = xmalloc(cls.n_rules * sizeof *json_entries);
>>>> +
>>>> + CLS_FOR_EACH (rt, cr, &cls) {
>>>> + struct json *json = json_object_create();
>>>> + struct ds ds;
>>>> +
>>>
>>> In the text output, we show if it’s a user-configured entry; Maybe we
>>> should do the same for json? Not sure about the name, but something like:
>>>
>>> json_object_put(json, “user_configured”, json_boolean_create((rt->priority
>>> != rt->plen && !rt->local)));
>>
>> added "user".
>>>
>>>> + json_object_put(json, "is_local", json_boolean_create(rt->local));
>>
>> ack.
>>
>>>
>>> I would keep the name "local" as looking at the existing code, we don’t
>>> seem to add prefixes for booleans, just the variable name.
>>>
>>>> + json_object_put(json, "priority",
>>>> json_integer_create(rt->priority));
>>>> + json_object_put(json, "plen", json_integer_create(rt->plen));
>>>
>>> In the text code the plen is normalized based on the network address type.
>>> Should we do the same here?
>>> Also, the plen is the network mask, so should we add it to the nw_addr
>>> instead?
>>>
>>
>> The purpose for json here is for scripts/sw and not user. so "raw" data will
>> be easier to handle
>> instead of parsing the string of ip/mask in the script later.
>
> Then I would at least export it as ‘mask’ so it’s clear what we’re talking
> about.
>
ack.
I'll send v2 for comments.
>>>> + json_object_put_string(json, "output_netdev", rt->output_netdev);
>>>> +
>>>> + ds_init(&ds);
>>>> + ipv6_format_mapped(&rt->nw_addr, &ds);
>>>> + json_object_put_string(json, "nw_addr", ds_cstr_ro(&ds));
>>>> + ds_destroy(&ds);
>>>> +
>>>> + ds_init(&ds);
>>>> + ipv6_format_mapped(&rt->src_addr, &ds);
>>>> + json_object_put_string(json, "src_addr", ds_cstr_ro(&ds));
>>>> + ds_destroy(&ds);
>>>> +
>>>> + if (rt->mark) {
>>>> + json_object_put(json, "mark", json_integer_create(rt->mark));
>>>> + }
>>>> +
>>>> + if (ipv6_addr_is_set(&rt->gw)) {
>>>> + ds_init(&ds);
>>>> + ipv6_format_mapped(&rt->gw, &ds);
>>>> + json_object_put_string(json, "gw", ds_cstr_ro(&ds));
>>>> + ds_destroy(&ds);
>>>> + }
>>>> +
>>>
>>> Rather than calling ds_destroy() each time, consider calling ds_clear()
>>> instead, and only call ds_destroy() once at the end. This avoids repeated
>>> malloc/free calls. Preferably move it outside the CLS_FOR_EACH() loop.
>>>
>>
>> ack.
>>
>>>> + json_entries[i++] = json;
>>>> + }
>>>> +
>>>> +out:
>>>> + *routes = json_array_create(json_entries, i);
>>>> +}
>>>> +
>>>> +static void
>>>> +ovs_router_show_text(struct ds *ds)
>>>> +{
>>>> + struct ovs_router_entry *rt;
>>>> +
>>>> + ds_put_format(ds, "Route Table:\n");
>>>> + CLS_FOR_EACH (rt, cr, &cls) {
>>>> uint8_t plen;
>>>> if (rt->priority == rt->plen || rt->local) {
>>>> - ds_put_format(&ds, "Cached: ");
>>>> + ds_put_format(ds, "Cached: ");
>>>> } else {
>>>> - ds_put_format(&ds, "User: ");
>>>> + ds_put_format(ds, "User: ");
>>>> }
>>>> - ipv6_format_mapped(&rt->nw_addr, &ds);
>>>> + ipv6_format_mapped(&rt->nw_addr, ds);
>>>> plen = rt->plen;
>>>> if (IN6_IS_ADDR_V4MAPPED(&rt->nw_addr)) {
>>>> plen -= 96;
>>>> }
>>>> - ds_put_format(&ds, "/%"PRIu8, plen);
>>>> + ds_put_format(ds, "/%"PRIu8, plen);
>>>> if (rt->mark) {
>>>> - ds_put_format(&ds, " MARK %"PRIu32, rt->mark);
>>>> + ds_put_format(ds, " MARK %"PRIu32, rt->mark);
>>>> }
>>>>
>>>> - ds_put_format(&ds, " dev %s", rt->output_netdev);
>>>> + ds_put_format(ds, " dev %s", rt->output_netdev);
>>>> if (ipv6_addr_is_set(&rt->gw)) {
>>>> - ds_put_format(&ds, " GW ");
>>>> - ipv6_format_mapped(&rt->gw, &ds);
>>>> + ds_put_format(ds, " GW ");
>>>> + ipv6_format_mapped(&rt->gw, ds);
>>>> }
>>>> - ds_put_format(&ds, " SRC ");
>>>> - ipv6_format_mapped(&rt->src_addr, &ds);
>>>> + ds_put_format(ds, " SRC ");
>>>> + ipv6_format_mapped(&rt->src_addr, ds);
>>>> if (rt->local) {
>>>> - ds_put_format(&ds, " local");
>>>> + ds_put_format(ds, " local");
>>>> }
>>>> - ds_put_format(&ds, "\n");
>>>> + ds_put_format(ds, "\n");
>>>> + }
>>>> +}
>>>> +
>>>> +static void
>>>> +ovs_router_show(struct unixctl_conn *conn, int argc OVS_UNUSED,
>>>> + const char *argv[] OVS_UNUSED, void *aux OVS_UNUSED)
>>>> +{
>>>> + if (unixctl_command_get_output_format(conn) ==
>>>> UNIXCTL_OUTPUT_FMT_JSON) {
>>>> + struct json *routes;
>>>> +
>>>> + ovs_router_show_json(&routes);
>>>> + unixctl_command_reply_json(conn, routes);
>>>> + } else {
>>>> + struct ds ds = DS_EMPTY_INITIALIZER;
>>>> +
>>>> + ovs_router_show_text(&ds);
>>>> + unixctl_command_reply(conn, ds_cstr(&ds));
>>>> + ds_destroy(&ds);
>>>> }
>>>> - unixctl_command_reply(conn, ds_cstr(&ds));
>>>> - ds_destroy(&ds);
>>>> }
>>>>
>>>> static void
>>>> diff --git a/tests/ovs-router.at b/tests/ovs-router.at
>>>> index b3314b3dff0d..13a38eb55055 100644
>>>> --- a/tests/ovs-router.at
>>>> +++ b/tests/ovs-router.at
>>>> @@ -28,6 +28,41 @@ User: 1.1.1.0/24 dev br0 GW 2.2.2.10 SRC 2.2.2.2
>>>> User: 1.1.2.0/24 MARK 2 dev br0 GW 2.2.2.10 SRC 2.2.2.2
>>>> User: 2.2.2.3/32 MARK 1 dev br0 SRC 2.2.2.2
>>>> ])
>>>> +AT_CHECK([ovs-appctl --format=json --pretty ovs/route/show], [0], [dnl
>>>> +[[
>>>> + {
>>>> + "is_local": true,
>>>> + "nw_addr": "2.2.2.0",
>>>> + "output_netdev": "br0",
>>>> + "plen": 120,
>>>> + "priority": 184,
>>>> + "src_addr": "2.2.2.2"},
>>>
>>> On json naming, should we align a bit more to the ‘ip [—6] -json’ route
>>> show output?
>>>
>>> nw_addr -> dst
>>> src_addr -> prefsrc
>>> output_netlink -> dev
>>>
>>> Maybe we should also be prepared for multiple next hops, so if a gw is
>>> present, we should do something like the below (for gateway and dev), so we
>>> do not have to change the format later;
>>>
>>> {
>>> "dst": "default",
>>> "protocol": "ra",
>>> "metric": 100,
>>> "flags": [],
>>> "pref": "medium",
>>> "nexthops": [
>>> {
>>> "gateway": "fe80::4a5a:d01:9431:5120",
>>> "dev": "eno8303",
>>> "weight": 1,
>>> "flags": []
>>> },
>>> {
>>> "gateway": "fe80::4a5a:d01:942f:2b20",
>>> "dev": "eno8303",
>>> "weight": 1,
>>> "flags": []
>>> }
>>> ]
>>> },
>>>
>>>
>>
>> It's possible but the help string for ovs/route/add uses the attribute
>> naming.
>>
>> ovs/route/add ip/plen output_netdev [gw] [pkt_mark=mark]
>> [src=src_ip]
>>
>> So this was done to be consistent. do you still prefer to update ?
>> if so, do we need to udpate the help print? output_netdev to dev, ip to
>> nw_addr, src_ip to src_addr..
>
> I guess it’s only two parameters that will change, which I think is fine. So
> it will become:
>
> -ovs/route/add ip/plen output_netdev [gw] [pkt_mark=mark]
> [src=src_ip]
> +ovs/route/add ip/mask dev [gw] [pkt_mark=mark]
> [src=src_ip]
>
ack.
>>>> + {
>>>> + "gw": "2.2.2.10",
>>>> + "is_local": false,
>>>> + "nw_addr": "1.1.1.0",
>>>> + "output_netdev": "br0",
>>>> + "plen": 120,
>>>> + "priority": 152,
>>>> + "src_addr": "2.2.2.2"},
>>>> + {
>>>> + "gw": "2.2.2.10",
>>>> + "is_local": false,
>>>> + "mark": 2,
>>>> + "nw_addr": "1.1.2.0",
>>>> + "output_netdev": "br0",
>>>> + "plen": 120,
>>>> + "priority": 152,
>>>> + "src_addr": "2.2.2.2"},
>>>> + {
>>>> + "is_local": false,
>>>> + "mark": 1,
>>>> + "nw_addr": "2.2.2.3",
>>>> + "output_netdev": "br0",
>>>> + "plen": 128,
>>>> + "priority": 160,
>>>> + "src_addr": "2.2.2.2"}]]
>>>> +])
>>>> OVS_VSWITCHD_STOP
>>>> AT_CLEANUP
>>>>
>>>> --
>>>> 2.21.0
>>>>
>>>> _______________________________________________
>>>> dev mailing list
>>>> d...@openvswitch.org
>>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>>
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
