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

[Ido Schimmel <idosch@idosch.org>]

On Mon, May 22, 2023 at 02:03:37PM +0800, Vladimir Nikishkin wrote:
> 
> Ido Schimmel <idosch@idosch.org> writes:
> 
> > On Sun, May 21, 2023 at 01:49:48PM +0800, Vladimir Nikishkin wrote:
> >> +	if (tb[IFLA_VXLAN_LOCALBYPASS] &&
> >> +	   rta_getattr_u8(tb[IFLA_VXLAN_LOCALBYPASS])) {
> >            ^ Unaligned 
> >
> >> +		print_bool(PRINT_ANY, "localbypass", "localbypass", true);
> >
> > Missing space after "localbypass":
> >
> > # ip -d link show dev vxlan0
> > 10: vxlan0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue state UNKNOWN mode DEFAULT group default qlen 1000
> >     link/ether ce:10:63:1d:f3:a8 brd ff:ff:ff:ff:ff:ff promiscuity 0  allmulti 0 minmtu 68 maxmtu 65535 
> >     vxlan id 10 srcport 0 0 dstport 4789 ttl auto ageing 300 udpcsum localbypassnoudp6zerocsumtx [...]
> >
> > Should be:
> >
> > print_bool(PRINT_ANY, "localbypass", "localbypass ", true);
> >
> >> +	}
> >
> > Parenthesis are unnecessary in this case.
> >
> > I disagree with Stephen's comment about "Use presence as a boolean in
> > JSON". I don't like the fact that we don't have JSON output when
> > "false":
> >
> >  # ip -d -j -p link show dev vxlan0 | jq '.[]["linkinfo"]["info_data"]["localbypass"]'
> >  true
> >  # ip link set dev vxlan0 type vxlan nolocalbypass
> >  # ip -d -j -p link show dev vxlan0 | jq '.[]["linkinfo"]["info_data"]["localbypass"]'
> >  null
> >
> > It's inconsistent with other iproute2 utilities such as "bridge" and
> > looks very much like an oversight in the initial JSON output
> > implementation.
> >
> > IOW, I don't have a problem with the code in v4 or simply:
> >
> > @@ -613,6 +622,10 @@ static void vxlan_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[])
> >                 }
> >         }
> >  
> > +       if (tb[IFLA_VXLAN_LOCALBYPASS])
> > +               print_bool(PRINT_ANY, "localbypass", "localbypass ",
> > +                          rta_getattr_u8(tb[IFLA_VXLAN_LOCALBYPASS]));
> > +
> >         if (tb[IFLA_VXLAN_UDP_ZERO_CSUM6_TX]) {
> >                 __u8 csum6 = rta_getattr_u8(tb[IFLA_VXLAN_UDP_ZERO_CSUM6_TX]);
> 
> If the two maintainers disagree, I am even more confused.

Stephen and David are the maintainers.

> 
> Shall I restore version 4? Use print_bool unconditionally, and add the
> json context check back into the code?

Please wait for their reply before sending another version. Ultimately
it's their call. It's better to decide now how we want to handle boolean
VXLAN options than repeating this discussion the next time a new option
is added.