Re: [ovs-dev] [PATCH v3 2/3] ovs-router: Introduce src option in ovs/route/add command.
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
> >
> > 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 = tru
Re: [ovs-dev] [PATCH v3 2/3] ovs-router: Introduce src option in ovs/route/add command.
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
>
> 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,
>>
Re: [ovs-dev] [PATCH v3 2/3] ovs-router: Introduce src option in ovs/route/add command.
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
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
> 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_a
[ovs-dev] [PATCH v3 2/3] ovs-router: Introduce src option in ovs/route/add command.
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
---
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
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_be3
