netdev - Re: [PATCH iproute2 v4 2/2] vxlan: make option printing more consistent

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]

Message-ID: <20230526151736.633919c0@hermes.local>
Date: Fri, 26 May 2023 15:17:36 -0700
From: Stephen Hemminger <stephen@...workplumber.org>
To: Andrea Claudi <aclaudi@...hat.com>
Cc: netdev@...r.kernel.org
Subject: Re: [PATCH iproute2 v4 2/2] vxlan: make option printing more
 consistent

On Fri, 26 May 2023 21:42:17 +0200
Andrea Claudi <aclaudi@...hat.com> wrote:

> On Fri, May 26, 2023 at 10:41:41AM -0700, Stephen Hemminger wrote:
> > Add new helper function print_bool_opt() which prints
> > with no prefix and use it for vxlan options.
> > 
> > If the option matches the expected default value,
> > it is not printed if in non JSON mode unless the details
> > setting is repeated.
> > 
> > Use a table for the vxlan options. This will change
> > the order of the printing of options.
> > 
> > Signed-off-by: Stephen Hemminger <stephen@...workplumber.org>
> > ---
> >  include/json_print.h |   9 ++++
> >  ip/iplink_vxlan.c    | 110 +++++++++++++------------------------------
> >  lib/json_print.c     |  19 ++++++++
> >  3 files changed, 60 insertions(+), 78 deletions(-)
> > 
> > diff --git a/include/json_print.h b/include/json_print.h
> > index 91b34571ceb0..49d3cc14789c 100644
> > --- a/include/json_print.h
> > +++ b/include/json_print.h
> > @@ -101,6 +101,15 @@ static inline int print_rate(bool use_iec, enum output_type t,
> >  	return print_color_rate(use_iec, t, COLOR_NONE, key, fmt, rate);
> >  }
> >  
> > +int print_color_bool_opt(enum output_type type, enum color_attr color,
> > +			 const char *key, bool value, bool show);
> > +
> > +static inline int print_bool_opt(enum output_type type,
> > +				 const char *key, bool value, bool show)
> > +{
> > +	return print_color_bool_opt(type, COLOR_NONE, key, value, show);
> > +}
> > +
> >  /* A backdoor to the size formatter. Please use print_size() instead. */
> >  char *sprint_size(__u32 sz, char *buf);
> >  
> > diff --git a/ip/iplink_vxlan.c b/ip/iplink_vxlan.c
> > index cb6745c74507..e77c3aa2e3a5 100644
> > --- a/ip/iplink_vxlan.c
> > +++ b/ip/iplink_vxlan.c
> > @@ -19,6 +19,25 @@
> >  
> >  #define VXLAN_ATTRSET(attrs, type) (((attrs) & (1L << (type))) != 0)
> >  
> > +static const struct vxlan_bool_opt {
> > +	const char *key;
> > +	int type;
> > +	bool default_value;
> > +} vxlan_opts[] = {
> > +	{ "metadata",	IFLA_VXLAN_COLLECT_METADATA,	false },  
> 
> Here you are changing the output from "external" to "metadata", while
> continuing to use "external" to toggle the option. This may surprise a
> user which checks the output for this option after enabling it.
> 
> Moreover, looking at the man page for ip link, this option is used to
> specify "whether an external control plane (e.g. ip route encap) or the
> internal FDB should be used", so maybe "external" is more close to the
> true meaning of this option.
> 
> I would suggest either to continue using "external" or to switch to
> "metadata" also for option toggling (and update the manpage, too).
> 
> All the rest looks good to me.
>

Thanks will fix that.