On 5 May 2025, at 10:46, Roi Dayan wrote:
> From: Dima Chumak <[email protected]> > > 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 <[email protected]> > Reviewed-by: Roi Dayan <[email protected]> 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 > --- > > 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 [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
