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

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

[Vladimir Nikishkin]

Add userspace support for the [no]localbypass vxlan netlink
attribute. With localbypass on (default), the vxlan driver processes
the packets destined to the local machine by itself, bypassing the
userspace nework stack. With nolocalbypass the packets are always
forwarded to the userspace network stack, so userspace programs,
such as tcpdump have a chance to process them.

Signed-off-by: Vladimir Nikishkin <vladimir@nikishkin.pw>
---
v4=>v5: Change the way nolocalbypass option status is printed
        in ip-link to the "new way".

 this patch matches
 commit 69474a8a5837be63f13c6f60a7d622b98ed5c539
 in the main tree

ip/iplink_vxlan.c     | 14 ++++++++++++++
 man/man8/ip-link.8.in | 10 ++++++++++
 2 files changed, 24 insertions(+)

diff --git a/ip/iplink_vxlan.c b/ip/iplink_vxlan.c
index c7e0e1c4..14142fda 100644
--- a/ip/iplink_vxlan.c
+++ b/ip/iplink_vxlan.c
@@ -45,6 +45,7 @@ static void print_explain(FILE *f)
 		"		[ [no]remcsumtx ] [ [no]remcsumrx ]\n"
 		"		[ [no]external ] [ gbp ] [ gpe ]\n"
 		"		[ [no]vnifilter ]\n"
+		"		[ [no]localbypass ]\n"
 		"\n"
 		"Where:	VNI	:= 0-16777215\n"
 		"	ADDR	:= { IP_ADDRESS | any }\n"
@@ -276,6 +277,14 @@ static int vxlan_parse_opt(struct link_util *lu, int argc, char **argv,
 		} else if (!matches(*argv, "noudpcsum")) {
 			check_duparg(&attrs, IFLA_VXLAN_UDP_CSUM, *argv, *argv);
 			addattr8(n, 1024, IFLA_VXLAN_UDP_CSUM, 0);
+		} else if (strcmp(*argv, "localbypass") == 0) {
+			check_duparg(&attrs, IFLA_VXLAN_LOCALBYPASS,
+				     *argv, *argv);
+			addattr8(n, 1024, IFLA_VXLAN_LOCALBYPASS, 1);
+		} else if (strcmp(*argv, "nolocalbypass") == 0) {
+			check_duparg(&attrs, IFLA_VXLAN_LOCALBYPASS,
+				     *argv, *argv);
+			addattr8(n, 1024, IFLA_VXLAN_LOCALBYPASS, 0);
 		} else if (!matches(*argv, "udp6zerocsumtx")) {
 			check_duparg(&attrs, IFLA_VXLAN_UDP_ZERO_CSUM6_TX,
 				     *argv, *argv);
@@ -613,6 +622,11 @@ static void vxlan_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[])
 		}
 	}
 
+	if (tb[IFLA_VXLAN_LOCALBYPASS] &&
+	   rta_getattr_u8(tb[IFLA_VXLAN_LOCALBYPASS])) {
+		print_bool(PRINT_ANY, "localbypass", "localbypass", true);
+	}
+
 	if (tb[IFLA_VXLAN_UDP_ZERO_CSUM6_TX]) {
 		__u8 csum6 = rta_getattr_u8(tb[IFLA_VXLAN_UDP_ZERO_CSUM6_TX]);
 
diff --git a/man/man8/ip-link.8.in b/man/man8/ip-link.8.in
index bf3605a9..27ebeeac 100644
--- a/man/man8/ip-link.8.in
+++ b/man/man8/ip-link.8.in
@@ -630,6 +630,8 @@ the following additional arguments are supported:
 ] [
 .RB [ no ] udpcsum
 ] [
+.RB [ no ] localbypass
+] [
 .RB [ no ] udp6zerocsumtx
 ] [
 .RB [ no ] udp6zerocsumrx
@@ -734,6 +736,14 @@ are entered into the VXLAN device forwarding database.
 .RB [ no ] udpcsum
 - specifies if UDP checksum is calculated for transmitted packets over IPv4.
 
+.sp
+.RB [ no ] localbypass
+- if FDB destination is local, with nolocalbypass set, forward encapsulated
+packets to the userspace network stack. If there is a userspace process
+listening for these packets, it will have a chance to process them. If
+localbypass is active (default), bypass the kernel network stack and
+inject the packets into the target VXLAN device, assuming one exists.
+
 .sp
 .RB [ no ] udp6zerocsumtx
 - skip UDP checksum calculation for transmitted packets over IPv6.
-- 
2.35.8

--
Fastmail.

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

[Ido Schimmel]

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]);

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

