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

On Sun, 26 Jul 2020 21:46:16 -0700
Briana Oursler <briana.oursler@...il.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@...il.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.