On 30/04/2025 16:43, Eelco Chaudron wrote:
>
>
> On 28 Apr 2025, at 9:32, Eelco Chaudron wrote:
>
>> On 27 Apr 2025, at 11:59, Roi Dayan wrote:
>>
>>> On 25/04/2025 14:42, Eelco Chaudron wrote:
>>>>
>>>>
>>>> On 22 Apr 2025, at 15:56, Roi Dayan wrote:
>>>>
>>>>> From: Dima Chumak <dchu...@nvidia.com>
>>>>
>>>> Hi Roi/Dima,
>>>>
>>>> See some more comments below.
>>>>
>>>> Cheers,
>>>>
>>>> Eelco
>>
>> See replies inline. +Ilya to get some feedback on “nexthops” json container.
>>
>> //Eelco
>>
>>>>
>>>>> The 'ovs/route/show' command now supports machine-readable JSON output
>>>>> in addition to the plain-text output for humans.
>>>>> Align the keys to match ip route output so also update the command
>>>>> help output from mask to mask.
>>>>>
>>>>> An example json output would be:
>>>>>
>>>>> ovs-appctl --format json --pretty ovs/route/show
>>>>> [
>>>>> {
>>>>> "dev": "lo",
>>>>> "dst": "::1",
>>>>> "local": false,
>>>>> "mask": 128,
>>>>> "prefsrc": "::1",
>>>>> "priority": 128,
>>>>> "user": false},
>>>>> {
>>>>> "dev": "eth1",
>>>>> "dst": "10.237.157.103",
>>>>> "local": true,
>>>>> "mask": 128,
>>>>> "prefsrc": "10.237.157.103",
>>>>> "priority": 192,
>>>>> "user": false},
>>>>> {
>>>>> "dev": "docker0",
>>>>> "dst": "fe80::42:67ff:fe28:188",
>>>>> "local": true,
>>>>> "mask": 128,
>>>>> "prefsrc": "fe80::42:67ff:fe28:188",
>>>>> "priority": 192,
>>>>> "user": false},
>>>>> {
>>>>> "dev": "eth0",
>>>>> "dst": "0.0.0.0",
>>>>> "gw": "192.168.121.1",
>>>>> "local": false,
>>>>> "mask": 96,
>>>>> "prefsrc": "192.168.121.203",
>>>>> "priority": 96,
>>>>> "user": false}]
>>>>>
>>>>> Signed-off-by: Dima Chumak <dchu...@nvidia.com>
>>>>> Reviewed-by: Roi Dayan <r...@nvidia.com>
>>>>> ---
>>>>>
>>>>> Notes:
>>>>> v3
>>>>> - Update ovs-router.at test.
>>>>> - Update example output in the commit msg.
>>>>>
>>>>> v2
>>>>> - Don't prefix boolean with "is_". Use key "local" instead of
>>>>> "is_local".
>>>>> - Add "user" key.
>>>>> - Use ds_clear() in the loop and ds_destroy() outside the loop to
>>>>> avoid
>>>>> repeated malloc/free.
>>>>> - Align json keys to ip route command output.
>>>>>
>>>>> lib/ovs-router.c | 113 +++++++++++++++++++++++++++++++++++---------
>>>>> tests/ovs-router.at | 41 +++++++++++++++-
>>>>> 2 files changed, 131 insertions(+), 23 deletions(-)
>>>>>
>>>>> diff --git a/lib/ovs-router.c b/lib/ovs-router.c
>>>>> index d955a3a543b8..11827bf3c72b 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"
>>>>> @@ -429,7 +430,7 @@ ovs_router_add(struct unixctl_conn *conn, int argc,
>>>>> is_ipv6 = true;
>>>>> } else {
>>>>> unixctl_command_reply_error(conn,
>>>>> - "Invalid 'ip/plen' parameter");
>>>>> + "Invalid 'ip/mask' parameter");
>>>>> return;
>>>>> }
>>>>>
>>>>> @@ -509,44 +510,112 @@ 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;
>>>>> + struct ds ds;
>>>>> + 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) {
>>>>> + bool user = rt->priority != rt->plen && !rt->local;
>>>>> + struct json *json = json_object_create();
>>>>> +
>>>>> + json_object_put(json, "user", json_boolean_create(user));
>>>>> + json_object_put(json, "local", json_boolean_create(rt->local));
>>>>> + json_object_put(json, "priority",
>>>>> json_integer_create(rt->priority));
>>>>> + json_object_put(json, "mask", json_integer_create(rt->plen));
>>>>
>>>> See my previous comment, the plen needs to be normalized.
>>>> if (IN6_IS_ADDR_V4MAPPED(&rt->nw_addr)) plen -= 96.
>>>>
>>>
>>> Hi, It was part of the reply that since this is a json output for scripts
>>> we thought to keep it raw data. Also since the output is in a different
>>> key prefix_length. unlike the text output showing nw_addr/plen is also a
>>> reason maybe to keep this as original plen. thoughts?
>>
>> The plen is odd because internally we store everything in IPv6 format. So we
>> need to adjust it before making it available to the outside world. Or else
>> IPv4 addresses will have a prefix of 120 which makes no sense.
>>
>>>> Also thinking about this again, mask is not the right name, as it’s
>>>> actually the prefix length. So I would change it to ‘prefix_length’
>>>> instead.
>>>>
>>>
>>> Ack. Renamed to 'prefix_length' in the json output and I'll revert to the
>>> original 'plen' in the command help.
>>
>> I would just call it prefix in the command help.
>>
>>>>> + json_object_put_string(json, "dev", rt->output_netdev);
>>>>> +
>>>>> + ds_init(&ds);
>>>>
>>>> As you already mentioned this ds_init (and others), need to move outside
>>>> the loop, and be replaced with a ds_clear().
>>>
>>> Ack. outside a single ds_init() and removed all of the calls from the loop.
>>> ds_clear() already exists next to each ds_init() so didn't need a replace.
>>>
>>>>
>>>>> + ipv6_format_mapped(&rt->nw_addr, &ds);
>>>>> + json_object_put_string(json, "dst", ds_cstr_ro(&ds));
>>>>> + ds_clear(&ds);
>>>>
>>>>> + ds_init(&ds);
>>>>> + ipv6_format_mapped(&rt->src_addr, &ds);
>>>>> + json_object_put_string(json, "prefsrc", ds_cstr_ro(&ds));
>>>>> + ds_clear(&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_clear(&ds);
>>>>> + }
>>>>> +
>>>>> + json_entries[i++] = json;
>>>>> + }
>>>>> +
>>>>> + ds_destroy(&ds);
>>>>> +
>>>>> +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
>>>>> @@ -619,12 +688,12 @@ ovs_router_init(void)
>>>>> fatal_signal_add_hook(ovs_router_flush_handler, NULL, NULL,
>>>>> true);
>>>>> classifier_init(&cls, NULL);
>>>>> unixctl_command_register("ovs/route/add",
>>>>> - "ip/plen output_netdev [gw] "
>>>>> + "ip/mask dev [gw] "
>>>>> "[pkt_mark=mark] [src=src_ip]",
>>>>> 2, 5, ovs_router_add, NULL);
>>>>> unixctl_command_register("ovs/route/show", "", 0, 0,
>>>>> ovs_router_show, NULL);
>>>>> - unixctl_command_register("ovs/route/del", "ip/plen "
>>>>> + unixctl_command_register("ovs/route/del", "ip/mask "
>>>>> "[pkt_mark=mark]", 1, 2, ovs_router_del,
>>>>> NULL);
>>>>> unixctl_command_register("ovs/route/lookup", "ip_addr "
>>>>> diff --git a/tests/ovs-router.at b/tests/ovs-router.at
>>>>> index b3314b3dff0d..6f2378666e7e 100644
>>>>> --- a/tests/ovs-router.at
>>>>> +++ b/tests/ovs-router.at
>>>>> @@ -16,7 +16,7 @@ Error while inserting route.
>>>>> ovs-appctl: ovs-vswitchd: server returned an error
>>>>> ])
>>>>> AT_CHECK([ovs-appctl ovs/route/add 1.1.foo.bar/24 br0 2.2.2.10], [2],
>>>>> [], [dnl
>>>>> -Invalid 'ip/plen' parameter
>>>>> +Invalid 'ip/mask' parameter
>>>>> ovs-appctl: ovs-vswitchd: server returned an error
>>>>> ])
>>>>> AT_CHECK([ovs-appctl ovs/route/add 2.2.2.4/24 br0 pkt_mark=baz], [2],
>>>>> [], [dnl
>>>>> @@ -28,6 +28,45 @@ 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
>>>>> +[[
>>>>> + {
>>>>> + "dev": "br0",
>>>>> + "dst": "2.2.2.0",
>>>>> + "local": true,
>>>>> + "mask": 120,
>>>>> + "prefsrc": "2.2.2.2",
>>>>> + "priority": 184,
>>>>> + "user": false},
>>>>> + {
>>>>> + "dev": "br0",
>>>>> + "dst": "1.1.1.0",
>>>>> + "gw": "2.2.2.10",
>>>>> + "local": false,
>>>>> + "mask": 120,
>>>>> + "prefsrc": "2.2.2.2",
>>>>> + "priority": 152,
>>>>> + "user": true},
>>>>> + {
>>>>> + "dev": "br0",
>>>>> + "dst": "1.1.2.0",
>>>>> + "gw": "2.2.2.10",
>>>>> + "local": false,
>>>>> + "mark": 2,
>>>>> + "mask": 120,
>>>>> + "prefsrc": "2.2.2.2",
>>>>> + "priority": 152,
>>>>> + "user": true},
>>>>> + {
>>>>> + "dev": "br0",
>>>>> + "dst": "2.2.2.3",
>>>>> + "local": false,
>>>>> + "mark": 1,
>>>>> + "mask": 128,
>>>>> + "prefsrc": "2.2.2.2",
>>>>> + "priority": 160,
>>>>> + "user": true}]]
>>>>> +])
>>>>
>>>> I think you missed the following comment (I’ve added a more explanatory
>>>> example. Note that gw is changed to gateway):
>>>>
>>>> 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": "1.1.1.0",
>>>> "local": false,
>>>> "mask": 120,
>>>> "prefsrc": "2.2.2.2",
>>>> "priority": 152,
>>>> "user": true,
>>>> "nexthops": [
>>>> {
>>>> "gateway": "2.2.2.10",
>>>> "dev": "br0"
>>>> }
>>>> ]
>>>> }
>>>>
>>>
>>> I thought the comment was general why to keep consistent naming with ip
>>> output.
>>> If we already wan to match the ip command output then it also doesn't have
>>> the
>>> nexthops object if it's only a single nexthop. The dev and gateway are at
>>> the top
>>> level of the json. So only in the future when ovs_router_entry will support
>>> multiple nexthops then we should add a nexthops json object to output all
>>> nexthops
>>> entries.
>>
>> I think we should introduce the nexthops object now, so the json logic can
>> stay the same in future versions. It does not make sense for a json parser
>> to differentiate between 1 or multiple nexthops. But let’s get some other
>> optinions. Ilya, or others?
>
> I had a quick offline discussion on this subject, and it would be best to
> create a consistent JSON output that is future-proof. Including the next hop
> objects in the nexthops JSON container, regardless of how many next hops
> exist, would be the best way forward.
>
ack. I'll send v5 with all suggestions.
thanks.
>>> I'll update though 'gw' to 'gateway'.
>>>
>>> I'll send v4 with all other modifications.
>>
>> I’d suggest holding off on sending new revisions until the ongoing
>> discussions have settled, that way we can avoid too many iterations.
>>
>>>>> OVS_VSWITCHD_STOP
>>>>> AT_CLEANUP
>>>>>
>>>>> --
>>>>> 2.21.0
>>>>
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
