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'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
