On Tue, Feb 21, 2023 at 06:33:32PM +0900, Nobuhiro MIKI wrote:
> On 2023/02/21 2:23, Simon Horman wrote:
> > On Tue, Feb 14, 2023 at 12:39:05PM +0900, Nobuhiro MIKI wrote:
> >> When adding a route with ovs/route/add command, the source address
> >> in "ovs_router_entry" structure is always the FIRST address that the
> >> interface has. See "ovs_router_get_netdev_source_address"
> >> function for more information.
> >>
> >> If an interface has multiple ipv4 and/or ipv6 addresses, there are use
> >> cases where the user wants to control the source address. This patch
> >> therefore addresses this issue by adding a src parameter.
> >>
> >> Note that same constraints also exist when caching routes from
> >> Kernel FIB with Netlink, but are not dealt with in this patch.
> >>
> >> Signed-off-by: Nobuhiro MIKI <nm...@yahoo-corp.jp>
> >
> > aside: this patch was base64 encoded, which is slightly inconvenient
> > on my side (which is not important in itself). Curiously
> > the other patches in this series are not.
>
> There was a warning from git send-mail. Perhaps the modification of the man
> page might be the cause of some misjudgment. Sorry for the inconvenience.
Thanks. No need to spend any more time on the base64 question.
> >> diff --git a/lib/ovs-router.c b/lib/ovs-router.c
> >> index 5d0fbd503e9e..8f2587444034 100644
> >> --- a/lib/ovs-router.c
> >> +++ b/lib/ovs-router.c
> >> @@ -164,6 +164,47 @@ static void rt_init_match(struct match *match,
> >> uint32_t mark,
> >> match->flow.pkt_mark = mark;
> >> }
> >>
> >> +static int
> >
> > Maybe bool is a more appropriate return type for this function?
> > Likewise for ovs_router_get_netdev_source_address().
> > But perhaps that is a cleanup for another time.
>
> Yes, boolean is appropriate here. I'll fix it.
>
> > Also, there is a lot of comonality between verify_prefsrc()
> > and ovs_router_get_netdev_source_address(). I am curious to know
> > if you considered combining them, or otherwise sharing code between them?
>
> I am aware that many parts are common in those two functions.
>
> However, to my ability, the logic to search for a source address
> and the logic to verify one given source address do not merge well,
> so I have split them into two functions.
>
> It would probably be possible to put them together by dividing
> the cases according to whether the prefsrc is null or not, but
> it would complicate the logic in the loop.
Understood. There is a balance to be had between clean and
avoiding duplication.
...
> >> @@ -236,11 +278,21 @@ ovs_router_insert__(uint32_t mark, uint8_t priority,
> >> bool local,
> >> p->plen = plen;
> >> p->local = local;
> >> p->priority = priority;
> >> - err = ovs_router_get_netdev_source_address(ip6_dst, output_bridge,
> >> - &p->src_addr);
> >> - if (err && ipv6_addr_is_set(gw)) {
> >> - err = ovs_router_get_netdev_source_address(gw, output_bridge,
> >> +
> >> + if (ipv6_addr_is_set(ip6_src)) {
> >> + p->src_addr = *ip6_src;
> >> +
> >> + err = verify_prefsrc(ip6_dst, output_bridge, &p->src_addr);
> >> + if (err && ipv6_addr_is_set(gw)) {
> >> + err = verify_prefsrc(gw, output_bridge, &p->src_addr);
> >> + }
> >> + } else {
> >> + err = ovs_router_get_netdev_source_address(ip6_dst, output_bridge,
> >> &p->src_addr);
> >> + if (err && ipv6_addr_is_set(gw)) {
> >> + err = ovs_router_get_netdev_source_address(gw, output_bridge,
> >> + &p->src_addr);
> >> + }
> >> }
> >
> > This seems very repetitive. Combining verify_prefsrc() and
> > ovs_router_get_netdev_source_address() might simplify things.
> >
> > Another idea I had was to use a function pointer.
> >
> > *compile tested only*
> >
> >
> > int (*f)(const struct in6_addr *ip6_dst,
> > const char output_bridge[],
> > struct in6_addr *prefsrc);
> >
> > ...
> >
> >
> > if (ipv6_addr_is_set(ip6_src)) {
> > p->src_addr = *ip6_src;
> >
> > f = verify_prefsrc;
> > } else {
> > f = ovs_router_get_netdev_source_address;
> > }
> >
> > err = f(ip6_dst, output_bridge, &p->src_addr);
> > if (err && ipv6_addr_is_set(gw)) {
> > err = f(gw, output_bridge, &p->src_addr);
> > }
>
> Oh thanks. This is clean code using a function pointer,
> I'll try this idea if it's difficult to combine the two functions.
Thanks.
...
> >> } 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");
> >> 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 (ovs_scan(argv[i], "src="IPV6_SCAN_FMT, src6_s) &&
> >> + ipv6_parse(src6_s, &src6)) {
> >> + continue;
> >> + }
> >> + if (ipv6_parse(argv[i], &gw6)) {
> >> + continue;
> >> + }
> >> + } else {
> >> + if (ovs_scan(argv[i], "src="IP_SCAN_FMT, IP_SCAN_ARGS(&src)))
> >> {
> >> + continue;
> >> + }
> >> + if (ip_parse(argv[i], &gw)) {
> >> + continue;
> >> + }
> >> + }
> >
> > If I read this correctly then, before this patch gw (if present)
> > must come before pkt_mark (if present). A maximum of two arguments
> > are parsed for gw and pkt_mark. And pkt_mark, but not gw,
> > may occur twice (in which case gw is not parsed).
> >
> > But after this patch pkt_mark, gw and src may come in any order.
> > And each may occur any number of times.
>
> Yes, this behavior matches purpose of this patch.
>
> > Perhaps my analysis is wrong. But my question is, are there
> > any backwards compatibility issues here?
> >
> > Also, perhaps it would be best to refactor the parser, as a cleanup
> > patch, then add support for 'src'.
>
> After this patch, I think that optional parameters can be given more
> flexibly without breaking backwards compatibility.
>
> pkt_mark, gw and src 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, gw and src have separate prefix strings so they can be
> parsed in any order.
On reflection, yes I agree.
I don't think there are any backwards compatibility issues.
And this approach does seem nicer.
> I'll a cleanup patch before this patch.
Great, thanks.
...
> >> diff --git a/ofproto/ofproto-tnl-unixctl.man
> >> b/ofproto/ofproto-tnl-unixctl.man
> >> index 13a465119a90..3d8f8261f3c4 100644
> >> --- a/ofproto/ofproto-tnl-unixctl.man
> >> +++ b/ofproto/ofproto-tnl-unixctl.man
> >> @@ -1,8 +1,9 @@
> >> .SS "OPENVSWITCH TUNNELING COMMANDS"
> >> These commands query and modify OVS tunnel components.
> >> .
> >> -.IP "\fBovs/route/add ipv4_address/plen output_bridge [GW]\fR"
> >> -Adds ipv4_address/plen route to vswitchd routing table. output_bridge
> >> +.IP "\fBovs/route/add \fIip\fB/\fIplen\fB \fIoutput_bridge\fB \
> >> +[\fIgateway\fB] [pkt_mark=\fImark\fB] [src=\fIsrc_ip\fB]\fR"
> >> +Adds \fIip\fR/\fIplen\fR route to vswitchd routing table.
> >> \fIoutput_bridge\fR
> >> needs to be OVS bridge name. This command is useful if OVS cached
> >> routes does not look right.
> >> .
> >> @@ -10,8 +11,8 @@ routes does not look right.
> >> Print all routes in OVS routing table, This includes routes cached
> >> from system routing table and user configured routes.
> >> .
> >> -.IP "\fBovs/route/del ipv4_address/plen\fR"
> >> -Delete ipv4_address/plen route from OVS routing table.
> >> +.IP "\fBovs/route/del \fIip\fB/\fIplen\fR"
> >> +Delete \fIip\fR/\fIplen\fR route from OVS routing table.
> >> .
> >> .IP "\fBtnl/neigh/show\fR"
> >> .IP "\fBtnl/arp/show\fR"
> >
> > Is the reason for the s/ipv4_address/ip/ portion of the man page changes
> > because both IPv4 and IPv6 are supported?
> >
> > If so is that also the case before this patch?
> >
> > If so, perhaps that change should be in a separate patch.
>
> Both ipv4 and ipv6 were supported before this patch. So this fix is necessary,
> but out of scope, so it is removed from this patch series.
Thanks. IMHO it's find to make this change in this patch-set.
But I think a separate patch is warranted.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
