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

[Stephen Hemminger <stephen@networkplumber.org>]

On Fri, 26 May 2023 21:42:17 +0200
Andrea Claudi <aclaudi@redhat.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@networkplumber.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.