[Stephen Hemminger]

On Sun, 21 May 2023 22:23:25 +0300
Ido Schimmel <idosch@idosch.org> wrote:

> +       if (tb[IFLA_VXLAN_LOCALBYPASS])
> +               print_bool(PRINT_ANY, "localbypass", "localbypass ",
> +                          rta_getattr_u8(tb[IFLA_VXLAN_LOCALBYPASS]))

That will not work for non json case.  It will print localbypass whether it is set or not.
The third argument is a format string used in the print routine.

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

[Vladimir Nikishkin]

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.

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

-- 
Your sincerely,
Vladimir Nikishkin (MiEr, lockywolf)
(Laptop)
--
Fastmail.

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

[Ido Schimmel]

On Sun, May 21, 2023 at 12:47:41PM -0700, Stephen Hemminger wrote:
> On Sun, 21 May 2023 22:23:25 +0300
> Ido Schimmel <idosch@idosch.org> wrote:
> 
> > +       if (tb[IFLA_VXLAN_LOCALBYPASS])
> > +               print_bool(PRINT_ANY, "localbypass", "localbypass ",
> > +                          rta_getattr_u8(tb[IFLA_VXLAN_LOCALBYPASS]))
> 
> That will not work for non json case.  It will print localbypass whether it is set or not.
> The third argument is a format string used in the print routine.

Yea, replied too late...

Anyway, my main problem is with the JSON output. Looking at other
boolean VXLAN options, we have at least 3 different formats:

1. Only print when "true" for both JSON and non-JSON output. Used for
"external", "vnifilter", "proxy", "rsc", "l2miss", "l3miss",
"remcsum_tx", "remcsum_rx".

2. Print when both "true" and "false" for both JSON and non-JSON output.
Used for "udp_csum", "udp_zero_csum6_tx", "udp_zero_csum6_rx".

3. Print JSON when both "true" and "false" and non-JSON only when
"false". Used for "learning".

I don't think we should be adding another format. We need to decide:

1. What is the canonical format going forward?

2. Do we change the format of existing options?

My preference is:

1. Format 2. Can be implemented in a common helper used for all VXLAN
options.

2. Yes. It makes all the boolean options consistent and avoids future
discussions such as this where a random option is used for a new option.

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

[Ido Schimmel]

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.

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

[Stephen Hemminger]

On Mon, 22 May 2023 09:15:34 +0300
Ido Schimmel <idosch@idosch.org> wrote:

> On Sun, May 21, 2023 at 12:47:41PM -0700, Stephen Hemminger wrote:
> > On Sun, 21 May 2023 22:23:25 +0300
> > Ido Schimmel <idosch@idosch.org> wrote:
> >   
> > > +       if (tb[IFLA_VXLAN_LOCALBYPASS])
> > > +               print_bool(PRINT_ANY, "localbypass", "localbypass ",
> > > +                          rta_getattr_u8(tb[IFLA_VXLAN_LOCALBYPASS]))  
> > 
> > That will not work for non json case.  It will print localbypass whether it is set or not.
> > The third argument is a format string used in the print routine.  
> 
> Yea, replied too late...
> 
> Anyway, my main problem is with the JSON output. Looking at other
> boolean VXLAN options, we have at least 3 different formats:
> 
> 1. Only print when "true" for both JSON and non-JSON output. Used for
> "external", "vnifilter", "proxy", "rsc", "l2miss", "l3miss",
> "remcsum_tx", "remcsum_rx".
> 
> 2. Print when both "true" and "false" for both JSON and non-JSON output.
> Used for "udp_csum", "udp_zero_csum6_tx", "udp_zero_csum6_rx".
> 
> 3. Print JSON when both "true" and "false" and non-JSON only when
> "false". Used for "learning".
> 
> I don't think we should be adding another format. We need to decide:
> 
> 1. What is the canonical format going forward?
> 
> 2. Do we change the format of existing options?
> 
> My preference is:
> 
> 1. Format 2. Can be implemented in a common helper used for all VXLAN
> options.
> 
> 2. Yes. It makes all the boolean options consistent and avoids future
> discussions such as this where a random option is used for a new option.

