| Message ID | 20130515134818.18b2c710@nehalam.linuxnetplumber.net |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | stephen hemminger |
| Headers | show |
Stephen, this doesn't work either. You're breaking people's scripts. Even worse, you're breaking things, and your error message doesn't even tell the user how to get the previous behavior. You're telling them how to get new behavior, which they probably don't give a crap about. They want their existing stuff to work. Stop being in denial, we are stuck with the old port number default. Again, we cannot change this default without breaking something which we've already deployed to users. If this port number issue was so important, we should have done something about it when we integrated vxlan. But we didn't, so we have to live with the consequences. Now is far too late to change the default. -- 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 5/15/2013 1:48 PM, Stephen Hemminger wrote: > This change shifts burden onto the users to choose the UDP port value. > There is no default value, the destination port must be specified. > > This is a migration compromise. The initial development of VXLAN > used UDP port 5287 but now there is an official assigned port for The original and current kernel default is 8472. > VXLAN. The kernel can't change because of legacy compatibility > but new deployments should not use the legacy port value. > > --- > ip/iplink_vxlan.c | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) > > diff --git a/ip/iplink_vxlan.c b/ip/iplink_vxlan.c > index 2d93ee2..263feca 100644 > --- a/ip/iplink_vxlan.c > +++ b/ip/iplink_vxlan.c > @@ -53,7 +53,8 @@ static int vxlan_parse_opt(struct link_util *lu, int argc, char **argv, > __u8 noage = 0; > __u32 age = 0; > __u32 maxaddr = 0; > - __u16 dstport = 4789; > + __u16 dstport = 0; > + int dst_port_set = 0; > struct ifla_vxlan_port_range range = { 0, 0 }; > > while (argc > 0) { > @@ -131,6 +132,7 @@ static int vxlan_parse_opt(struct link_util *lu, int argc, char **argv, > NEXT_ARG(); > if (get_u16(&dstport, *argv, 0)) > invarg("dst port", *argv); > + dst_port_set = 1; > } else if (!matches(*argv, "nolearning")) { > learning = 0; > } else if (!matches(*argv, "learning")) { > @@ -161,10 +163,18 @@ static int vxlan_parse_opt(struct link_util *lu, int argc, char **argv, > } > argc--, argv++; > } > + > if (!vni_set) { > fprintf(stderr, "vxlan: missing virtual network identifier\n"); > return -1; > } > + > + if (!dst_port_set) { > + fprintf(stderr, "vxlan: destination port not specified\n" > + "Use 'dstport 4789' to get the IANA assigned value\n"); > + return -1; > + } Just setting the dstport 4789 is not enough. The user has to make sure that the vxlan module is loaded with a module parameter 'udp_port' set to 4789. Thanks Sridhar > + > addattr32(n, 1024, IFLA_VXLAN_ID, vni); > if (gaddr) > addattr_l(n, 1024, IFLA_VXLAN_GROUP, &gaddr, 4); > @@ -179,6 +189,7 @@ static int vxlan_parse_opt(struct link_util *lu, int argc, char **argv, > addattr8(n, 1024, IFLA_VXLAN_RSC, rsc); > addattr8(n, 1024, IFLA_VXLAN_L2MISS, l2miss); > addattr8(n, 1024, IFLA_VXLAN_L3MISS, l3miss); > + > if (noage) > addattr32(n, 1024, IFLA_VXLAN_AGEING, 0); > else if (age) -- 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 Wed, 15 May 2013 14:57:54 -0700 Sridhar Samudrala <samudrala.sridhar@gmail.com> wrote: > On 5/15/2013 1:48 PM, Stephen Hemminger wrote: > > This change shifts burden onto the users to choose the UDP port value. > > There is no default value, the destination port must be specified. > > > > This is a migration compromise. The initial development of VXLAN > > used UDP port 5287 but now there is an official assigned port for > The original and current kernel default is 8472. > > VXLAN. The kernel can't change because of legacy compatibility > > but new deployments should not use the legacy port value. > > > > --- > > ip/iplink_vxlan.c | 13 ++++++++++++- > > 1 file changed, 12 insertions(+), 1 deletion(-) > > > > diff --git a/ip/iplink_vxlan.c b/ip/iplink_vxlan.c > > index 2d93ee2..263feca 100644 > > --- a/ip/iplink_vxlan.c > > +++ b/ip/iplink_vxlan.c > > @@ -53,7 +53,8 @@ static int vxlan_parse_opt(struct link_util *lu, int argc, char **argv, > > __u8 noage = 0; > > __u32 age = 0; > > __u32 maxaddr = 0; > > - __u16 dstport = 4789; > > + __u16 dstport = 0; > > + int dst_port_set = 0; > > struct ifla_vxlan_port_range range = { 0, 0 }; > > > > while (argc > 0) { > > @@ -131,6 +132,7 @@ static int vxlan_parse_opt(struct link_util *lu, int argc, char **argv, > > NEXT_ARG(); > > if (get_u16(&dstport, *argv, 0)) > > invarg("dst port", *argv); > > + dst_port_set = 1; > > } else if (!matches(*argv, "nolearning")) { > > learning = 0; > > } else if (!matches(*argv, "learning")) { > > @@ -161,10 +163,18 @@ static int vxlan_parse_opt(struct link_util *lu, int argc, char **argv, > > } > > argc--, argv++; > > } > > + > > if (!vni_set) { > > fprintf(stderr, "vxlan: missing virtual network identifier\n"); > > return -1; > > } > > + > > + if (!dst_port_set) { > > + fprintf(stderr, "vxlan: destination port not specified\n" > > + "Use 'dstport 4789' to get the IANA assigned value\n"); > > + return -1; > > + } > Just setting the dstport 4789 is not enough. The user has to make sure > that the > vxlan module is loaded with a module parameter 'udp_port' set to 4789. With this (and other fix) the kernel parameter is irrelevant. -- 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 Wed, 15 May 2013 14:47:30 -0700 (PDT) David Miller <davem@davemloft.net> wrote: > > Stephen, this doesn't work either. > > You're breaking people's scripts. > > Even worse, you're breaking things, and your error message doesn't > even tell the user how to get the previous behavior. You're telling > them how to get new behavior, which they probably don't give a > crap about. They want their existing stuff to work. > > Stop being in denial, we are stuck with the old port number default. > > Again, we cannot change this default without breaking something which > we've already deployed to users. > > If this port number issue was so important, we should have done > something about it when we integrated vxlan. But we didn't, so we > have to live with the consequences. > > Now is far too late to change the default. So you want RHEL customers to continue to use the pre-standard Cisco port in their clouds and be incompatible with standards? I made a mistake in the initial implementation using that value and every user for time immemorial has to suffer. Since every distro patches iproute anyway, let them keep the non-standard compatibility if that is what they demand. -- 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 Wed, 15 May 2013 14:47:30 -0700 (PDT) David Miller <davem@davemloft.net> wrote: > > Stephen, this doesn't work either. > > You're breaking people's scripts. > > Even worse, you're breaking things, and your error message doesn't > even tell the user how to get the previous behavior. You're telling > them how to get new behavior, which they probably don't give a > crap about. They want their existing stuff to work. > > Stop being in denial, we are stuck with the old port number default. > > Again, we cannot change this default without breaking something which > we've already deployed to users. > > If this port number issue was so important, we should have done > something about it when we integrated vxlan. But we didn't, so we > have to live with the consequences. > > Now is far too late to change the default. I relented slightly and turned it into a nag, the scripts will still work but they will be noisy. -- 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
From: Stephen Hemminger <stephen@networkplumber.org> Date: Wed, 15 May 2013 15:04:33 -0700 > I made a mistake in the initial implementation using that value and > every user for time immemorial has to suffer. We don't break userland, period. You don't have to explain to me how unfortunate this situation is, I understand. But that doesn't give us a license to break things on people. -- 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
diff --git a/ip/iplink_vxlan.c b/ip/iplink_vxlan.c index 2d93ee2..263feca 100644 --- a/ip/iplink_vxlan.c +++ b/ip/iplink_vxlan.c @@ -53,7 +53,8 @@ static int vxlan_parse_opt(struct link_util *lu, int argc, char **argv, __u8 noage = 0; __u32 age = 0; __u32 maxaddr = 0; - __u16 dstport = 4789; + __u16 dstport = 0; + int dst_port_set = 0; struct ifla_vxlan_port_range range = { 0, 0 }; while (argc > 0) { @@ -131,6 +132,7 @@ static int vxlan_parse_opt(struct link_util *lu, int argc, char **argv, NEXT_ARG(); if (get_u16(&dstport, *argv, 0)) invarg("dst port", *argv); + dst_port_set = 1; } else if (!matches(*argv, "nolearning")) { learning = 0; } else if (!matches(*argv, "learning")) { @@ -161,10 +163,18 @@ static int vxlan_parse_opt(struct link_util *lu, int argc, char **argv, } argc--, argv++; } + if (!vni_set) { fprintf(stderr, "vxlan: missing virtual network identifier\n"); return -1; } + + if (!dst_port_set) { + fprintf(stderr, "vxlan: destination port not specified\n" + "Use 'dstport 4789' to get the IANA assigned value\n"); + return -1; + } + addattr32(n, 1024, IFLA_VXLAN_ID, vni); if (gaddr) addattr_l(n, 1024, IFLA_VXLAN_GROUP, &gaddr, 4); @@ -179,6 +189,7 @@ static int vxlan_parse_opt(struct link_util *lu, int argc, char **argv, addattr8(n, 1024, IFLA_VXLAN_RSC, rsc); addattr8(n, 1024, IFLA_VXLAN_L2MISS, l2miss); addattr8(n, 1024, IFLA_VXLAN_L3MISS, l3miss); + if (noage) addattr32(n, 1024, IFLA_VXLAN_AGEING, 0); else if (age)