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
> 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.
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.
> + 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().
> + 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"
}
]
}
> OVS_VSWITCHD_STOP
> AT_CLEANUP
>
> --
> 2.21.0
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
