| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <ZG5SF/8CLymhAQsT@renaissance-vector>
Date: Wed, 24 May 2023 20:06:15 +0200
From: Andrea Claudi <aclaudi@...hat.com>
To: Stephen Hemminger <stephen@...workplumber.org>
Cc: netdev@...r.kernel.org
Subject: Re: [RFC 2/2] vxlan: make option printing more consistent
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@...workplumber.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
Powered by blists - more mailing lists