[PATCH iproute2] geneve: fix IPv6 remote address reporting

[PATCH iproute2] geneve: fix IPv6 remote address reporting

[Edward Cree]

Since we can only configure unicast, we probably want to be able to
display unicast, rather than multicast.

Fixes: 906ac5437ab8 ("geneve: add support for IPv6 link partners")
Signed-off-by: Edward Cree <ecree@solarflare.com>
---
I'm assuming this is what was intended, but tbh I don't know why we
need to check for multicast on the display side at all, rather than
just displaying whatever the kernel gives us.

 ip/iplink_geneve.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ip/iplink_geneve.c b/ip/iplink_geneve.c
index 84d948f..65af6b3 100644
--- a/ip/iplink_geneve.c
+++ b/ip/iplink_geneve.c
@@ -204,7 +204,7 @@ static void geneve_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[])
 
 		memcpy(&addr, RTA_DATA(tb[IFLA_GENEVE_REMOTE6]), sizeof(struct in6_addr));
 		if (memcmp(&addr, &in6addr_any, sizeof(addr)) != 0) {
-			if (IN6_IS_ADDR_MULTICAST(&addr))
+			if (!IN6_IS_ADDR_MULTICAST(&addr))
 				fprintf(f, "remote %s ",
 					format_host(AF_INET6, sizeof(struct in6_addr), &addr));
 		}
-- 
2.4.3

Re: [PATCH iproute2] geneve: fix IPv6 remote address reporting

[Phil Sutter]

On Fri, May 06, 2016 at 03:28:25PM +0100, Edward Cree wrote:
> Since we can only configure unicast, we probably want to be able to
> display unicast, rather than multicast.

Furthermore, the kernel even rejects multicast peer addresses.

> I'm assuming this is what was intended, but tbh I don't know why we
> need to check for multicast on the display side at all, rather than
> just displaying whatever the kernel gives us.

Why do you then propose a dubious fix to a dubious check instead of
getting rid of it in the first place? Reminds me a bit of this here (no
offense intended):

http://geekandpoke.typepad.com/geekandpoke/2011/07/good-coders.html

Cheers, Phil

Re: [PATCH iproute2] geneve: fix IPv6 remote address reporting

[Edward Cree]

On 06/05/16 15:43, Phil Sutter wrote:
> On Fri, May 06, 2016 at 03:28:25PM +0100, Edward Cree wrote:
>> Since we can only configure unicast, we probably want to be able to
>> display unicast, rather than multicast.
> Furthermore, the kernel even rejects multicast peer addresses.
Yes, but a future kernel might not, and iproute2 is meant to be forward-
compatible.

> Why do you then propose a dubious fix to a dubious check instead of
> getting rid of it in the first place?
Because John Linville clearly had some reason for putting a check there,
and he probably knows better than me.  Chesterton's fence.

-Ed

Re: [PATCH iproute2] geneve: fix IPv6 remote address reporting

[Phil Sutter]

On Fri, May 06, 2016 at 04:14:11PM +0100, Edward Cree wrote:
> On 06/05/16 15:43, Phil Sutter wrote:
> > On Fri, May 06, 2016 at 03:28:25PM +0100, Edward Cree wrote:
> >> Since we can only configure unicast, we probably want to be able to
> >> display unicast, rather than multicast.
> > Furthermore, the kernel even rejects multicast peer addresses.
> Yes, but a future kernel might not, and iproute2 is meant to be forward-
> compatible.

Sorry, but I fail to see how this might break forward compatibility.
Quite the contrary, suppose geneve in future supported multicast peers,
current iproute2 would fail to recognize it's existence. What am I
missing here?

> > Why do you then propose a dubious fix to a dubious check instead of
> > getting rid of it in the first place?
> Because John Linville clearly had some reason for putting a check there,
> and he probably knows better than me.  Chesterton's fence.

A valid point, indeed. In my opinion the same applies to your patch as
well, as instead of removing the fence you're moving it to the other
lane. :)

Cheers, Phil

Re: [PATCH iproute2] geneve: fix IPv6 remote address reporting

[Stephen Hemminger]

On Fri, 6 May 2016 15:28:25 +0100
Edward Cree <ecree@solarflare.com> wrote:

> Since we can only configure unicast, we probably want to be able to
> display unicast, rather than multicast.
> 
> Fixes: 906ac5437ab8 ("geneve: add support for IPv6 link partners")
> Signed-off-by: Edward Cree <ecree@solarflare.com>
> ---
> I'm assuming this is what was intended, but tbh I don't know why we
> need to check for multicast on the display side at all, rather than
> just displaying whatever the kernel gives us.
> 
>  ip/iplink_geneve.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Applied thanks