Message-ID: <CAPpH65y84E1Jq=Y8CSTZNAnP-F9c-J=pBF-RPUnT9Um=gFOs2A@mail.gmail.com>
Date: Thu, 25 May 2023 18:16:51 +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 Wed, May 24, 2023 at 8:44 PM Stephen Hemminger
<stephen@...workplumber.org> wrote:
>
> On Wed, 24 May 2023 20:06:15 +0200
> Andrea Claudi <aclaudi@...hat.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.

That's true even before the "localbypass" option, since we already have
the "udpcsum" option enabled by default and printed in the current
non-JSON output. So we already have an output that does not match the
command line used to create the device.

>
> 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.
>

Fair point. However, I argue that if the user asks for details, we
should provide them in easy-to-read format, as far as possible, and
avoid to provide info of little use (for example, we don't print
"noalias" if there is no alias set).

As you say in your other email, there's a trend in growing the number
of options in vxlan. As we can do little about that, going down this path
can bring us to the point where finding an option can take much more
than a quick glance, resulting in a bad user experience.

Anyway, whatever you choose, I'm glad iproute2 will finally have some
guidelines about the output. :)

Regards,
Andrea

Powered by blists - more mailing lists