| Message ID | 1442518065-23294-1-git-send-email-linville@tuxdriver.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | stephen hemminger |
| Headers | show |
On Thu, 2015-09-17 at 15:27 -0400, John W. Linville wrote: > Signed-off-by: John W. Linville <linville@tuxdriver.com> > --- > } > > @@ -150,6 +159,10 @@ static void geneve_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[]) > else > fprintf(f, "tos %#x ", tos); > } > + > + if (tb[IFLA_GENEVE_PORT]) > + fprintf(f, "dstport %u ", > + ntohs(rta_getattr_u16(tb[IFLA_GENEVE_PORT]))); This looks strange. Kernel does : if (nla_put_u16(skb, IFLA_GENEVE_PORT, ntohs(geneve->dst_port))) goto nla_put_failure; -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Sep 17, 2015 at 01:49:49PM -0700, Eric Dumazet wrote: > On Thu, 2015-09-17 at 15:27 -0400, John W. Linville wrote: > > Signed-off-by: John W. Linville <linville@tuxdriver.com> > > --- > > > } > > > > @@ -150,6 +159,10 @@ static void geneve_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[]) > > else > > fprintf(f, "tos %#x ", tos); > > } > > + > > + if (tb[IFLA_GENEVE_PORT]) > > + fprintf(f, "dstport %u ", > > + ntohs(rta_getattr_u16(tb[IFLA_GENEVE_PORT]))); > > This looks strange. > > Kernel does : > > if (nla_put_u16(skb, IFLA_GENEVE_PORT, ntohs(geneve->dst_port))) > goto nla_put_failure; Indeed, you are right. I had essentially copied some vxlan code when I did my version of adding the port attribute, and didn't take much care when adapting that code for the version that actually got merged. The current geneve code is using host byte-order for the UDP port in the netlink messages. But, I see that vxlan, gre, iptnl, etc are using network byte order for specifying UDP ports in their netlink stuff. Should geneve follow that practice as well? Or does it matter? John
On Fri, 2015-09-18 at 10:26 -0400, John W. Linville wrote: > On Thu, Sep 17, 2015 at 01:49:49PM -0700, Eric Dumazet wrote: > > On Thu, 2015-09-17 at 15:27 -0400, John W. Linville wrote: > > > Signed-off-by: John W. Linville <linville@tuxdriver.com> > > > --- > > > > > } > > > > > > @@ -150,6 +159,10 @@ static void geneve_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[]) > > > else > > > fprintf(f, "tos %#x ", tos); > > > } > > > + > > > + if (tb[IFLA_GENEVE_PORT]) > > > + fprintf(f, "dstport %u ", > > > + ntohs(rta_getattr_u16(tb[IFLA_GENEVE_PORT]))); > > > > This looks strange. > > > > Kernel does : > > > > if (nla_put_u16(skb, IFLA_GENEVE_PORT, ntohs(geneve->dst_port))) > > goto nla_put_failure; > > Indeed, you are right. I had essentially copied some vxlan code when > I did my version of adding the port attribute, and didn't take much > care when adapting that code for the version that actually got merged. > > The current geneve code is using host byte-order for the UDP port in > the netlink messages. But, I see that vxlan, gre, iptnl, etc are using > network byte order for specifying UDP ports in their netlink stuff. > Should geneve follow that practice as well? Or does it matter? It might be too late to change the ABI, as geneve is in linux-4.2 Keep current host order in netlink messages and respin your iproute2 patch, and maybe add a comment to explain difference for a casual reader ? Thanks ! -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Sep 18, 2015 at 7:45 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote: > On Fri, 2015-09-18 at 10:26 -0400, John W. Linville wrote: >> On Thu, Sep 17, 2015 at 01:49:49PM -0700, Eric Dumazet wrote: >> > On Thu, 2015-09-17 at 15:27 -0400, John W. Linville wrote: >> > > Signed-off-by: John W. Linville <linville@tuxdriver.com> >> > > --- >> > >> > > } >> > > >> > > @@ -150,6 +159,10 @@ static void geneve_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[]) >> > > else >> > > fprintf(f, "tos %#x ", tos); >> > > } >> > > + >> > > + if (tb[IFLA_GENEVE_PORT]) >> > > + fprintf(f, "dstport %u ", >> > > + ntohs(rta_getattr_u16(tb[IFLA_GENEVE_PORT]))); >> > >> > This looks strange. >> > >> > Kernel does : >> > >> > if (nla_put_u16(skb, IFLA_GENEVE_PORT, ntohs(geneve->dst_port))) >> > goto nla_put_failure; >> >> Indeed, you are right. I had essentially copied some vxlan code when >> I did my version of adding the port attribute, and didn't take much >> care when adapting that code for the version that actually got merged. >> >> The current geneve code is using host byte-order for the UDP port in >> the netlink messages. But, I see that vxlan, gre, iptnl, etc are using >> network byte order for specifying UDP ports in their netlink stuff. >> Should geneve follow that practice as well? Or does it matter? > > It might be too late to change the ABI, as geneve is in linux-4.2 We might be in luck actually. Geneve itself came earlier but I think the ability to specify the dest port is in 4.3 only, so we still have a chance to make it consistent. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Sep 18, 2015 at 09:15:56AM -0700, Jesse Gross wrote: > On Fri, Sep 18, 2015 at 7:45 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote: > > On Fri, 2015-09-18 at 10:26 -0400, John W. Linville wrote: > >> On Thu, Sep 17, 2015 at 01:49:49PM -0700, Eric Dumazet wrote: > >> > On Thu, 2015-09-17 at 15:27 -0400, John W. Linville wrote: > >> > > Signed-off-by: John W. Linville <linville@tuxdriver.com> > >> > > --- > >> > > >> > > } > >> > > > >> > > @@ -150,6 +159,10 @@ static void geneve_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[]) > >> > > else > >> > > fprintf(f, "tos %#x ", tos); > >> > > } > >> > > + > >> > > + if (tb[IFLA_GENEVE_PORT]) > >> > > + fprintf(f, "dstport %u ", > >> > > + ntohs(rta_getattr_u16(tb[IFLA_GENEVE_PORT]))); > >> > > >> > This looks strange. > >> > > >> > Kernel does : > >> > > >> > if (nla_put_u16(skb, IFLA_GENEVE_PORT, ntohs(geneve->dst_port))) > >> > goto nla_put_failure; > >> > >> Indeed, you are right. I had essentially copied some vxlan code when > >> I did my version of adding the port attribute, and didn't take much > >> care when adapting that code for the version that actually got merged. > >> > >> The current geneve code is using host byte-order for the UDP port in > >> the netlink messages. But, I see that vxlan, gre, iptnl, etc are using > >> network byte order for specifying UDP ports in their netlink stuff. > >> Should geneve follow that practice as well? Or does it matter? > > > > It might be too late to change the ABI, as geneve is in linux-4.2 > > We might be in luck actually. Geneve itself came earlier but I think > the ability to specify the dest port is in 4.3 only, so we still have > a chance to make it consistent. Right, that's what I was thinking. So we are agreed to use network byte order? I'll be happy to post a patch. John
diff --git a/ip/iplink_geneve.c b/ip/iplink_geneve.c index 331240a6a3d9..0a45647844f5 100644 --- a/ip/iplink_geneve.c +++ b/ip/iplink_geneve.c @@ -19,6 +19,7 @@ static void print_explain(FILE *f) { fprintf(f, "Usage: ... geneve id VNI remote ADDR\n"); fprintf(f, " [ ttl TTL ] [ tos TOS ]\n"); + fprintf(f, " [ dstport PORT ]\n"); fprintf(f, "\n"); fprintf(f, "Where: VNI := 0-16777215\n"); fprintf(f, " ADDR := IP_ADDRESS\n"); @@ -40,6 +41,7 @@ static int geneve_parse_opt(struct link_util *lu, int argc, char **argv, struct in6_addr daddr6 = IN6ADDR_ANY_INIT; __u8 ttl = 0; __u8 tos = 0; + __u16 dstport = 0; while (argc > 0) { if (!matches(*argv, "id") || @@ -80,6 +82,10 @@ static int geneve_parse_opt(struct link_util *lu, int argc, char **argv, tos = uval; } else tos = 1; + } else if (!matches(*argv, "dstport")){ + NEXT_ARG(); + if (get_u16(&dstport, *argv, 0)) + invarg("dst port", *argv); } else if (matches(*argv, "help") == 0) { explain(); return -1; @@ -111,6 +117,9 @@ static int geneve_parse_opt(struct link_util *lu, int argc, char **argv, addattr8(n, 1024, IFLA_GENEVE_TTL, ttl); addattr8(n, 1024, IFLA_GENEVE_TOS, tos); + if (dstport) + addattr16(n, 1024, IFLA_GENEVE_PORT, htons(dstport)); + return 0; } @@ -150,6 +159,10 @@ static void geneve_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[]) else fprintf(f, "tos %#x ", tos); } + + if (tb[IFLA_GENEVE_PORT]) + fprintf(f, "dstport %u ", + ntohs(rta_getattr_u16(tb[IFLA_GENEVE_PORT]))); } static void geneve_print_help(struct link_util *lu, int argc, char **argv, diff --git a/man/man8/ip-link.8.in b/man/man8/ip-link.8.in index 1896eb6f185e..2e1889af650e 100644 --- a/man/man8/ip-link.8.in +++ b/man/man8/ip-link.8.in @@ -747,6 +747,8 @@ the following additional arguments are supported: .BI ttl " TTL " .R " ] [ " .BI tos " TOS " +.R " ] [ " +.BI dstport " PORT " .R " ]" .in +8 @@ -766,6 +768,10 @@ the following additional arguments are supported: .BI tos " TOS" - specifies the TOS value to use in outgoing packets. +.sp +.BI dstport " PORT " +- specifies the UDP destination port to communicate at both ends of the GENEVE tunnel. + .in -8 .SS ip link delete - delete virtual link
Signed-off-by: John W. Linville <linville@tuxdriver.com> --- I didn't see an iproute2 patch posted for this option, so here is my version... ip/iplink_geneve.c | 13 +++++++++++++ man/man8/ip-link.8.in | 6 ++++++ 2 files changed, 19 insertions(+)