Re: Question Print Formatting iproute2

[Briana Oursler <briana.oursler@gmail.com>]

On Mon, 2020-07-27 at 16:47 -0700, Stephen Hemminger wrote:
> On Sun, 26 Jul 2020 21:46:16 -0700
> Briana Oursler <briana.oursler@gmail.com> wrote:
> 
> > I have a patch I've written to address a format specifier change
> > that
> > breaks some tests in tc-testing, but I wanted to ask about the
> > change
> > and for some guidance with respect to how formatters are approached
> > in
> > iproute2. 
> > 
> > On a recent run of tdc tests I ran ./tdc.py -c qdisc and found:
> > 
> > 1..91
> > not ok 1 8b6e - Create RED with no flags
> >         Could not match regex pattern. Verify command output:
> > qdisc red 1: root refcnt 2 limit 1Mb min 100Kb max 300Kb
> > 
> > not ok 2 342e - Create RED with adaptive flag
> >         Could not match regex pattern. Verify command output:
> > qdisc red 1: root refcnt 2 limit 1Mb min 100Kb max 300Kbadaptive
> > 
> > not ok 3 2d4b - Create RED with ECN flag
> >         Could not match regex pattern. Verify command output:
> > qdisc red 1: root refcnt 2 limit 1Mb min 100Kb max 300Kbecn
> > 
> > not ok 4 650f - Create RED with flags ECN, adaptive
> >         Could not match regex pattern. Verify command output:
> > qdisc red 1: root refcnt 2 limit 1Mb min 100Kb max 300Kbecn
> > adaptive
> > 
> > not ok 5 5f15 - Create RED with flags ECN, harddrop
> >         Could not match regex pattern. Verify command output:
> > qdisc red 1: root refcnt 2 limit 1Mb min 100Kb max 300Kbecn
> > harddrop
> > 
> > not ok 6 53e8 - Create RED with flags ECN, nodrop
> >         Could not match regex pattern. Verify command output:
> > qdisc red 1: root refcnt 2 limit 1Mb min 100Kb max 300Kbecn nodrop
> > 
> > ok 7 d091 - Fail to create RED with only nodrop flag
> > not ok 8 af8e - Create RED with flags ECN, nodrop, harddrop
> >         Could not match regex pattern. Verify command output:
> > qdisc red 1: root refcnt 2 limit 1Mb min 100Kb max 300Kbecn
> > harddrop
> > nodrop
> > 
> > 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.
> > 
> > Thanks 
> > > 8--------------------------------------------------------------
> > > ----------8<  
> > From 1e7bee22a799a320bd230ad959d459b386bec26d Mon Sep 17 00:00:00
> > 2001
> > Subject: [RFC iproute2-next] tc: Add space after format specifier
> > 
> > Add space after format specifier in print_string call. Fixes broken
> > qdisc tests within tdc testing suite.
> > 
> > Fixes: d0e450438571("tc: q_red: Add support for
> > qevents "mark" and "early_drop")
> > 
> > Signed-off-by: Briana Oursler <briana.oursler@gmail.com>
> > ---
> >  tc/q_red.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/tc/q_red.c b/tc/q_red.c
> > index dfef1bf8..7106645a 100644
> > --- a/tc/q_red.c
> > +++ b/tc/q_red.c
> > @@ -222,7 +222,7 @@ 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);
> >  
> > 
> > base-commit: 1ca65af1c5e131861a3989cca3c7ca8b067e0833
> 
> Looks fine, please resend a normal patch targeted at current iproute2
> not next.
> 
Will do, thank you.