Skip to contentSkip to conversation

geneve: fix IPv6 remote address reporting

PATCH In review Edward Cree 3 participants 5 messages May 6, 2016 – May 13, 2016

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 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")

---
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(-)
ip/iplink_geneve.c +1 -1
index 84d948f..65af6b3 100644
@@ -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));
 		}
Signature · 1 line
2.4.3
#1 lore raw
Quoted 2 lines · Edward Cree

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.

Quoted 3 lines

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

#2 lore raw
Quoted 4 lines · Phil Sutter
Quoted 2 lines · Edward Cree

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.

Quoted 2 lines

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

#3 lore raw
Quoted 7 lines · Edward Cree
Quoted 4 lines · Phil Sutter
Quoted 2 lines · Edward Cree

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 4 lines
Quoted 2 lines

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

#4 lore raw
In reply to Edward Cree
Quoted 5 lines · On Fri, 6 May 2016 15:28:25 +0100 Edward Cree <[email protected]> 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")
Quoted 7 lines

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