A fourth option is to us print_null(). The term null is confusing and people
seem to avoid it.  But it is often used by python programmers as way to represent
options. That would be my preferred option but others seem to disagree.

Option #2 is no good. Any printing of true/false in non-JSON output is a diveregence
from the most common practice across iproute2.

That leaves #3 as the correct and best output.

FYI - The iproute2 maintainers are David Ahern and me. The kernel bits have
other subsystem maintainers.

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

[Petr Machata]

Stephen Hemminger <stephen@networkplumber.org> writes:

> On Mon, 22 May 2023 09:15:34 +0300
> Ido Schimmel <idosch@idosch.org> wrote:
>
>> On Sun, May 21, 2023 at 12:47:41PM -0700, Stephen Hemminger wrote:
>> > On Sun, 21 May 2023 22:23:25 +0300
>> > Ido Schimmel <idosch@idosch.org> wrote:
>> >   
>> > > +       if (tb[IFLA_VXLAN_LOCALBYPASS])
>> > > +               print_bool(PRINT_ANY, "localbypass", "localbypass ",
>> > > +                          rta_getattr_u8(tb[IFLA_VXLAN_LOCALBYPASS]))  
>> > 
>> > That will not work for non json case.  It will print localbypass whether it is set or not.
>> > The third argument is a format string used in the print routine.  
>> 
>> Yea, replied too late...
>> 
>> Anyway, my main problem is with the JSON output. Looking at other
>> boolean VXLAN options, we have at least 3 different formats:
>> 
>> 1. Only print when "true" for both JSON and non-JSON output. Used for
>> "external", "vnifilter", "proxy", "rsc", "l2miss", "l3miss",
>> "remcsum_tx", "remcsum_rx".
>> 
>> 2. Print when both "true" and "false" for both JSON and non-JSON output.
>> Used for "udp_csum", "udp_zero_csum6_tx", "udp_zero_csum6_rx".
>> 
>> 3. Print JSON when both "true" and "false" and non-JSON only when
>> "false". Used for "learning".
>> 
>> I don't think we should be adding another format. We need to decide:
>> 
>> 1. What is the canonical format going forward?
>> 
>> 2. Do we change the format of existing options?
>> 
>> My preference is:
>> 
>> 1. Format 2. Can be implemented in a common helper used for all VXLAN
>> options.
>> 
>> 2. Yes. It makes all the boolean options consistent and avoids future
>> discussions such as this where a random option is used for a new option.
>
> A fourth option is to us print_null(). The term null is confusing and people
> seem to avoid it.  But it is often used by python programmers as way to represent
> options. That would be my preferred option but others seem to disagree.
>
> Option #2 is no good. Any printing of true/false in non-JSON output is a diveregence
> from the most common practice across iproute2.

I think Ido means printing something in both true and false cases, not
using literally the words true and false. That would be a divergence,
yes.

> That leaves #3 as the correct and best output.

The attribute should IMHO be present in JSON output always, even when
the value is false. JSON is for programmatic consumption, and always
having the value available makes the code less error prone. You can hard
expect the value to be there, and just test what it is, instead of
assuming that absence means false, and possibly silently consuming the
wrong object, or interpreting based on an old version of iproute2.

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

[Andrea Claudi]

