| Message ID | 1516639614-12960-2-git-send-email-serhe.popovych@gmail.com |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Ahern |
| Headers | show |
| Series | [iproute2-next,1/2] ip/tunnel: Be consistent when printing tunnel collect metadata | expand |
On Mon, 22 Jan 2018 18:46:53 +0200, Serhey Popovych wrote: > + if (tb[IFLA_GRE_COLLECT_METADATA]) { > + print_bool(PRINT_ANY, "collect_metadata", "external", true); > + return; > + } Nacked-by: Jiri Benc <jbenc@redhat.com> Don't ever use "collect_metadata" for anything visible to the user. collect_metadata is a *horrible* name. It describes the internal implementation of the lwtunneling in the kernel and provides zero explanation to the user about what's that feature good for. The netlink attribute should have never had such name but it's uAPI and we have to live with it. But there's no reason to expose this to the user. Stick with the "external" name. It explains what it is about: instead of the traffic being controlled by the tunnel internal logic (or tunnel control plane, if you want), an external logic needs to be attached to the tunnel in order for the tunneling to work. Jiri
Jiri Benc wrote: > On Mon, 22 Jan 2018 18:46:53 +0200, Serhey Popovych wrote: >> + if (tb[IFLA_GRE_COLLECT_METADATA]) { >> + print_bool(PRINT_ANY, "collect_metadata", "external", true); >> + return; >> + } > > Nacked-by: Jiri Benc <jbenc@redhat.com> > > Don't ever use "collect_metadata" for anything visible to the user. > > collect_metadata is a *horrible* name. It describes the internal > implementation of the lwtunneling in the kernel and provides zero > explanation to the user about what's that feature good for. > > The netlink attribute should have never had such name but it's uAPI and > we have to live with it. But there's no reason to expose this to the > user. > > Stick with the "external" name. It explains what it is about: instead of > the traffic being controlled by the tunnel internal logic (or tunnel > control plane, if you want), an external logic needs to be attached to > the tunnel in order for the tunneling to work. Thanks! That's probably what I expect to hear. Will prepare v2 shortly. > > Jiri >
diff --git a/ip/link_gre.c b/ip/link_gre.c index 674603b..131a087 100644 --- a/ip/link_gre.c +++ b/ip/link_gre.c @@ -395,7 +395,7 @@ get_failed: return 0; } -static void gre_print_direct_opt(FILE *f, struct rtattr *tb[]) +static void gre_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[]) { char s2[64]; const char *local = "any"; @@ -405,6 +405,14 @@ static void gre_print_direct_opt(FILE *f, struct rtattr *tb[]) __u8 ttl = 0; __u8 tos = 0; + if (!tb) + return; + + if (tb[IFLA_GRE_COLLECT_METADATA]) { + print_bool(PRINT_ANY, "collect_metadata", "external", true); + return; + } + if (tb[IFLA_GRE_REMOTE]) { unsigned int addr = rta_getattr_u32(tb[IFLA_GRE_REMOTE]); @@ -455,6 +463,9 @@ static void gre_print_direct_opt(FILE *f, struct rtattr *tb[]) print_bool(PRINT_JSON, "pmtudisc", NULL, true); } + if (tb[IFLA_GRE_IGNORE_DF] && rta_getattr_u8(tb[IFLA_GRE_IGNORE_DF])) + print_bool(PRINT_ANY, "ignore_df", "ignore-df ", true); + if (tb[IFLA_GRE_IFLAGS]) iflags = rta_getattr_u16(tb[IFLA_GRE_IFLAGS]); @@ -488,20 +499,6 @@ static void gre_print_direct_opt(FILE *f, struct rtattr *tb[]) "fwmark", "fwmark 0x%x ", fwmark); } } -} - -static void gre_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[]) -{ - if (!tb) - return; - - if (!tb[IFLA_GRE_COLLECT_METADATA]) - gre_print_direct_opt(f, tb); - else - print_bool(PRINT_ANY, "external", "external ", true); - - if (tb[IFLA_GRE_IGNORE_DF] && rta_getattr_u8(tb[IFLA_GRE_IGNORE_DF])) - print_bool(PRINT_ANY, "ignore_df", "ignore-df ", true); if (tb[IFLA_GRE_ERSPAN_INDEX]) { __u32 erspan_idx = rta_getattr_u32(tb[IFLA_GRE_ERSPAN_INDEX]); diff --git a/ip/link_ip6tnl.c b/ip/link_ip6tnl.c index 8f5c9bd..e4db643 100644 --- a/ip/link_ip6tnl.c +++ b/ip/link_ip6tnl.c @@ -341,8 +341,10 @@ static void ip6tunnel_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb if (!tb) return; - if (tb[IFLA_IPTUN_COLLECT_METADATA]) - print_bool(PRINT_ANY, "external", "external ", true); + if (tb[IFLA_IPTUN_COLLECT_METADATA]) { + print_bool(PRINT_ANY, "collect_metadata", "external", true); + return; + } if (tb[IFLA_IPTUN_FLAGS]) flags = rta_getattr_u32(tb[IFLA_IPTUN_FLAGS]); diff --git a/ip/link_iptnl.c b/ip/link_iptnl.c index ce3855c..93a1eb4 100644 --- a/ip/link_iptnl.c +++ b/ip/link_iptnl.c @@ -370,8 +370,10 @@ static void iptunnel_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[ if (!tb) return; - if (tb[IFLA_IPTUN_COLLECT_METADATA]) - print_bool(PRINT_ANY, "external", "external ", true); + if (tb[IFLA_IPTUN_COLLECT_METADATA]) { + print_bool(PRINT_ANY, "collect_metadata", "external", true); + return; + } if (tb[IFLA_IPTUN_PROTO]) { switch (rta_getattr_u8(tb[IFLA_IPTUN_PROTO])) {
Print only "external" if collect meta data attribute is given: rest of parameters are irrelevant. This is to follow gre6. For JSON output use "collect_metadata" for iptnl, ip6tnl and gre to match with vxlan, geneve and gre6 modules. Signed-off-by: Serhey Popovych <serhe.popovych@gmail.com> --- ip/link_gre.c | 27 ++++++++++++--------------- ip/link_ip6tnl.c | 6 ++++-- ip/link_iptnl.c | 6 ++++-- 3 files changed, 20 insertions(+), 19 deletions(-)