On 16/05/2025 11:44, Eelco Chaudron wrote:
>
>
> On 5 May 2025, at 10:46, Roi Dayan 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.
>> Align the keys to match ip route output.
>>
>> An example json output would be:
>>
>> ovs-appctl --format json --pretty ovs/route/show
>> [
>> {
>> "dst": "::1",
>> "local": false,
>> "nexthops": [
>> {
>> "dev": "lo"}],
>> "prefix": 128,
>> "prefsrc": "::1",
>> "priority": 128,
>> "user": false},
>> {
>> "dst": "10.237.157.103",
>> "local": true,
>> "nexthops": [
>> {
>> "dev": "eth1"}],
>> "prefix": 32,
>> "prefsrc": "10.237.157.103",
>> "priority": 192,
>> "user": false},
>> {
>> "dst": "fe80::42:67ff:fe28:188",
>> "local": true,
>> "nexthops": [
>> {
>> "dev": "docker0"}],
>> "prefix": 128,
>> "prefsrc": "fe80::42:67ff:fe28:188",
>> "priority": 192,
>> "user": false},
>> {
>> "dst": "0.0.0.0",
>> "local": false,
>> "nexthops": [
>> {
>> "dev": "eth0",
>> "gateway": "192.168.121.1"}],
>> "prefix": 0,
>> "prefsrc": "192.168.121.203",
>> "priority": 96,
>> "user": false}]
>>
>> Signed-off-by: Dima Chumak <dchu...@nvidia.com>
>> Reviewed-by: Roi Dayan <r...@nvidia.com>
>
> Hi Roi/Dima,
>
> I reviewed the changes, and they look good to me. However, when reviewing the
> entire patch, I noticed that the route table is RCU-based, so the size could,
> in theory, change while looping through it. To avoid writing too many entries
> to the JSON array, we need to add some protection. This is the change I
> suggest:
>
> diff --git a/lib/ovs-router.c b/lib/ovs-router.c
> index 5e6949ef3..fabde1b3e 100644
> --- a/lib/ovs-router.c
> +++ b/lib/ovs-router.c
> @@ -512,16 +512,17 @@ ovs_router_del(struct unixctl_conn *conn, int argc
> OVS_UNUSED,
> static void
> ovs_router_show_json(struct json **routes)
> {
> + int n_rules = classifier_count(&cls);
> struct json **json_entries = NULL;
> struct ovs_router_entry *rt;
> struct ds ds;
> int i = 0;
>
> - if (!cls.n_rules) {
> + if (!n_rules) {
> goto out;
> }
>
> - json_entries = xmalloc(cls.n_rules * sizeof *json_entries);
> + json_entries = xmalloc(n_rules * sizeof *json_entries);
> ds_init(&ds);
>
> CLS_FOR_EACH (rt, cr, &cls) {
> @@ -530,6 +531,10 @@ ovs_router_show_json(struct json **routes)
> struct json *nh = json_object_create();
> uint8_t plen = rt->plen;
>
> + if (i >= n_rules) {
> + break;
> + }
> +
> if (IN6_IS_ADDR_V4MAPPED(&rt->nw_addr)) {
> plen -= 96;
> }
>
> Let me know what you think. If you agree, I can wrap this in when applying
> the patch. I’ll probably do this by the end of next week to give others a
> chance to review.
>
> Cheers,
>
> Eelco
>
Hi, sure. this looks good. thanks.
>> ---
>>
>> Notes:
>> v5
>> - Rename 'prefix_length' to 'prefix'.
>> - Normalize prefix for ipv4.
>> - Add nexthops json object even though its a single entry.
>>
>> v4
>> - Fix memleak by moving ds_init() call to once outside the loop.
>> - Update 'prefix_length' key.
>> - Rename 'gw' to 'gateway' key.
>>
>> 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 | 114 ++++++++++++++++++++++++++++++++++++--------
>> tests/ovs-router.at | 47 ++++++++++++++++++
>> 2 files changed, 141 insertions(+), 20 deletions(-)
>>
>> diff --git a/lib/ovs-router.c b/lib/ovs-router.c
>> index d955a3a543b8..5e6949ef3acb 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,117 @@ 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);
>> + ds_init(&ds);
>> +
>> + CLS_FOR_EACH (rt, cr, &cls) {
>> + bool user = rt->priority != rt->plen && !rt->local;
>> + struct json *json = json_object_create();
>> + struct json *nh = json_object_create();
>> + uint8_t plen = rt->plen;
>> +
>> + if (IN6_IS_ADDR_V4MAPPED(&rt->nw_addr)) {
>> + plen -= 96;
>> + }
>> +
>> + 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, "prefix", json_integer_create(plen));
>> + json_object_put_string(nh, "dev", rt->output_netdev);
>> +
>> + ipv6_format_mapped(&rt->nw_addr, &ds);
>> + json_object_put_string(json, "dst", ds_cstr_ro(&ds));
>> + ds_clear(&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)) {
>> + ipv6_format_mapped(&rt->gw, &ds);
>> + json_object_put_string(nh, "gateway", ds_cstr_ro(&ds));
>> + ds_clear(&ds);
>> + }
>> +
>> + json_object_put(json, "nexthops", json_array_create_1(nh));
>> + 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,7 +693,7 @@ 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/plen dev [gw] "
>> "[pkt_mark=mark] [src=src_ip]",
>> 2, 5, ovs_router_add, NULL);
>> unixctl_command_register("ovs/route/show", "", 0, 0,
>> diff --git a/tests/ovs-router.at b/tests/ovs-router.at
>> index b3314b3dff0d..641b780a582a 100644
>> --- a/tests/ovs-router.at
>> +++ b/tests/ovs-router.at
>> @@ -28,6 +28,53 @@ 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
>> +[[
>> + {
>> + "dst": "2.2.2.0",
>> + "local": true,
>> + "nexthops": [
>> + {
>> + "dev": "br0"}],
>> + "prefix": 24,
>> + "prefsrc": "2.2.2.2",
>> + "priority": 184,
>> + "user": false},
>> + {
>> + "dst": "1.1.1.0",
>> + "local": false,
>> + "nexthops": [
>> + {
>> + "dev": "br0",
>> + "gateway": "2.2.2.10"}],
>> + "prefix": 24,
>> + "prefsrc": "2.2.2.2",
>> + "priority": 152,
>> + "user": true},
>> + {
>> + "dst": "1.1.2.0",
>> + "local": false,
>> + "mark": 2,
>> + "nexthops": [
>> + {
>> + "dev": "br0",
>> + "gateway": "2.2.2.10"}],
>> + "prefix": 24,
>> + "prefsrc": "2.2.2.2",
>> + "priority": 152,
>> + "user": true},
>> + {
>> + "dst": "2.2.2.3",
>> + "local": false,
>> + "mark": 1,
>> + "nexthops": [
>> + {
>> + "dev": "br0"}],
>> + "prefix": 32,
>> + "prefsrc": "2.2.2.2",
>> + "priority": 160,
>> + "user": true}]]
>> +])
>> OVS_VSWITCHD_STOP
>> AT_CLEANUP
>>
>> --
>> 2.21.0
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
