| Message ID | 20200727044616.735-1-briana.oursler@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | stephen hemminger |
| Headers | show |
| Series | Question Print Formatting iproute2 | expand |
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?
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.
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.
On Mon, 2020-07-27 at 21:31 +0200, Petr Machata 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: > Thank you for the response. I see what you are saying. I had not seen the qevents, I'll put all 3 in. > 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? I will, I'll amend the commit I included so it will have the other changes you suggest and send as a regular patch.
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.
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);
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(-) base-commit: 1ca65af1c5e131861a3989cca3c7ca8b067e0833