netdev - Re: Question Print Formatting iproute2

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: <20200727133730.344d583e@hermes.lan>
Date:   Mon, 27 Jul 2020 13:37:30 -0700
From:   Stephen Hemminger <stephen@...workplumber.org>
To:     Petr Machata <petrm@...lanox.com>
Cc:     Briana Oursler <briana.oursler@...il.com>, netdev@...r.kernel.org,
        Alexey Kuznetsov <kuznet@....inr.ac.ru>,
        Davide Caratti <dcaratti@...hat.com>,
        Stefano Brivio <sbrivio@...hat.com>,
        Jiri Pirko <jiri@...lanox.com>
Subject: Re: Question Print Formatting iproute2

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

> Briana Oursler <briana.oursler@...il.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.