[PATCH iproute2] geneve: fix IPv6 remote address reporting

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 <redacted>
---
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

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

On 06/05/16 15:43, Phil Sutter wrote:

On Fri, May 06, 2016 at 03:28:25PM +0100, Edward Cree wrote:

quoted

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

On Fri, May 06, 2016 at 04:14:11PM +0100, Edward Cree wrote:

On 06/05/16 15:43, Phil Sutter wrote:

quoted

On Fri, May 06, 2016 at 03:28:25PM +0100, Edward Cree wrote:

quoted

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?

quoted

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

On Fri, 6 May 2016 15:28:25 +0100
Edward Cree [off-list ref] 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 <redacted>
---
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