On Mon, May 22, 2023 at 08:32:16AM -0700, Stephen Hemminger wrote:
> On Mon, 22 May 2023 09:15:34 +0300
> Ido Schimmel <idosch@idosch.org> wrote:
> 
> > On Sun, May 21, 2023 at 12:47:41PM -0700, Stephen Hemminger wrote:
> > > On Sun, 21 May 2023 22:23:25 +0300
> > > Ido Schimmel <idosch@idosch.org> wrote:
> > >   
> > > > +       if (tb[IFLA_VXLAN_LOCALBYPASS])
> > > > +               print_bool(PRINT_ANY, "localbypass", "localbypass ",
> > > > +                          rta_getattr_u8(tb[IFLA_VXLAN_LOCALBYPASS]))  
> > > 
> > > That will not work for non json case.  It will print localbypass whether it is set or not.
> > > The third argument is a format string used in the print routine.  
> > 
> > Yea, replied too late...
> > 
> > Anyway, my main problem is with the JSON output. Looking at other
> > boolean VXLAN options, we have at least 3 different formats:
> > 
> > 1. Only print when "true" for both JSON and non-JSON output. Used for
> > "external", "vnifilter", "proxy", "rsc", "l2miss", "l3miss",
> > "remcsum_tx", "remcsum_rx".
> > 
> > 2. Print when both "true" and "false" for both JSON and non-JSON output.
> > Used for "udp_csum", "udp_zero_csum6_tx", "udp_zero_csum6_rx".
> > 
> > 3. Print JSON when both "true" and "false" and non-JSON only when
> > "false". Used for "learning".
> > 
> > I don't think we should be adding another format. We need to decide:
> > 
> > 1. What is the canonical format going forward?
> > 
> > 2. Do we change the format of existing options?
> > 
> > My preference is:
> > 
> > 1. Format 2. Can be implemented in a common helper used for all VXLAN
> > options.
> > 
> > 2. Yes. It makes all the boolean options consistent and avoids future
> > discussions such as this where a random option is used for a new option.
> 
> A fourth option is to us print_null(). The term null is confusing and people
> seem to avoid it.  But it is often used by python programmers as way to represent
> options. That would be my preferred option but others seem to disagree.
> 
> Option #2 is no good. Any printing of true/false in non-JSON output is a diveregence
> from the most common practice across iproute2.
> 
> That leaves #3 as the correct and best output.
> 
> FYI - The iproute2 maintainers are David Ahern and me. The kernel bits have
> other subsystem maintainers.
> 

Just to make sure I understand correctly, this means we are printing
"nolocalbypass" in non-JSON output because it's the non-default
settings, right?

If this is correct, then if we have another option in the future that
comes disabled by default, this means we are going to print it in
non-JSON output when enabled.

As the primary consumer of non-JSON output are humans, I am a bit
concerned since a succession of enabled/noenabled options is awkward and
difficult to read, in my opinion.

Wouldn't it be better to have non-JSON print out options only when
enabled, regardless of their default value?

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

[Vladimir Nikishkin]

Andrea Claudi <aclaudi@redhat.com> writes:

