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)));
> + json_object_put(json, "is_local", json_boolean_create(rt->local));
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?
> + 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.
> + 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": []
}
]
},
> + {
> + "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
