| Message ID | 20230214033906.23576-3-nmiki@yahoo-corp.jp |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add support for preffered src address in ovs-router | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
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 <nmiki@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. > --- > NEWS | 3 + > lib/ovs-router.c | 134 ++++++++++++++++++++++++-------- > ofproto/ofproto-tnl-unixctl.man | 9 ++- > tests/ovs-router.at | 78 +++++++++++++++++++ > 4 files changed, 188 insertions(+), 36 deletions(-) > > diff --git a/NEWS b/NEWS > index fe6055a2700b..9d98e1573e3b 100644 > --- a/NEWS > +++ b/NEWS > @@ -4,6 +4,9 @@ Post-v3.1.0 > * OVS now collects per-interface upcall statistics that can be obtained > via 'ovs-appctl dpctl/show -s' or the interface's statistics column > in OVSDB. Available with upstream kernel 6.2+. > + - ovs-appctl: > + * Add support for selecting the source address with the > + “ovs-appctl ovs/route/add" command. > > > v3.1.0 - xx xxx xxxx > 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. 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? > +verify_prefsrc(const struct in6_addr *ip6_dst, > + const char output_bridge[], > + struct in6_addr *prefsrc) > +{ > + struct in6_addr *mask, *addr6; > + int err, n_in6, i; > + struct netdev *dev; > + > + err = netdev_open(output_bridge, NULL, &dev); > + if (err) { > + return err; > + } > + > + err = netdev_get_addr_list(dev, &addr6, &mask, &n_in6); > + if (err) { > + goto out; > + } > + > + err = ENOENT; nit: If you move this to below the for loop... > + for (i = 0; i < n_in6; i++) { > + struct in6_addr a1, a2; > + a1 = ipv6_addr_bitand(ip6_dst, &mask[i]); > + a2 = ipv6_addr_bitand(prefsrc, &mask[i]); > + > + /* Check that the intarface has "prefsrc" and > + * it is same broadcast domain with "ip6_dst". */ > + if (IN6_ARE_ADDR_EQUAL(prefsrc, &addr6[i]) && > + IN6_ARE_ADDR_EQUAL(&a1, &a2)) { > + err = 0; ... then I don't think you need to set err here, as it will already be set to zero from the call to netdev_get_addr_list. > + goto out; > + } > + } > + > +out: > + free(addr6); > + free(mask); > + netdev_close(dev); > + return err; > +} > + > int > ovs_router_get_netdev_source_address(const struct in6_addr *ip6_dst, > const char output_bridge[], > @@ -217,7 +258,8 @@ static int > ovs_router_insert__(uint32_t mark, uint8_t priority, bool local, > const struct in6_addr *ip6_dst, > uint8_t plen, const char output_bridge[], > - const struct in6_addr *gw) > + const struct in6_addr *gw, > + const struct in6_addr *ip6_src) > { > const struct cls_rule *cr; > struct ovs_router_entry *p; > @@ -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); } > if (err) { > struct ds ds = DS_EMPTY_INITIALIZER; > @@ -274,7 +326,8 @@ ovs_router_insert(uint32_t mark, const struct in6_addr *ip_dst, uint8_t plen, > { > if (use_system_routing_table) { > uint8_t priority = local ? plen + 64 : plen; > - ovs_router_insert__(mark, priority, local, ip_dst, plen, output_bridge, gw); > + ovs_router_insert__(mark, priority, local, ip_dst, plen, > + output_bridge, gw, &in6addr_any); > } > } > > @@ -342,47 +395,64 @@ ovs_router_add(struct unixctl_conn *conn, int argc, > const char *argv[], void *aux OVS_UNUSED) > { > struct in6_addr gw6 = in6addr_any; > + struct in6_addr src6 = in6addr_any; > struct in6_addr ip6; > uint32_t mark = 0; > unsigned int plen; > + ovs_be32 gw = 0; > + ovs_be32 src = 0; > ovs_be32 ip; > int err; > + bool is_ipv6 = false; > + char src6_s[IPV6_SCAN_LEN + 1]; > + int i; In general I think it would be best if code moved towards reverse-xmas tree sorting of local variables - longest line to shortest. This seems to go in the opposite direction. > > 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; I think it would be cleaner to set is_ipv6 = false here and not set it when it is declared. > } 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. 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'. > + > + unixctl_command_reply_error(conn, "Invalid parameters"); > + return; > } > > - err = ovs_router_insert__(mark, plen + 32, false, &ip6, plen, argv[2], &gw6); > + if (gw) { > + in6_addr_set_mapped_ipv4(&gw6, gw); > + } > + if (src) { > + in6_addr_set_mapped_ipv4(&src6, src); > + } > + > + err = ovs_router_insert__(mark, plen + 32, false, &ip6, plen, argv[2], > + &gw6, &src6); > if (err) { > unixctl_command_reply_error(conn, "Error while inserting route."); > } else { > @@ -533,8 +603,8 @@ ovs_router_init(void) > classifier_init(&cls, NULL); > unixctl_command_register("ovs/route/add", > "ip_addr/prefix_len out_br_name [gw] " > - "[pkt_mark=mark]", > - 2, 4, ovs_router_add, NULL); > + "[pkt_mark=mark] [src=src_ip_addr]", > + 2, 5, ovs_router_add, NULL); > unixctl_command_register("ovs/route/show", "", 0, 0, > ovs_router_show, NULL); > unixctl_command_register("ovs/route/del", "ip_addr/prefix_len " > 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.
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 <nmiki@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. >> 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. >> +verify_prefsrc(const struct in6_addr *ip6_dst, >> + const char output_bridge[], >> + struct in6_addr *prefsrc) >> +{ >> + struct in6_addr *mask, *addr6; >> + int err, n_in6, i; >> + struct netdev *dev; >> + >> + err = netdev_open(output_bridge, NULL, &dev); >> + if (err) { >> + return err; >> + } >> + >> + err = netdev_get_addr_list(dev, &addr6, &mask, &n_in6); >> + if (err) { >> + goto out; >> + } >> + >> + err = ENOENT; > > nit: If you move this to below the for loop... > >> + for (i = 0; i < n_in6; i++) { >> + struct in6_addr a1, a2; >> + a1 = ipv6_addr_bitand(ip6_dst, &mask[i]); >> + a2 = ipv6_addr_bitand(prefsrc, &mask[i]); >> + >> + /* Check that the intarface has "prefsrc" and >> + * it is same broadcast domain with "ip6_dst". */ >> + if (IN6_ARE_ADDR_EQUAL(prefsrc, &addr6[i]) && >> + IN6_ARE_ADDR_EQUAL(&a1, &a2)) { >> + err = 0; > > ... then I don't think you need to set err here, as it will already > be set to zero from the call to netdev_get_addr_list. It's clean and nice. Thanks. >> + goto out; >> + } >> + } >> + >> +out: >> + free(addr6); >> + free(mask); >> + netdev_close(dev); >> + return err; >> +} >> + >> int >> ovs_router_get_netdev_source_address(const struct in6_addr *ip6_dst, >> const char output_bridge[], >> @@ -217,7 +258,8 @@ static int >> ovs_router_insert__(uint32_t mark, uint8_t priority, bool local, >> const struct in6_addr *ip6_dst, >> uint8_t plen, const char output_bridge[], >> - const struct in6_addr *gw) >> + const struct in6_addr *gw, >> + const struct in6_addr *ip6_src) >> { >> const struct cls_rule *cr; >> struct ovs_router_entry *p; >> @@ -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. >> if (err) { >> struct ds ds = DS_EMPTY_INITIALIZER; >> @@ -274,7 +326,8 @@ ovs_router_insert(uint32_t mark, const struct in6_addr *ip_dst, uint8_t plen, >> { >> if (use_system_routing_table) { >> uint8_t priority = local ? plen + 64 : plen; >> - ovs_router_insert__(mark, priority, local, ip_dst, plen, output_bridge, gw); >> + ovs_router_insert__(mark, priority, local, ip_dst, plen, >> + output_bridge, gw, &in6addr_any); >> } >> } >> >> @@ -342,47 +395,64 @@ ovs_router_add(struct unixctl_conn *conn, int argc, >> const char *argv[], void *aux OVS_UNUSED) >> { >> struct in6_addr gw6 = in6addr_any; >> + struct in6_addr src6 = in6addr_any; >> struct in6_addr ip6; >> uint32_t mark = 0; >> unsigned int plen; >> + ovs_be32 gw = 0; >> + ovs_be32 src = 0; >> ovs_be32 ip; >> int err; >> + bool is_ipv6 = false; >> + char src6_s[IPV6_SCAN_LEN + 1]; >> + int i; > > In general I think it would be best if code moved towards > reverse-xmas tree sorting of local variables - longest line to shortest. > This seems to go in the opposite direction. OK. I'll fix it. >> >> 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; > > I think it would be cleaner to set is_ipv6 = false here > and not set it when it is declared. OK. I'll fix. >> } 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. I'll a cleanup patch before this patch. >> + >> + unixctl_command_reply_error(conn, "Invalid parameters"); >> + return; >> } >> >> - err = ovs_router_insert__(mark, plen + 32, false, &ip6, plen, argv[2], &gw6); >> + if (gw) { >> + in6_addr_set_mapped_ipv4(&gw6, gw); >> + } >> + if (src) { >> + in6_addr_set_mapped_ipv4(&src6, src); >> + } >> + >> + err = ovs_router_insert__(mark, plen + 32, false, &ip6, plen, argv[2], >> + &gw6, &src6); >> if (err) { >> unixctl_command_reply_error(conn, "Error while inserting route."); >> } else { >> @@ -533,8 +603,8 @@ ovs_router_init(void) >> classifier_init(&cls, NULL); >> unixctl_command_register("ovs/route/add", >> "ip_addr/prefix_len out_br_name [gw] " >> - "[pkt_mark=mark]", >> - 2, 4, ovs_router_add, NULL); >> + "[pkt_mark=mark] [src=src_ip_addr]", >> + 2, 5, ovs_router_add, NULL); >> unixctl_command_register("ovs/route/show", "", 0, 0, >> ovs_router_show, NULL); >> unixctl_command_register("ovs/route/del", "ip_addr/prefix_len " >> 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. Best Ragards, Nobuhiro MIKI
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 <nmiki@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.
diff --git a/NEWS b/NEWS index fe6055a2700b..9d98e1573e3b 100644 --- a/NEWS +++ b/NEWS @@ -4,6 +4,9 @@ Post-v3.1.0 * OVS now collects per-interface upcall statistics that can be obtained via 'ovs-appctl dpctl/show -s' or the interface's statistics column in OVSDB. Available with upstream kernel 6.2+. + - ovs-appctl: + * Add support for selecting the source address with the + “ovs-appctl ovs/route/add" command. v3.1.0 - xx xxx xxxx 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 +verify_prefsrc(const struct in6_addr *ip6_dst, + const char output_bridge[], + struct in6_addr *prefsrc) +{ + struct in6_addr *mask, *addr6; + int err, n_in6, i; + struct netdev *dev; + + err = netdev_open(output_bridge, NULL, &dev); + if (err) { + return err; + } + + err = netdev_get_addr_list(dev, &addr6, &mask, &n_in6); + if (err) { + goto out; + } + + err = ENOENT; + for (i = 0; i < n_in6; i++) { + struct in6_addr a1, a2; + a1 = ipv6_addr_bitand(ip6_dst, &mask[i]); + a2 = ipv6_addr_bitand(prefsrc, &mask[i]); + + /* Check that the intarface has "prefsrc" and + * it is same broadcast domain with "ip6_dst". */ + if (IN6_ARE_ADDR_EQUAL(prefsrc, &addr6[i]) && + IN6_ARE_ADDR_EQUAL(&a1, &a2)) { + err = 0; + goto out; + } + } + +out: + free(addr6); + free(mask); + netdev_close(dev); + return err; +} + int ovs_router_get_netdev_source_address(const struct in6_addr *ip6_dst, const char output_bridge[], @@ -217,7 +258,8 @@ static int ovs_router_insert__(uint32_t mark, uint8_t priority, bool local, const struct in6_addr *ip6_dst, uint8_t plen, const char output_bridge[], - const struct in6_addr *gw) + const struct in6_addr *gw, + const struct in6_addr *ip6_src) { const struct cls_rule *cr; struct ovs_router_entry *p; @@ -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); + } } if (err) { struct ds ds = DS_EMPTY_INITIALIZER; @@ -274,7 +326,8 @@ ovs_router_insert(uint32_t mark, const struct in6_addr *ip_dst, uint8_t plen, { if (use_system_routing_table) { uint8_t priority = local ? plen + 64 : plen; - ovs_router_insert__(mark, priority, local, ip_dst, plen, output_bridge, gw); + ovs_router_insert__(mark, priority, local, ip_dst, plen, + output_bridge, gw, &in6addr_any); } } @@ -342,47 +395,64 @@ ovs_router_add(struct unixctl_conn *conn, int argc, const char *argv[], void *aux OVS_UNUSED) { struct in6_addr gw6 = in6addr_any; + struct in6_addr src6 = in6addr_any; struct in6_addr ip6; uint32_t mark = 0; unsigned int plen; + ovs_be32 gw = 0; + ovs_be32 src = 0; ovs_be32 ip; int err; + bool is_ipv6 = false; + char src6_s[IPV6_SCAN_LEN + 1]; + 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; } 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; + } + } + + unixctl_command_reply_error(conn, "Invalid parameters"); + return; } - err = ovs_router_insert__(mark, plen + 32, false, &ip6, plen, argv[2], &gw6); + if (gw) { + in6_addr_set_mapped_ipv4(&gw6, gw); + } + if (src) { + in6_addr_set_mapped_ipv4(&src6, src); + } + + err = ovs_router_insert__(mark, plen + 32, false, &ip6, plen, argv[2], + &gw6, &src6); if (err) { unixctl_command_reply_error(conn, "Error while inserting route."); } else { @@ -533,8 +603,8 @@ ovs_router_init(void) classifier_init(&cls, NULL); unixctl_command_register("ovs/route/add", "ip_addr/prefix_len out_br_name [gw] " - "[pkt_mark=mark]", - 2, 4, ovs_router_add, NULL); + "[pkt_mark=mark] [src=src_ip_addr]", + 2, 5, ovs_router_add, NULL); unixctl_command_register("ovs/route/show", "", 0, 0, ovs_router_show, NULL); unixctl_command_register("ovs/route/del", "ip_addr/prefix_len " 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" diff --git a/tests/ovs-router.at b/tests/ovs-router.at index 6dacc2954bc6..42f01b6281e9 100644 --- a/tests/ovs-router.at +++ b/tests/ovs-router.at @@ -12,6 +12,84 @@ AT_CHECK([ovs-appctl ovs/route/add 1.1.1.0/24 br0 2.2.2.10], [0], [OK OVS_VSWITCHD_STOP AT_CLEANUP +AT_SETUP([appctl - route/add with src - ipv4]) +AT_KEYWORDS([ovs_router]) +OVS_VSWITCHD_START([add-port br0 p1 -- set Interface p1 type=dummy]) +AT_CHECK([ovs-appctl netdev-dummy/ip4addr br0 192.168.9.2/24], [0], [OK +]) +AT_CHECK([ovs-appctl netdev-dummy/ip4addr br0 192.168.9.3/24], [0], [OK +]) +AT_CHECK([ovs-appctl ovs/route/add 192.168.9.11/32 br0 src=192.168.9.3], [0], [OK +]) +AT_CHECK([ovs-appctl ovs/route/add 192.168.10.12/32 br0 192.168.9.1 src=192.168.9.3], [0], [OK +]) +AT_CHECK([ovs-appctl ovs/route/add 192.168.10.13/32 br0 192.168.9.1 pkt_mark=13 src=192.168.9.3], [0], [OK +]) +AT_CHECK([ovs-appctl ovs/route/add 192.168.10.14/32 br0 192.168.9.1 pkt_mark=14 src=192.168.9.2], [0], [OK +]) +AT_CHECK([ovs-appctl ovs/route/add 192.168.10.15/32 br0 192.168.9.1 src=foo.bar.9.200], [2], [], [dnl +Invalid parameters +ovs-appctl: ovs-vswitchd: server returned an error +]) +AT_CHECK([ovs-appctl ovs/route/add 192.168.10.16/32 br0 192.168.9.1 src=192.168.9.200], [2], [], [dnl +Error while inserting route. +ovs-appctl: ovs-vswitchd: server returned an error +]) +AT_CHECK([ovs-appctl ovs/route/add 192.168.10.17/32 br0 192.168.11.1 src=192.168.9.3], [2], [], [dnl +Error while inserting route. +ovs-appctl: ovs-vswitchd: server returned an error +]) +AT_CHECK([ovs-appctl ovs/route/add 192.168.10.18/32 br0 src=192.168.9.3], [2], [], [dnl +Error while inserting route. +ovs-appctl: ovs-vswitchd: server returned an error +]) +AT_CHECK([ovs-appctl ovs/route/show | grep User | grep 192.168.10 | sort], [0], [dnl +User: 192.168.10.12/32 dev br0 GW 192.168.9.1 SRC 192.168.9.3 +User: 192.168.10.13/32 MARK 13 dev br0 GW 192.168.9.1 SRC 192.168.9.3 +User: 192.168.10.14/32 MARK 14 dev br0 GW 192.168.9.1 SRC 192.168.9.2 +]) +OVS_VSWITCHD_STOP +AT_CLEANUP + +AT_SETUP([appctl - route/add with src - ipv6]) +AT_KEYWORDS([ovs_router]) +OVS_VSWITCHD_START([add-port br0 p1 -- set Interface p1 type=dummy]) +AT_CHECK([ovs-appctl netdev-dummy/ip6addr br0 2001:db8:cafe::2/64], [0], [OK +]) +AT_CHECK([ovs-appctl netdev-dummy/ip6addr br0 2001:db8:cafe::3/64], [0], [OK +]) +AT_CHECK([ovs-appctl ovs/route/add 2001:db8:cafe::11/128 br0 src=2001:db8:cafe::3], [0], [OK +]) +AT_CHECK([ovs-appctl ovs/route/add 2001:db8:beef::12/128 br0 2001:db8:cafe::1 src=2001:db8:cafe::3], [0], [OK +]) +AT_CHECK([ovs-appctl ovs/route/add 2001:db8:beef::13/128 br0 2001:db8:cafe::1 pkt_mark=13 src=2001:db8:cafe::3], [0], [OK +]) +AT_CHECK([ovs-appctl ovs/route/add 2001:db8:beef::14/128 br0 2001:db8:cafe::1 pkt_mark=14 src=2001:db8:cafe::2], [0], [OK +]) +AT_CHECK([ovs-appctl ovs/route/add 2001:db8:beef::15/128 br0 2001:db8:cafe::1 src=foo:bar:2001:db8:cafe], [2], [], [dnl +Invalid parameters +ovs-appctl: ovs-vswitchd: server returned an error +]) +AT_CHECK([ovs-appctl ovs/route/add 2001:db8:beef::16/128 br0 2001:db8:cafe::1 src=2001:db8:cafe::200], [2], [], [dnl +Error while inserting route. +ovs-appctl: ovs-vswitchd: server returned an error +]) +AT_CHECK([ovs-appctl ovs/route/add 2001:db8:beef::17/128 br0 2001:db8:face::1 src=2001:db8:cafe::3], [2], [], [dnl +Error while inserting route. +ovs-appctl: ovs-vswitchd: server returned an error +]) +AT_CHECK([ovs-appctl ovs/route/add 2001:db8:beef::18/128 br0 src=2001:db8:cafe::3], [2], [], [dnl +Error while inserting route. +ovs-appctl: ovs-vswitchd: server returned an error +]) +AT_CHECK([ovs-appctl ovs/route/show | grep User | grep 2001:db8:beef | sort], [0], [dnl +User: 2001:db8:beef::12/128 dev br0 GW 2001:db8:cafe::1 SRC 2001:db8:cafe::3 +User: 2001:db8:beef::13/128 MARK 13 dev br0 GW 2001:db8:cafe::1 SRC 2001:db8:cafe::3 +User: 2001:db8:beef::14/128 MARK 14 dev br0 GW 2001:db8:cafe::1 SRC 2001:db8:cafe::2 +]) +OVS_VSWITCHD_STOP +AT_CLEANUP + AT_SETUP([appctl - route/lookup]) AT_KEYWORDS([ovs_router]) OVS_VSWITCHD_START([add-port br0 p1 -- set Interface p1 type=dummy])
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 <nmiki@yahoo-corp.jp> --- NEWS | 3 + lib/ovs-router.c | 134 ++++++++++++++++++++++++-------- ofproto/ofproto-tnl-unixctl.man | 9 ++- tests/ovs-router.at | 78 +++++++++++++++++++ 4 files changed, 188 insertions(+), 36 deletions(-)