Re: [RFC 2/2] vxlan: make option printing more consistent

[Stephen Hemminger <stephen@networkplumber.org>]

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.