> On Mon, May 22, 2023 at 08:32:16AM -0700, Stephen Hemminger wrote:
>> On Mon, 22 May 2023 09:15:34 +0300
>> Ido Schimmel <idosch@idosch.org> wrote:
>> 
>> > On Sun, May 21, 2023 at 12:47:41PM -0700, Stephen Hemminger wrote:
>> > > On Sun, 21 May 2023 22:23:25 +0300
>> > > Ido Schimmel <idosch@idosch.org> wrote:
>> > >   
>> > > > +       if (tb[IFLA_VXLAN_LOCALBYPASS])
>> > > > +               print_bool(PRINT_ANY, "localbypass", "localbypass ",
>> > > > +                          rta_getattr_u8(tb[IFLA_VXLAN_LOCALBYPASS]))  
>> > > 
>> > > That will not work for non json case.  It will print localbypass whether it is set or not.
>> > > The third argument is a format string used in the print routine.  
>> > 
>> > Yea, replied too late...
>> > 
>> > Anyway, my main problem is with the JSON output. Looking at other
>> > boolean VXLAN options, we have at least 3 different formats:
>> > 
>> > 1. Only print when "true" for both JSON and non-JSON output. Used for
>> > "external", "vnifilter", "proxy", "rsc", "l2miss", "l3miss",
>> > "remcsum_tx", "remcsum_rx".
>> > 
>> > 2. Print when both "true" and "false" for both JSON and non-JSON output.
>> > Used for "udp_csum", "udp_zero_csum6_tx", "udp_zero_csum6_rx".
>> > 
>> > 3. Print JSON when both "true" and "false" and non-JSON only when
>> > "false". Used for "learning".
>> > 
>> > I don't think we should be adding another format. We need to decide:
>> > 
>> > 1. What is the canonical format going forward?
>> > 
>> > 2. Do we change the format of existing options?
>> > 
>> > My preference is:
>> > 
>> > 1. Format 2. Can be implemented in a common helper used for all VXLAN
>> > options.
>> > 
>> > 2. Yes. It makes all the boolean options consistent and avoids future
>> > discussions such as this where a random option is used for a new option.
>> 
>> A fourth option is to us print_null(). The term null is confusing and people
>> seem to avoid it.  But it is often used by python programmers as way to represent
>> options. That would be my preferred option but others seem to disagree.
>> 
>> Option #2 is no good. Any printing of true/false in non-JSON output is a diveregence
>> from the most common practice across iproute2.
>> 
>> That leaves #3 as the correct and best output.
>> 
>> FYI - The iproute2 maintainers are David Ahern and me. The kernel bits have
>> other subsystem maintainers.
>> 
>
> Just to make sure I understand correctly, this means we are printing
> "nolocalbypass" in non-JSON output because it's the non-default
> settings, right?
>
> If this is correct, then if we have another option in the future that
> comes disabled by default, this means we are going to print it in
> non-JSON output when enabled.
>
> As the primary consumer of non-JSON output are humans, I am a bit
> concerned since a succession of enabled/noenabled options is awkward and
> difficult to read, in my opinion.
>
> Wouldn't it be better to have non-JSON print out options only when
> enabled, regardless of their default value?

Sorry, what is "enabled" and what is "disabled by default"?
I think this is a major source of confusion.

If the option is "nolocalbypass", it is "disabled by default".
If the option is "localbypass", it is "enabled by default".

Intuitively, it seems that everything that is "default" should be
considered disabled, hence the actual option is "nolocalbypass", an by
default it is disabled, and hence not printed. Its opposite requires
explicitly adding a command-line parameter, and hence the "enabled"
state is "nolocalbypass". I think this is the logic that Stephen is
proposing.


-- 
Your sincerely,
Vladimir Nikishkin (MiEr, lockywolf)
(Laptop)
--
Fastmail.

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

[Andrea Claudi]

