netdev - Re: [PATCH iproute2-next v6] ip-link: add support for nolocalbypass in vxlan

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Tue, 23 May 2023 20:37:04 +0200
From: Andrea Claudi <aclaudi@...hat.com>
To: Stephen Hemminger <stephen@...workplumber.org>
Cc: Vladimir Nikishkin <vladimir@...ishkin.pw>, netdev@...r.kernel.org,
	davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
	pabeni@...hat.com, eng.alaamohamedsoliman.am@...il.com,
	gnault@...hat.com, razor@...ckwall.org, idosch@...dia.com,
	liuhangbin@...il.com, eyal.birger@...il.com, jtoppins@...hat.com
Subject: Re: [PATCH iproute2-next v6] ip-link: add support for nolocalbypass
 in vxlan

On Tue, May 23, 2023 at 09:04:41AM -0700, Stephen Hemminger wrote:
> On Tue, 23 May 2023 12:48:05 +0800
> Vladimir Nikishkin <vladimir@...ishkin.pw> wrote:
> 
> > +	if (tb[IFLA_VXLAN_LOCALBYPASS]) {
> > +		__u8 localbypass = rta_getattr_u8(tb[IFLA_VXLAN_LOCALBYPASS]);
> > +
> > +		print_bool(PRINT_JSON, "localbypass", NULL, localbypass);
> > +		if (!localbypass)
> > +			print_bool(PRINT_FP, NULL, "nolocalbypass ", true);
> > +	}
> 
> This is backwards since nolocalbypass is the default.
>

Stephen, I'll try to summarize the discussion we had in v5 here.

- We agree that it's a good idea to have JSON attributes printed both
  when 'true' and 'false'. As Petr said, this makes the code less error
  prone and makes it clear attribute is supported.
- I have some concerns about printing options only when non-default
  values are set. Non-JSON output is mostly consumed by humans, that
  usually expects something to be visible if present/true/enabled. I
  know I'm advocating for a change in the iproute output here, and we
  usually don't do that, but I argue there's value in having a less
  cluttered and confusing output.

  For example, let's take what you see with a default vxlan:
  $ ip link add type vxlan id 12
  $ ip -j link show vxlan0
  [...] udpcsum noudp6zerocsumtx noudp6zerocsumrx [...]

  IMHO printing only "udpcsum" is enough to make the user aware that
  the "udpcsum" feature is enabled and the rest is off.

I'm not against Vladimir's change, of course. But I would be very happy
if we can agree on a direction for the output from now on, and try to
enforce it, maybe deprecating the "old way" to print out stuff step by
step, if we find it useful.

What do you think?
Andrea