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.
>>> + 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]
>>> + {
>>> + "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
