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