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.
>> 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
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
