Re: Question Print Formatting iproute2

[Stephen Hemminger <stephen@networkplumber.org>]

On Mon, 27 Jul 2020 21:31:36 +0200
Petr Machata <petrm@mellanox.com> wrote:

> Briana Oursler <briana.oursler@gmail.com> writes:
> 
> > I git bisected and found d0e450438571("tc: q_red: Add support for
> > qevents "mark" and "early_drop"), the commit that introduced the
> > formatting change causing the break.
> >
> > -       print_string(PRINT_FP, NULL, "max %s ", sprint_size(qopt->qth_max, b3));
> > +       print_string(PRINT_FP, NULL, "max %s", sprint_size(qopt->qth_max, b3));
> >
> > I made a patch that adds a space after the format specifier in the
> > iproute2 tc/q_red.c and tested it using: tdc.py -c qdisc. After the
> > change, all the broken tdc qdisc red tests return ok. I'm including the
> > patch under the scissors line.
> >
> > I wanted to ask the ML if adding the space after the specifier is preferred usage.
> > The commit also had:
> >  -               print_uint(PRINT_ANY, "ewma", "ewma %u ", qopt->Wlog);
> >  +               print_uint(PRINT_ANY, "ewma", " ewma %u ", qopt->Wlog);
> >
> > so I wanted to check with everyone.  
> 
> Yeah, I outsmarted myself with those space changes. Those two chunks
> need reversing, and qevents need to have the space changed. This should
> work:
> 
> modified	  tc/q_red.c
> @@ -222,12 +222,12 @@ static int red_print_opt(struct qdisc_util *qu, FILE *f, struct rtattr *opt)
>  	print_uint(PRINT_JSON, "min", NULL, qopt->qth_min);
>  	print_string(PRINT_FP, NULL, "min %s ", sprint_size(qopt->qth_min, b2));
>  	print_uint(PRINT_JSON, "max", NULL, qopt->qth_max);
> -	print_string(PRINT_FP, NULL, "max %s", sprint_size(qopt->qth_max, b3));
> +	print_string(PRINT_FP, NULL, "max %s ", sprint_size(qopt->qth_max, b3));
> 
>  	tc_red_print_flags(qopt->flags);
> 
>  	if (show_details) {
> -		print_uint(PRINT_ANY, "ewma", " ewma %u ", qopt->Wlog);
> +		print_uint(PRINT_ANY, "ewma", "ewma %u ", qopt->Wlog);
>  		if (max_P)
>  			print_float(PRINT_ANY, "probability",
>  				    "probability %lg ", max_P / pow(2, 32));
> modified	  tc/tc_qevent.c
> @@ -82,8 +82,9 @@ void qevents_print(struct qevent_util *qevents, FILE *f)
>  			}
> 
>  			open_json_object(NULL);
> -			print_string(PRINT_ANY, "kind", " qevent %s", qevents->id);
> +			print_string(PRINT_ANY, "kind", "qevent %s", qevents->id);
>  			qevents->print_qevent(qevents, f);
> +			print_string(PRINT_FP, NULL, "%s", " ");
>  			close_json_object();
>  		}
>  	}
> 
> Are you going to take care of this, or should I?

Missing spaces makes it impossible to read adding extra spaces is annoying, 
From a long term perspective it is better if anything that is trying to
parse output pro grammatically should use JSON output format. With JSON
it is easier to handle new data and not as dependent on ordering.
Plus if some tests used JSON, maybe the issues would be found sooner.