On Tue, May 23, 2023 at 05:52:30PM +0800, Vladimir Nikishkin wrote:
> 
> Andrea Claudi <aclaudi@redhat.com> writes:
> 
> > On Mon, May 22, 2023 at 08:32:16AM -0700, Stephen Hemminger wrote:
> >> On Mon, 22 May 2023 09:15:34 +0300
> >> Ido Schimmel <idosch@idosch.org> wrote:
> >> 
> >> > On Sun, May 21, 2023 at 12:47:41PM -0700, Stephen Hemminger wrote:
> >> > > On Sun, 21 May 2023 22:23:25 +0300
> >> > > Ido Schimmel <idosch@idosch.org> wrote:
> >> > >   
> >> > > > +       if (tb[IFLA_VXLAN_LOCALBYPASS])
> >> > > > +               print_bool(PRINT_ANY, "localbypass", "localbypass ",
> >> > > > +                          rta_getattr_u8(tb[IFLA_VXLAN_LOCALBYPASS]))  
> >> > > 
> >> > > That will not work for non json case.  It will print localbypass whether it is set or not.
> >> > > The third argument is a format string used in the print routine.  
> >> > 
> >> > Yea, replied too late...
> >> > 
> >> > Anyway, my main problem is with the JSON output. Looking at other
> >> > boolean VXLAN options, we have at least 3 different formats:
> >> > 
> >> > 1. Only print when "true" for both JSON and non-JSON output. Used for
> >> > "external", "vnifilter", "proxy", "rsc", "l2miss", "l3miss",
> >> > "remcsum_tx", "remcsum_rx".
> >> > 
> >> > 2. Print when both "true" and "false" for both JSON and non-JSON output.
> >> > Used for "udp_csum", "udp_zero_csum6_tx", "udp_zero_csum6_rx".
> >> > 
> >> > 3. Print JSON when both "true" and "false" and non-JSON only when
> >> > "false". Used for "learning".
> >> > 
> >> > I don't think we should be adding another format. We need to decide:
> >> > 
> >> > 1. What is the canonical format going forward?
> >> > 
> >> > 2. Do we change the format of existing options?
> >> > 
> >> > My preference is:
> >> > 
> >> > 1. Format 2. Can be implemented in a common helper used for all VXLAN
> >> > options.
> >> > 
> >> > 2. Yes. It makes all the boolean options consistent and avoids future
> >> > discussions such as this where a random option is used for a new option.
> >> 
> >> A fourth option is to us print_null(). The term null is confusing and people
> >> seem to avoid it.  But it is often used by python programmers as way to represent
> >> options. That would be my preferred option but others seem to disagree.
> >> 
> >> Option #2 is no good. Any printing of true/false in non-JSON output is a diveregence
> >> from the most common practice across iproute2.
> >> 
> >> That leaves #3 as the correct and best output.
> >> 
> >> FYI - The iproute2 maintainers are David Ahern and me. The kernel bits have
> >> other subsystem maintainers.
> >> 
> >
> > Just to make sure I understand correctly, this means we are printing
> > "nolocalbypass" in non-JSON output because it's the non-default
> > settings, right?
> >
> > If this is correct, then if we have another option in the future that
> > comes disabled by default, this means we are going to print it in
> > non-JSON output when enabled.
> >
> > As the primary consumer of non-JSON output are humans, I am a bit
> > concerned since a succession of enabled/noenabled options is awkward and
> > difficult to read, in my opinion.
> >
> > Wouldn't it be better to have non-JSON print out options only when
> > enabled, regardless of their default value?
> 
> Sorry, what is "enabled" and what is "disabled by default"?
> I think this is a major source of confusion.
> 
> If the option is "nolocalbypass", it is "disabled by default".
> If the option is "localbypass", it is "enabled by default".
> 
> Intuitively, it seems that everything that is "default" should be
> considered disabled, hence the actual option is "nolocalbypass", an by
> default it is disabled, and hence not printed. Its opposite requires
> explicitly adding a command-line parameter, and hence the "enabled"
> state is "nolocalbypass". I think this is the logic that Stephen is
> proposing.
>

This is indeed confusing, let me try to be more clear.

Let's start considering that we have a single place to store this info,
tb[IFLA_VXLAN_LOCALBYPASS], and this is either true or false.

So, after:

localbypass = rta_getattr_u8(tb[IFLA_VXLAN_LOCALBYPASS]);

you expect localbypass to be true if the user does not modify it, and
you print "nolocalbypass" when instead it is changed to false. Fine.

Now, let's have another option, tb[IFLA_VXLAN_MYOPTION]. Using:

myoption = rta_getattr_u8(tb[IFLA_VXLAN_MYOPTION]);

I expect myoption to be false without user intervention, because this is
how this new option work. I'll print this out only when the user toogle
this to true.

Now, if we decide to print only what happens when the user toogle the
option with a command-line parameter, we may have:

nolocalbypass myoption nooption2 option3 nooption4 ...

which seems to me awkward and difficult to read.

Instead, printing only when true:

myoption option3

This simply says "myoption and option3 are enabled, all the rest is
disabled". It seems to me much more easier to read and understand.

> 
> -- 
> Your sincerely,
> Vladimir Nikishkin (MiEr, lockywolf)
> (Laptop)
> --
> Fastmail.
>