* [RFC 1/2] vxlan: use print_nll for gbp and gpe @ 2023-05-23 16:59 Stephen Hemminger 2023-05-23 16:59 ` [RFC 2/2] vxlan: make option printing more consistent Stephen Hemminger 0 siblings, 1 reply; 6+ messages in thread From: Stephen Hemminger @ 2023-05-23 16:59 UTC (permalink / raw) To: netdev; +Cc: Stephen Hemminger The Gbp and Gpe are presence, not booleans so use print_null() for them Signed-off-by: Stephen Hemminger <stephen@networkplumber.org> --- ip/iplink_vxlan.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ip/iplink_vxlan.c b/ip/iplink_vxlan.c index c7e0e1c47606..cb6745c74507 100644 --- a/ip/iplink_vxlan.c +++ b/ip/iplink_vxlan.c @@ -650,9 +650,9 @@ static void vxlan_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[]) print_bool(PRINT_ANY, "remcsum_rx", "remcsumrx ", true); if (tb[IFLA_VXLAN_GBP]) - print_bool(PRINT_ANY, "gbp", "gbp ", true); + print_null(PRINT_ANY, "gbp", "gbp ", NULL); if (tb[IFLA_VXLAN_GPE]) - print_bool(PRINT_ANY, "gpe", "gpe ", true); + print_null(PRINT_ANY, "gpe", "gpe ", NULL); } static void vxlan_print_help(struct link_util *lu, int argc, char **argv, -- 2.39.2 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [RFC 2/2] vxlan: make option printing more consistent 2023-05-23 16:59 [RFC 1/2] vxlan: use print_nll for gbp and gpe Stephen Hemminger @ 2023-05-23 16:59 ` Stephen Hemminger 2023-05-24 18:06 ` Andrea Claudi 0 siblings, 1 reply; 6+ messages in thread From: Stephen Hemminger @ 2023-05-23 16:59 UTC (permalink / raw) To: netdev; +Cc: Stephen Hemminger Add new helper function print_bool_opt() which prints with no prefix and use it for vxlan options. Based on discussion around how to handle new localbypass option. Initial version of this was from Ido Schimmel. Signed-off-by: Stephen Hemminger <stephen@networkplumber.org> --- include/json_print.h | 9 +++++ ip/iplink_vxlan.c | 80 ++++++++++++++++---------------------------- lib/json_print.c | 18 ++++++++++ 3 files changed, 55 insertions(+), 52 deletions(-) diff --git a/include/json_print.h b/include/json_print.h index 91b34571ceb0..4d165a91c23a 100644 --- a/include/json_print.h +++ b/include/json_print.h @@ -101,6 +101,15 @@ static inline int print_rate(bool use_iec, enum output_type t, return print_color_rate(use_iec, t, COLOR_NONE, key, fmt, rate); } +int print_color_bool_opt(enum output_type type, enum color_attr color, + const char *key, bool value); + +static inline int print_bool_opt(enum output_type type, + const char *key, bool value) +{ + return print_color_bool_opt(type, COLOR_NONE, key, value); +} + /* A backdoor to the size formatter. Please use print_size() instead. */ char *sprint_size(__u32 sz, char *buf); diff --git a/ip/iplink_vxlan.c b/ip/iplink_vxlan.c index cb6745c74507..292e19cdb940 100644 --- a/ip/iplink_vxlan.c +++ b/ip/iplink_vxlan.c @@ -427,15 +427,13 @@ static void vxlan_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[]) if (!tb) return; - if (tb[IFLA_VXLAN_COLLECT_METADATA] && - rta_getattr_u8(tb[IFLA_VXLAN_COLLECT_METADATA])) { - print_bool(PRINT_ANY, "external", "external ", true); - } + if (tb[IFLA_VXLAN_COLLECT_METADATA]) + print_bool_opt(PRINT_ANY, "external", + rta_getattr_u8(tb[IFLA_VXLAN_COLLECT_METADATA])); - if (tb[IFLA_VXLAN_VNIFILTER] && - rta_getattr_u8(tb[IFLA_VXLAN_VNIFILTER])) { - print_bool(PRINT_ANY, "vnifilter", "vnifilter", true); - } + if (tb[IFLA_VXLAN_VNIFILTER]) + print_bool_opt(PRINT_ANY, "vnifilter", + rta_getattr_u8(tb[IFLA_VXLAN_VNIFILTER])); if (tb[IFLA_VXLAN_ID] && RTA_PAYLOAD(tb[IFLA_VXLAN_ID]) >= sizeof(__u32)) { @@ -532,22 +530,24 @@ static void vxlan_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[]) if (tb[IFLA_VXLAN_LEARNING]) { __u8 learning = rta_getattr_u8(tb[IFLA_VXLAN_LEARNING]); - print_bool(PRINT_JSON, "learning", NULL, learning); - if (!learning) - print_bool(PRINT_FP, NULL, "nolearning ", true); + print_bool_opt(PRINT_ANY, "learning", learning); } - if (tb[IFLA_VXLAN_PROXY] && rta_getattr_u8(tb[IFLA_VXLAN_PROXY])) - print_bool(PRINT_ANY, "proxy", "proxy ", true); + if (tb[IFLA_VXLAN_PROXY]) + print_bool_opt(PRINT_ANY, "proxy", + rta_getattr_u8(tb[IFLA_VXLAN_PROXY])); - if (tb[IFLA_VXLAN_RSC] && rta_getattr_u8(tb[IFLA_VXLAN_RSC])) - print_bool(PRINT_ANY, "rsc", "rsc ", true); + if (tb[IFLA_VXLAN_RSC]) + print_bool_opt(PRINT_ANY, "rsc", + rta_getattr_u8(tb[IFLA_VXLAN_RSC])); - if (tb[IFLA_VXLAN_L2MISS] && rta_getattr_u8(tb[IFLA_VXLAN_L2MISS])) - print_bool(PRINT_ANY, "l2miss", "l2miss ", true); + if (tb[IFLA_VXLAN_L2MISS]) + print_bool_opt(PRINT_ANY, "l2miss", + rta_getattr_u8(tb[IFLA_VXLAN_L2MISS])); - if (tb[IFLA_VXLAN_L3MISS] && rta_getattr_u8(tb[IFLA_VXLAN_L3MISS])) - print_bool(PRINT_ANY, "l3miss", "l3miss ", true); + if (tb[IFLA_VXLAN_L3MISS]) + print_bool_opt(PRINT_ANY, "l3miss", + rta_getattr_u8(tb[IFLA_VXLAN_L3MISS])); if (tb[IFLA_VXLAN_TOS]) tos = rta_getattr_u8(tb[IFLA_VXLAN_TOS]); @@ -604,51 +604,27 @@ static void vxlan_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[]) if (tb[IFLA_VXLAN_UDP_CSUM]) { __u8 udp_csum = rta_getattr_u8(tb[IFLA_VXLAN_UDP_CSUM]); - if (is_json_context()) { - print_bool(PRINT_ANY, "udp_csum", NULL, udp_csum); - } else { - if (!udp_csum) - fputs("no", f); - fputs("udpcsum ", f); - } + print_bool_opt(PRINT_ANY, "udp_csum", udp_csum); } if (tb[IFLA_VXLAN_UDP_ZERO_CSUM6_TX]) { __u8 csum6 = rta_getattr_u8(tb[IFLA_VXLAN_UDP_ZERO_CSUM6_TX]); - if (is_json_context()) { - print_bool(PRINT_ANY, - "udp_zero_csum6_tx", NULL, csum6); - } else { - if (!csum6) - fputs("no", f); - fputs("udp6zerocsumtx ", f); - } + print_bool_opt(PRINT_ANY, "udp_zero_csum6_tx", csum6); } if (tb[IFLA_VXLAN_UDP_ZERO_CSUM6_RX]) { __u8 csum6 = rta_getattr_u8(tb[IFLA_VXLAN_UDP_ZERO_CSUM6_RX]); - if (is_json_context()) { - print_bool(PRINT_ANY, - "udp_zero_csum6_rx", - NULL, - csum6); - } else { - if (!csum6) - fputs("no", f); - fputs("udp6zerocsumrx ", f); - } + print_bool_opt(PRINT_ANY, "udp_zero_csum6_rx", csum6); } - if (tb[IFLA_VXLAN_REMCSUM_TX] && - rta_getattr_u8(tb[IFLA_VXLAN_REMCSUM_TX])) - print_bool(PRINT_ANY, "remcsum_tx", "remcsumtx ", true); - - if (tb[IFLA_VXLAN_REMCSUM_RX] && - rta_getattr_u8(tb[IFLA_VXLAN_REMCSUM_RX])) - print_bool(PRINT_ANY, "remcsum_rx", "remcsumrx ", true); - + if (tb[IFLA_VXLAN_REMCSUM_TX]) + print_bool_opt(PRINT_ANY, "remcsum_tx", + rta_getattr_u8(tb[IFLA_VXLAN_REMCSUM_TX])); + if (tb[IFLA_VXLAN_REMCSUM_RX]) + print_bool_opt(PRINT_ANY, "remcsum_rx", + rta_getattr_u8(tb[IFLA_VXLAN_REMCSUM_RX])); if (tb[IFLA_VXLAN_GBP]) print_null(PRINT_ANY, "gbp", "gbp ", NULL); if (tb[IFLA_VXLAN_GPE]) diff --git a/lib/json_print.c b/lib/json_print.c index d7ee76b10de8..29959e7335c3 100644 --- a/lib/json_print.c +++ b/lib/json_print.c @@ -215,6 +215,24 @@ int print_color_bool(enum output_type type, value ? "true" : "false"); } +/* In JSON mode, acts like print_color_bool + * otherwise, prints key with no prefix if false + */ +int print_color_bool_opt(enum output_type type, + enum color_attr color, + const char *key, + bool value) +{ + int ret = 0; + + if (_IS_JSON_CONTEXT(type)) + jsonw_bool_field(_jw, key, value); + else if (_IS_FP_CONTEXT(type)) + ret = color_fprintf(stdout, color, "%s%s ", + value ? "" : "no", key); + return ret; +} + int print_color_on_off(enum output_type type, enum color_attr color, const char *key, -- 2.39.2 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [RFC 2/2] vxlan: make option printing more consistent 2023-05-23 16:59 ` [RFC 2/2] vxlan: make option printing more consistent Stephen Hemminger @ 2023-05-24 18:06 ` Andrea Claudi 2023-05-24 18:44 ` Stephen Hemminger 0 siblings, 1 reply; 6+ messages in thread From: Andrea Claudi @ 2023-05-24 18:06 UTC (permalink / raw) To: Stephen Hemminger; +Cc: netdev On Tue, May 23, 2023 at 09:59:32AM -0700, Stephen Hemminger wrote: > Add new helper function print_bool_opt() which prints > with no prefix and use it for vxlan options. > > Based on discussion around how to handle new localbypass option. > Initial version of this was from Ido Schimmel. > > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org> > --- > include/json_print.h | 9 +++++ > ip/iplink_vxlan.c | 80 ++++++++++++++++---------------------------- > lib/json_print.c | 18 ++++++++++ > 3 files changed, 55 insertions(+), 52 deletions(-) > > diff --git a/include/json_print.h b/include/json_print.h > index 91b34571ceb0..4d165a91c23a 100644 > --- a/include/json_print.h > +++ b/include/json_print.h > @@ -101,6 +101,15 @@ static inline int print_rate(bool use_iec, enum output_type t, > return print_color_rate(use_iec, t, COLOR_NONE, key, fmt, rate); > } > > +int print_color_bool_opt(enum output_type type, enum color_attr color, > + const char *key, bool value); > + > +static inline int print_bool_opt(enum output_type type, > + const char *key, bool value) > +{ > + return print_color_bool_opt(type, COLOR_NONE, key, value); > +} > + > /* A backdoor to the size formatter. Please use print_size() instead. */ > char *sprint_size(__u32 sz, char *buf); > > diff --git a/ip/iplink_vxlan.c b/ip/iplink_vxlan.c > index cb6745c74507..292e19cdb940 100644 > --- a/ip/iplink_vxlan.c > +++ b/ip/iplink_vxlan.c > @@ -427,15 +427,13 @@ static void vxlan_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[]) > if (!tb) > return; > > - if (tb[IFLA_VXLAN_COLLECT_METADATA] && > - rta_getattr_u8(tb[IFLA_VXLAN_COLLECT_METADATA])) { > - print_bool(PRINT_ANY, "external", "external ", true); > - } > + if (tb[IFLA_VXLAN_COLLECT_METADATA]) > + print_bool_opt(PRINT_ANY, "external", > + rta_getattr_u8(tb[IFLA_VXLAN_COLLECT_METADATA])); > > - if (tb[IFLA_VXLAN_VNIFILTER] && > - rta_getattr_u8(tb[IFLA_VXLAN_VNIFILTER])) { > - print_bool(PRINT_ANY, "vnifilter", "vnifilter", true); > - } > + if (tb[IFLA_VXLAN_VNIFILTER]) > + print_bool_opt(PRINT_ANY, "vnifilter", > + rta_getattr_u8(tb[IFLA_VXLAN_VNIFILTER])); > > if (tb[IFLA_VXLAN_ID] && > RTA_PAYLOAD(tb[IFLA_VXLAN_ID]) >= sizeof(__u32)) { > @@ -532,22 +530,24 @@ static void vxlan_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[]) > if (tb[IFLA_VXLAN_LEARNING]) { > __u8 learning = rta_getattr_u8(tb[IFLA_VXLAN_LEARNING]); > > - print_bool(PRINT_JSON, "learning", NULL, learning); > - if (!learning) > - print_bool(PRINT_FP, NULL, "nolearning ", true); > + print_bool_opt(PRINT_ANY, "learning", learning); > } > > - if (tb[IFLA_VXLAN_PROXY] && rta_getattr_u8(tb[IFLA_VXLAN_PROXY])) > - print_bool(PRINT_ANY, "proxy", "proxy ", true); > + if (tb[IFLA_VXLAN_PROXY]) > + print_bool_opt(PRINT_ANY, "proxy", > + rta_getattr_u8(tb[IFLA_VXLAN_PROXY])); > > - if (tb[IFLA_VXLAN_RSC] && rta_getattr_u8(tb[IFLA_VXLAN_RSC])) > - print_bool(PRINT_ANY, "rsc", "rsc ", true); > + if (tb[IFLA_VXLAN_RSC]) > + print_bool_opt(PRINT_ANY, "rsc", > + rta_getattr_u8(tb[IFLA_VXLAN_RSC])); > > - if (tb[IFLA_VXLAN_L2MISS] && rta_getattr_u8(tb[IFLA_VXLAN_L2MISS])) > - print_bool(PRINT_ANY, "l2miss", "l2miss ", true); > + if (tb[IFLA_VXLAN_L2MISS]) > + print_bool_opt(PRINT_ANY, "l2miss", > + rta_getattr_u8(tb[IFLA_VXLAN_L2MISS])); > > - if (tb[IFLA_VXLAN_L3MISS] && rta_getattr_u8(tb[IFLA_VXLAN_L3MISS])) > - print_bool(PRINT_ANY, "l3miss", "l3miss ", true); > + if (tb[IFLA_VXLAN_L3MISS]) > + print_bool_opt(PRINT_ANY, "l3miss", > + rta_getattr_u8(tb[IFLA_VXLAN_L3MISS])); > > if (tb[IFLA_VXLAN_TOS]) > tos = rta_getattr_u8(tb[IFLA_VXLAN_TOS]); > @@ -604,51 +604,27 @@ static void vxlan_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[]) > if (tb[IFLA_VXLAN_UDP_CSUM]) { > __u8 udp_csum = rta_getattr_u8(tb[IFLA_VXLAN_UDP_CSUM]); > > - if (is_json_context()) { > - print_bool(PRINT_ANY, "udp_csum", NULL, udp_csum); > - } else { > - if (!udp_csum) > - fputs("no", f); > - fputs("udpcsum ", f); > - } > + print_bool_opt(PRINT_ANY, "udp_csum", udp_csum); > } > > if (tb[IFLA_VXLAN_UDP_ZERO_CSUM6_TX]) { > __u8 csum6 = rta_getattr_u8(tb[IFLA_VXLAN_UDP_ZERO_CSUM6_TX]); > > - if (is_json_context()) { > - print_bool(PRINT_ANY, > - "udp_zero_csum6_tx", NULL, csum6); > - } else { > - if (!csum6) > - fputs("no", f); > - fputs("udp6zerocsumtx ", f); > - } > + print_bool_opt(PRINT_ANY, "udp_zero_csum6_tx", csum6); > } > > if (tb[IFLA_VXLAN_UDP_ZERO_CSUM6_RX]) { > __u8 csum6 = rta_getattr_u8(tb[IFLA_VXLAN_UDP_ZERO_CSUM6_RX]); > > - if (is_json_context()) { > - print_bool(PRINT_ANY, > - "udp_zero_csum6_rx", > - NULL, > - csum6); > - } else { > - if (!csum6) > - fputs("no", f); > - fputs("udp6zerocsumrx ", f); > - } > + print_bool_opt(PRINT_ANY, "udp_zero_csum6_rx", csum6); > } > > - if (tb[IFLA_VXLAN_REMCSUM_TX] && > - rta_getattr_u8(tb[IFLA_VXLAN_REMCSUM_TX])) > - print_bool(PRINT_ANY, "remcsum_tx", "remcsumtx ", true); > - > - if (tb[IFLA_VXLAN_REMCSUM_RX] && > - rta_getattr_u8(tb[IFLA_VXLAN_REMCSUM_RX])) > - print_bool(PRINT_ANY, "remcsum_rx", "remcsumrx ", true); > - > + if (tb[IFLA_VXLAN_REMCSUM_TX]) > + print_bool_opt(PRINT_ANY, "remcsum_tx", > + rta_getattr_u8(tb[IFLA_VXLAN_REMCSUM_TX])); > + if (tb[IFLA_VXLAN_REMCSUM_RX]) > + print_bool_opt(PRINT_ANY, "remcsum_rx", > + rta_getattr_u8(tb[IFLA_VXLAN_REMCSUM_RX])); > if (tb[IFLA_VXLAN_GBP]) > print_null(PRINT_ANY, "gbp", "gbp ", NULL); > if (tb[IFLA_VXLAN_GPE]) > diff --git a/lib/json_print.c b/lib/json_print.c > index d7ee76b10de8..29959e7335c3 100644 > --- a/lib/json_print.c > +++ b/lib/json_print.c > @@ -215,6 +215,24 @@ int print_color_bool(enum output_type type, > value ? "true" : "false"); > } > > +/* In JSON mode, acts like print_color_bool > + * otherwise, prints key with no prefix if false > + */ > +int print_color_bool_opt(enum output_type type, > + enum color_attr color, > + const char *key, > + bool value) > +{ > + int ret = 0; > + > + if (_IS_JSON_CONTEXT(type)) > + jsonw_bool_field(_jw, key, value); > + else if (_IS_FP_CONTEXT(type)) > + ret = color_fprintf(stdout, color, "%s%s ", > + value ? "" : "no", key); > + return ret; > +} > + > int print_color_on_off(enum output_type type, > enum color_attr color, > const char *key, > -- > 2.39.2 > > Thanks Stephen for pointing this series out to me, I overlooked it due to the missing "iproute" in the subject. I'm fine with the JSON result, having all params printed out is much better than the current output. My main objection to this is the non-JSON output result. Let's compare the current output with the one resulting from this RFC: $ ip link add type vxlan id 12 $ ip -d link show vxlan0 79: vxlan0: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN mode DEFAULT group default qlen 1000 link/ether b6:f6:12:c3:2d:52 brd ff:ff:ff:ff:ff:ff promiscuity 0 allmulti 0 minmtu 68 maxmtu 65535 vxlan id 12 srcport 0 0 dstport 8472 ttl auto ageing 300 udpcsum noudp6zerocsumtx noudp6zerocsumrx addrgenmode eui64 numtxqueues 1 numrxqueues 1 gso_max_size 65536 gso_max_segs 65535 tso_max_size 65536 tso_max_segs 65535 gro_max_size 65536 $ ip.new -d link show vxlan0 79: vxlan0: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN mode DEFAULT group default qlen 1000 link/ether b6:f6:12:c3:2d:52 brd ff:ff:ff:ff:ff:ff promiscuity 0 allmulti 0 minmtu 68 maxmtu 65535 vxlan noexternal id 12 srcport 0 0 dstport 8472 learning noproxy norsc nol2miss nol3miss ttl auto ageing 300 udp_csum noudp_zero_csum6_tx noudp_zero_csum6_rx noremcsum_tx noremcsum_rx addrgenmode eui64 numtxqueues 1 numrxqueues 1 gso_max_size 65536 gso_max_segs 65535 tso_max_size 65536 tso_max_segs 65535 gro_max_size 65536 In my opinion, the new output is much longer and less human-readable. The main problem (besides intermixed boolean and numerical params) is that we have a lot of useless info. If the ARP proxy is turned off, what's the use of "noproxy" over there? Let's not print anything at all, I don't expect to find anything about proxy in the output if I'm not asking to have it. It seems to me the same can be said for all the "no"-params over there. What I'm proposing is something along this line: +int print_color_bool_opt(enum output_type type, + enum color_attr color, + const char *key, + bool value) +{ + int ret = 0; + + if (_IS_JSON_CONTEXT(type)) + jsonw_bool_field(_jw, key, value); + else if (_IS_FP_CONTEXT(type) && value) + ret = color_fprintf(stdout, color, "%s ", key); + return ret; +} This should lead to no change in the JSON output w.r.t. this patch, and to this non-JSON output 79: vxlan0: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN mode DEFAULT group default qlen 1000 link/ether b6:f6:12:c3:2d:52 brd ff:ff:ff:ff:ff:ff promiscuity 0 allmulti 0 minmtu 68 maxmtu 65535 vxlan id 12 srcport 0 0 dstport 8472 learning ttl auto ageing 300 udp_csum addrgenmode eui64 numtxqueues 1 numrxqueues 1 gso_max_size 65536 gso_max_segs 65535 tso_max_size 65536 tso_max_segs 65535 gro_max_size 65536 that seems to me much more clear and concise. What do you think? Andrea ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC 2/2] vxlan: make option printing more consistent 2023-05-24 18:06 ` Andrea Claudi @ 2023-05-24 18:44 ` Stephen Hemminger 2023-05-24 21:08 ` Stephen Hemminger 2023-05-25 16:16 ` Andrea Claudi 0 siblings, 2 replies; 6+ messages in thread From: Stephen Hemminger @ 2023-05-24 18:44 UTC (permalink / raw) To: Andrea Claudi; +Cc: netdev On Wed, 24 May 2023 20:06:15 +0200 Andrea Claudi <aclaudi@redhat.com> wrote: > Thanks Stephen for pointing this series out to me, I overlooked it due > to the missing "iproute" in the subject. > > I'm fine with the JSON result, having all params printed out is much > better than the current output. > > My main objection to this is the non-JSON output result. Let's compare > the current output with the one resulting from this RFC: > > $ ip link add type vxlan id 12 > $ ip -d link show vxlan0 > 79: vxlan0: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN mode DEFAULT group default qlen 1000 > link/ether b6:f6:12:c3:2d:52 brd ff:ff:ff:ff:ff:ff promiscuity 0 allmulti 0 minmtu 68 maxmtu 65535 > vxlan id 12 srcport 0 0 dstport 8472 ttl auto ageing 300 udpcsum noudp6zerocsumtx noudp6zerocsumrx addrgenmode eui64 numtxqueues 1 numrxqueues 1 gso_max_size 65536 gso_max_segs 65535 tso_max_size 65536 tso_max_segs 65535 gro_max_size 65536 > > $ ip.new -d link show vxlan0 > 79: vxlan0: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN mode DEFAULT group default qlen 1000 > link/ether b6:f6:12:c3:2d:52 brd ff:ff:ff:ff:ff:ff promiscuity 0 allmulti 0 minmtu 68 maxmtu 65535 > vxlan noexternal id 12 srcport 0 0 dstport 8472 learning noproxy norsc nol2miss nol3miss ttl auto ageing 300 udp_csum noudp_zero_csum6_tx noudp_zero_csum6_rx noremcsum_tx noremcsum_rx addrgenmode eui64 numtxqueues 1 numrxqueues 1 gso_max_size 65536 gso_max_segs 65535 tso_max_size 65536 tso_max_segs 65535 gro_max_size 65536 > > In my opinion, the new output is much longer and less human-readable. > The main problem (besides intermixed boolean and numerical params) is > that we have a lot of useless info. If the ARP proxy is turned off, > what's the use of "noproxy" over there? Let's not print anything at all, > I don't expect to find anything about proxy in the output if I'm not > asking to have it. It seems to me the same can be said for all the > "no"-params over there. > > What I'm proposing is something along this line: > > +int print_color_bool_opt(enum output_type type, > + enum color_attr color, > + const char *key, > + bool value) > +{ > + int ret = 0; > + > + if (_IS_JSON_CONTEXT(type)) > + jsonw_bool_field(_jw, key, value); > + else if (_IS_FP_CONTEXT(type) && value) > + ret = color_fprintf(stdout, color, "%s ", key); > + return ret; > +} > > This should lead to no change in the JSON output w.r.t. this patch, and > to this non-JSON output > > 79: vxlan0: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN mode DEFAULT group default qlen 1000 > link/ether b6:f6:12:c3:2d:52 brd ff:ff:ff:ff:ff:ff promiscuity 0 allmulti 0 minmtu 68 maxmtu 65535 > vxlan id 12 srcport 0 0 dstport 8472 learning ttl auto ageing 300 udp_csum addrgenmode eui64 numtxqueues 1 numrxqueues 1 gso_max_size 65536 gso_max_segs 65535 tso_max_size 65536 tso_max_segs 65535 gro_max_size 65536 > > that seems to me much more clear and concise. > The problem is that one of the options is now by default enabled. The current practice in iproute2 is that the output of the show command must match the equivalent command line used to create the device. There were even some VPN's using that. The proposed localbypass would have similar semantics. The learning option defaults to true, so either it has to be a special case or it needs to be printed only if false. Seems to me that if you ask for details in the output, that showing everything is less surprising, even if it is overly verbose. But the user asked for the details, so show them. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC 2/2] vxlan: make option printing more consistent 2023-05-24 18:44 ` Stephen Hemminger @ 2023-05-24 21:08 ` Stephen Hemminger 2023-05-25 16:16 ` Andrea Claudi 1 sibling, 0 replies; 6+ messages in thread From: Stephen Hemminger @ 2023-05-24 21:08 UTC (permalink / raw) To: Andrea Claudi; +Cc: netdev On Wed, 24 May 2023 11:44:51 -0700 Stephen Hemminger <stephen@networkplumber.org> wrote: > On Wed, 24 May 2023 20:06:15 +0200 > Andrea Claudi <aclaudi@redhat.com> wrote: > > > Thanks Stephen for pointing this series out to me, I overlooked it due > > to the missing "iproute" in the subject. > > > > I'm fine with the JSON result, having all params printed out is much > > better than the current output. > > > > My main objection to this is the non-JSON output result. Let's compare > > the current output with the one resulting from this RFC: > > > > $ ip link add type vxlan id 12 > > $ ip -d link show vxlan0 > > 79: vxlan0: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN mode DEFAULT group default qlen 1000 > > link/ether b6:f6:12:c3:2d:52 brd ff:ff:ff:ff:ff:ff promiscuity 0 allmulti 0 minmtu 68 maxmtu 65535 > > vxlan id 12 srcport 0 0 dstport 8472 ttl auto ageing 300 udpcsum noudp6zerocsumtx noudp6zerocsumrx addrgenmode eui64 numtxqueues 1 numrxqueues 1 gso_max_size 65536 gso_max_segs 65535 tso_max_size 65536 tso_max_segs 65535 gro_max_size 65536 > > > > $ ip.new -d link show vxlan0 > > 79: vxlan0: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN mode DEFAULT group default qlen 1000 > > link/ether b6:f6:12:c3:2d:52 brd ff:ff:ff:ff:ff:ff promiscuity 0 allmulti 0 minmtu 68 maxmtu 65535 > > vxlan noexternal id 12 srcport 0 0 dstport 8472 learning noproxy norsc nol2miss nol3miss ttl auto ageing 300 udp_csum noudp_zero_csum6_tx noudp_zero_csum6_rx noremcsum_tx noremcsum_rx addrgenmode eui64 numtxqueues 1 numrxqueues 1 gso_max_size 65536 gso_max_segs 65535 tso_max_size 65536 tso_max_segs 65535 gro_max_size 65536 > > > > In my opinion, the new output is much longer and less human-readable. > > The main problem (besides intermixed boolean and numerical params) is > > that we have a lot of useless info. If the ARP proxy is turned off, > > what's the use of "noproxy" over there? Let's not print anything at all, > > I don't expect to find anything about proxy in the output if I'm not > > asking to have it. It seems to me the same can be said for all the > > "no"-params over there. > > > > What I'm proposing is something along this line: > > > > +int print_color_bool_opt(enum output_type type, > > + enum color_attr color, > > + const char *key, > > + bool value) > > +{ > > + int ret = 0; > > + > > + if (_IS_JSON_CONTEXT(type)) > > + jsonw_bool_field(_jw, key, value); > > + else if (_IS_FP_CONTEXT(type) && value) > > + ret = color_fprintf(stdout, color, "%s ", key); > > + return ret; > > +} > > > > This should lead to no change in the JSON output w.r.t. this patch, and > > to this non-JSON output > > > > 79: vxlan0: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN mode DEFAULT group default qlen 1000 > > link/ether b6:f6:12:c3:2d:52 brd ff:ff:ff:ff:ff:ff promiscuity 0 allmulti 0 minmtu 68 maxmtu 65535 > > vxlan id 12 srcport 0 0 dstport 8472 learning ttl auto ageing 300 udp_csum addrgenmode eui64 numtxqueues 1 numrxqueues 1 gso_max_size 65536 gso_max_segs 65535 tso_max_size 65536 tso_max_segs 65535 gro_max_size 65536 > > > > that seems to me much more clear and concise. > > > > The problem is that one of the options is now by default enabled. > The current practice in iproute2 is that the output of the show command must match the equivalent > command line used to create the device. There were even some VPN's using that. > The proposed localbypass would have similar semantics. > > The learning option defaults to true, so either it has to be a special case or it needs to be > printed only if false. > > Seems to me that if you ask for details in the output, that showing everything is less surprising, > even if it is overly verbose. But the user asked for the details, so show them. I notice that the number of options in vxlan driver has gotten out of control. There are too many. But the preponderance of nerd knobs to deal with non standard usage is an industry wide problem ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC 2/2] vxlan: make option printing more consistent 2023-05-24 18:44 ` Stephen Hemminger 2023-05-24 21:08 ` Stephen Hemminger @ 2023-05-25 16:16 ` Andrea Claudi 1 sibling, 0 replies; 6+ messages in thread From: Andrea Claudi @ 2023-05-25 16:16 UTC (permalink / raw) To: Stephen Hemminger; +Cc: netdev On Wed, May 24, 2023 at 8:44 PM Stephen Hemminger <stephen@networkplumber.org> wrote: > > On Wed, 24 May 2023 20:06:15 +0200 > Andrea Claudi <aclaudi@redhat.com> wrote: > > > Thanks Stephen for pointing this series out to me, I overlooked it due > > to the missing "iproute" in the subject. > > > > I'm fine with the JSON result, having all params printed out is much > > better than the current output. > > > > My main objection to this is the non-JSON output result. Let's compare > > the current output with the one resulting from this RFC: > > > > $ ip link add type vxlan id 12 > > $ ip -d link show vxlan0 > > 79: vxlan0: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN mode DEFAULT group default qlen 1000 > > link/ether b6:f6:12:c3:2d:52 brd ff:ff:ff:ff:ff:ff promiscuity 0 allmulti 0 minmtu 68 maxmtu 65535 > > vxlan id 12 srcport 0 0 dstport 8472 ttl auto ageing 300 udpcsum noudp6zerocsumtx noudp6zerocsumrx addrgenmode eui64 numtxqueues 1 numrxqueues 1 gso_max_size 65536 gso_max_segs 65535 tso_max_size 65536 tso_max_segs 65535 gro_max_size 65536 > > > > $ ip.new -d link show vxlan0 > > 79: vxlan0: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN mode DEFAULT group default qlen 1000 > > link/ether b6:f6:12:c3:2d:52 brd ff:ff:ff:ff:ff:ff promiscuity 0 allmulti 0 minmtu 68 maxmtu 65535 > > vxlan noexternal id 12 srcport 0 0 dstport 8472 learning noproxy norsc nol2miss nol3miss ttl auto ageing 300 udp_csum noudp_zero_csum6_tx noudp_zero_csum6_rx noremcsum_tx noremcsum_rx addrgenmode eui64 numtxqueues 1 numrxqueues 1 gso_max_size 65536 gso_max_segs 65535 tso_max_size 65536 tso_max_segs 65535 gro_max_size 65536 > > > > In my opinion, the new output is much longer and less human-readable. > > The main problem (besides intermixed boolean and numerical params) is > > that we have a lot of useless info. If the ARP proxy is turned off, > > what's the use of "noproxy" over there? Let's not print anything at all, > > I don't expect to find anything about proxy in the output if I'm not > > asking to have it. It seems to me the same can be said for all the > > "no"-params over there. > > > > What I'm proposing is something along this line: > > > > +int print_color_bool_opt(enum output_type type, > > + enum color_attr color, > > + const char *key, > > + bool value) > > +{ > > + int ret = 0; > > + > > + if (_IS_JSON_CONTEXT(type)) > > + jsonw_bool_field(_jw, key, value); > > + else if (_IS_FP_CONTEXT(type) && value) > > + ret = color_fprintf(stdout, color, "%s ", key); > > + return ret; > > +} > > > > This should lead to no change in the JSON output w.r.t. this patch, and > > to this non-JSON output > > > > 79: vxlan0: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN mode DEFAULT group default qlen 1000 > > link/ether b6:f6:12:c3:2d:52 brd ff:ff:ff:ff:ff:ff promiscuity 0 allmulti 0 minmtu 68 maxmtu 65535 > > vxlan id 12 srcport 0 0 dstport 8472 learning ttl auto ageing 300 udp_csum addrgenmode eui64 numtxqueues 1 numrxqueues 1 gso_max_size 65536 gso_max_segs 65535 tso_max_size 65536 tso_max_segs 65535 gro_max_size 65536 > > > > that seems to me much more clear and concise. > > > > The problem is that one of the options is now by default enabled. > The current practice in iproute2 is that the output of the show command must match the equivalent > command line used to create the device. There were even some VPN's using that. > The proposed localbypass would have similar semantics. That's true even before the "localbypass" option, since we already have the "udpcsum" option enabled by default and printed in the current non-JSON output. So we already have an output that does not match the command line used to create the device. > > The learning option defaults to true, so either it has to be a special case or it needs to be > printed only if false. > > Seems to me that if you ask for details in the output, that showing everything is less surprising, > even if it is overly verbose. But the user asked for the details, so show them. > Fair point. However, I argue that if the user asks for details, we should provide them in easy-to-read format, as far as possible, and avoid to provide info of little use (for example, we don't print "noalias" if there is no alias set). As you say in your other email, there's a trend in growing the number of options in vxlan. As we can do little about that, going down this path can bring us to the point where finding an option can take much more than a quick glance, resulting in a bad user experience. Anyway, whatever you choose, I'm glad iproute2 will finally have some guidelines about the output. :) Regards, Andrea ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-05-25 16:17 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-05-23 16:59 [RFC 1/2] vxlan: use print_nll for gbp and gpe Stephen Hemminger 2023-05-23 16:59 ` [RFC 2/2] vxlan: make option printing more consistent Stephen Hemminger 2023-05-24 18:06 ` Andrea Claudi 2023-05-24 18:44 ` Stephen Hemminger 2023-05-24 21:08 ` Stephen Hemminger 2023-05-25 16:16 ` Andrea Claudi
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).