On 22 Feb 2023, at 11:29, Nobuhiro MIKI wrote:
> This patch cleans up the parser to accept pkt_mark and gw in any order. > > pkt_mark and gw are normally expected to be specified exactly once. > However, as with other tools, if specified multiple times, the last > specification is used. Also, pkt_mark and gw have separate prefix > strings so they can be parsed in any order. > > Reviewed-by: Simon Horman <[email protected]> > Signed-off-by: Nobuhiro MIKI <[email protected]> Two small nits below, maybe Ilya can fold them in when applying? The rest looks good to me. Acked-by: Eelco Chaudron <[email protected]> > --- > lib/ovs-router.c | 50 ++++++++++++++++++++++++--------------------- > tests/ovs-router.at | 27 ++++++++++++++++++++---- > 2 files changed, 50 insertions(+), 27 deletions(-) > > diff --git a/lib/ovs-router.c b/lib/ovs-router.c > index 5d0fbd503e9e..6c5faf46ea15 100644 > --- a/lib/ovs-router.c > +++ b/lib/ovs-router.c > @@ -345,41 +345,45 @@ ovs_router_add(struct unixctl_conn *conn, int argc, > struct in6_addr ip6; > uint32_t mark = 0; > unsigned int plen; > + ovs_be32 gw = 0; > + bool is_ipv6; > ovs_be32 ip; > int err; > + int i; > > if (scan_ipv4_route(argv[1], &ip, &plen)) { > - ovs_be32 gw = 0; > - > - if (argc > 3) { > - if (!ovs_scan(argv[3], "pkt_mark=%"SCNi32, &mark) && > - !ip_parse(argv[3], &gw)) { > - unixctl_command_reply_error(conn, "Invalid pkt_mark or > gateway"); > - return; > - } > - } > in6_addr_set_mapped_ipv4(&ip6, ip); > - if (gw) { > - in6_addr_set_mapped_ipv4(&gw6, gw); > - } > plen += 96; > + is_ipv6 = false; > } else if (scan_ipv6_route(argv[1], &ip6, &plen)) { > - if (argc > 3) { > - if (!ovs_scan(argv[3], "pkt_mark=%"SCNi32, &mark) && > - !ipv6_parse(argv[3], &gw6)) { > - unixctl_command_reply_error(conn, "Invalid pkt_mark or IPv6 > gateway"); > - return; > - } > - } > + is_ipv6 = true; > } else { > unixctl_command_reply_error(conn, "Invalid parameters"); With the above changes this error can be more specific, i.e. “Invalid ‘ip_addr/prefix_len’ parameter”. > return; > } > - if (argc > 4) { > - if (!ovs_scan(argv[4], "pkt_mark=%"SCNi32, &mark)) { > - unixctl_command_reply_error(conn, "Invalid pkt_mark"); > - return; > + > + /* Parse optional parameters. */ > + for (i = 3; i < argc; i++) { > + if (ovs_scan(argv[i], "pkt_mark=%"SCNi32, &mark)) { > + continue; > } > + > + if (is_ipv6) { > + if (ipv6_parse(argv[i], &gw6)) { > + continue; > + } > + } else { > + if (ip_parse(argv[i], &gw)) { > + continue; > + } > + } > + > + unixctl_command_reply_error(conn, "Invalid parameters"); We can re-use the removed error message here, i.e. “Invalid pkt_mark or IP gateway” > + return; > + } > + > + if (gw) { > + in6_addr_set_mapped_ipv4(&gw6, gw); > } > > err = ovs_router_insert__(mark, plen + 32, false, &ip6, plen, argv[2], > &gw6); > diff --git a/tests/ovs-router.at b/tests/ovs-router.at > index 6dacc2954bc6..96fb6e188eef 100644 > --- a/tests/ovs-router.at > +++ b/tests/ovs-router.at > @@ -1,14 +1,33 @@ > AT_BANNER([ovs-router]) > > -AT_SETUP([appctl - route/add with gateway]) > +AT_SETUP([appctl - route/add with gateway and pkt_mark]) > AT_KEYWORDS([ovs_router]) > -OVS_VSWITCHD_START([add-port br0 p2 -- set Interface p2 type=gre \ > - options:local_ip=2.2.2.2 options:remote_ip=1.1.1.1 \ > - -- add-port br0 p1 -- set interface p1 type=dummy]) > +OVS_VSWITCHD_START([add-port br0 p1 -- set Interface p1 type=dummy]) > AT_CHECK([ovs-appctl netdev-dummy/ip4addr br0 2.2.2.2/24], [0], [OK > ]) > +AT_CHECK([ovs-appctl ovs/route/add 2.2.2.3/32 br0 pkt_mark=1], [0], [OK > +]) > AT_CHECK([ovs-appctl ovs/route/add 1.1.1.0/24 br0 2.2.2.10], [0], [OK > ]) > +AT_CHECK([ovs-appctl ovs/route/add 1.1.2.0/24 br0 2.2.2.10 pkt_mark=2], [0], > [OK > +]) > +AT_CHECK([ovs-appctl ovs/route/add 1.1.3.0/24 br0 pkt_mark=3], [2], [], [dnl > +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 parameters > +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 > +Invalid parameters > +ovs-appctl: ovs-vswitchd: server returned an error > +]) > +AT_CHECK([ovs-appctl ovs/route/show | grep User | sort], [0], [dnl > +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 > +]) > OVS_VSWITCHD_STOP > AT_CLEANUP > > -- > 2.31.1 > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
