[PATCH iproute2 v3 0/2] add json support on tc u32

[PATCH iproute2 v3 0/2] add json support on tc u32

[Wen Liang]

This adds support for json output on tc u32.
- The first patch is replacing with proper json functions in `u32_print_opt()`
- The second patch is fixing the json support in u32 `print_raw()`, `print_ipv4()`
  and `print_ipv6()`

Wen Liang (2):
  tc: u32: add support for json output
  tc: u32: add json support in `print_raw`, `print_ipv4`, `print_ipv6`

 tc/f_u32.c | 173 +++++++++++++++++++++++++++++------------------------
 1 file changed, 96 insertions(+), 77 deletions(-)

-- 
2.26.3

[PATCH iproute2 v3 1/2] tc: u32: add support for json output

[Wen Liang]

Currently u32 filter output does not support json. This commit uses
proper json functions to add support for it.

Signed-off-by: Wen Liang <liangwen12year@gmail.com>
---
 tc/f_u32.c | 72 ++++++++++++++++++++++++++++--------------------------
 1 file changed, 37 insertions(+), 35 deletions(-)

diff --git a/tc/f_u32.c b/tc/f_u32.c
index a5747f67..be16ba1e 100644
--- a/tc/f_u32.c
+++ b/tc/f_u32.c
@@ -1213,11 +1213,11 @@ static int u32_print_opt(struct filter_util *qu, FILE *f, struct rtattr *opt,
 
 	if (handle) {
 		SPRINT_BUF(b1);
-		fprintf(f, "fh %s ", sprint_u32_handle(handle, b1));
+		print_string(PRINT_ANY, "fh", "fh %s ", sprint_u32_handle(handle, b1));
 	}
 
 	if (TC_U32_NODE(handle))
-		fprintf(f, "order %d ", TC_U32_NODE(handle));
+		print_int(PRINT_ANY, "order", "order %d ", TC_U32_NODE(handle));
 
 	if (tb[TCA_U32_SEL]) {
 		if (RTA_PAYLOAD(tb[TCA_U32_SEL])  < sizeof(*sel))
@@ -1227,15 +1227,13 @@ static int u32_print_opt(struct filter_util *qu, FILE *f, struct rtattr *opt,
 	}
 
 	if (tb[TCA_U32_DIVISOR]) {
-		fprintf(f, "ht divisor %d ",
-			rta_getattr_u32(tb[TCA_U32_DIVISOR]));
+		print_int(PRINT_ANY, "ht_divisor", "ht divisor %d ", rta_getattr_u32(tb[TCA_U32_DIVISOR]));
 	} else if (tb[TCA_U32_HASH]) {
 		__u32 htid = rta_getattr_u32(tb[TCA_U32_HASH]);
-
-		fprintf(f, "key ht %x bkt %x ", TC_U32_USERHTID(htid),
-			TC_U32_HASH(htid));
+		print_hex(PRINT_ANY, "key_ht", "key ht %x ", TC_U32_USERHTID(htid));
+		print_hex(PRINT_ANY, "bkt", "bkt %x ", TC_U32_HASH(htid));
 	} else {
-		fprintf(f, "??? ");
+		fprintf(stderr, "divisor and hash missing ");
 	}
 	if (tb[TCA_U32_CLASSID]) {
 		SPRINT_BUF(b1);
@@ -1244,12 +1242,11 @@ static int u32_print_opt(struct filter_util *qu, FILE *f, struct rtattr *opt,
 			sprint_tc_classid(rta_getattr_u32(tb[TCA_U32_CLASSID]),
 					  b1));
 	} else if (sel && sel->flags & TC_U32_TERMINAL) {
-		fprintf(f, "terminal flowid ??? ");
+		print_bool(PRINT_ANY, "terminal_flowid", "terminal flowid ??? ", true);
 	}
 	if (tb[TCA_U32_LINK]) {
 		SPRINT_BUF(b1);
-		fprintf(f, "link %s ",
-			sprint_u32_handle(rta_getattr_u32(tb[TCA_U32_LINK]),
+		print_string(PRINT_ANY, "link", "link %s ", sprint_u32_handle(rta_getattr_u32(tb[TCA_U32_LINK]),
 					  b1));
 	}
 
@@ -1257,14 +1254,14 @@ static int u32_print_opt(struct filter_util *qu, FILE *f, struct rtattr *opt,
 		__u32 flags = rta_getattr_u32(tb[TCA_U32_FLAGS]);
 
 		if (flags & TCA_CLS_FLAGS_SKIP_HW)
-			fprintf(f, "skip_hw ");
+			print_bool(PRINT_ANY, "skip_hw", "skip_hw ", true);
 		if (flags & TCA_CLS_FLAGS_SKIP_SW)
-			fprintf(f, "skip_sw ");
+			print_bool(PRINT_ANY, "skip_sw", "skip_sw ", true);
 
 		if (flags & TCA_CLS_FLAGS_IN_HW)
-			fprintf(f, "in_hw ");
+			print_bool(PRINT_ANY, "in_hw", "in_hw ", true);
 		else if (flags & TCA_CLS_FLAGS_NOT_IN_HW)
-			fprintf(f, "not_in_hw ");
+			print_bool(PRINT_ANY, "not_in_hw", "not_in_hw ", true);
 	}
 
 	if (tb[TCA_U32_PCNT]) {
@@ -1275,10 +1272,10 @@ static int u32_print_opt(struct filter_util *qu, FILE *f, struct rtattr *opt,
 		pf = RTA_DATA(tb[TCA_U32_PCNT]);
 	}
 
-	if (sel && show_stats && NULL != pf)
-		fprintf(f, " (rule hit %llu success %llu)",
-			(unsigned long long) pf->rcnt,
-			(unsigned long long) pf->rhit);
+	if (sel && show_stats && NULL != pf) {
+		print_u64(PRINT_ANY, "rule_hit", "(rule hit %llu ", pf->rcnt);
+		print_u64(PRINT_ANY, "success", "success %llu)", pf->rhit);
+	}
 
 	if (tb[TCA_U32_MARK]) {
 		struct tc_u32_mark *mark = RTA_DATA(tb[TCA_U32_MARK]);
@@ -1286,8 +1283,10 @@ static int u32_print_opt(struct filter_util *qu, FILE *f, struct rtattr *opt,
 		if (RTA_PAYLOAD(tb[TCA_U32_MARK]) < sizeof(*mark)) {
 			fprintf(f, "\n  Invalid mark (kernel&iproute2 mismatch)\n");
 		} else {
-			fprintf(f, "\n  mark 0x%04x 0x%04x (success %d)",
-				mark->val, mark->mask, mark->success);
+			print_nl();
+			print_0xhex(PRINT_ANY, "fwmark_value", "  mark 0x%04x ", mark->val);
+			print_0xhex(PRINT_ANY, "fwmark_mask", "0x%04x ", mark->mask);
+			print_int(PRINT_ANY, "fwmark_success", "(success %d)", mark->success);
 		}
 	}
 
@@ -1298,38 +1297,41 @@ static int u32_print_opt(struct filter_util *qu, FILE *f, struct rtattr *opt,
 			for (i = 0; i < sel->nkeys; i++) {
 				show_keys(f, sel->keys + i);
 				if (show_stats && NULL != pf)
-					fprintf(f, " (success %llu ) ",
-						(unsigned long long) pf->kcnts[i]);
+					print_u64(PRINT_ANY, "success", " (success %llu ) ", pf->kcnts[i]);
 			}
 		}
 
 		if (sel->flags & (TC_U32_VAROFFSET | TC_U32_OFFSET)) {
-			fprintf(f, "\n    offset ");
-			if (sel->flags & TC_U32_VAROFFSET)
-				fprintf(f, "%04x>>%d at %d ",
-					ntohs(sel->offmask),
-					sel->offshift,  sel->offoff);
+			print_nl();
+			print_string(PRINT_ANY, NULL, "%s", "    offset ");
+			if (sel->flags & TC_U32_VAROFFSET) {
+				print_hex(PRINT_ANY, "offset_mask", "%04x", ntohs(sel->offmask));
+				print_int(PRINT_ANY, "offset_shift", ">>%d ", sel->offshift);
+				print_int(PRINT_ANY, "offset_off", "at %d ", sel->offoff);
+			}
 			if (sel->off)
-				fprintf(f, "plus %d ", sel->off);
+				print_int(PRINT_ANY, "plus", "plus %d ", sel->off);
 		}
 		if (sel->flags & TC_U32_EAT)
-			fprintf(f, " eat ");
+			print_string(PRINT_ANY, NULL, "%s", " eat ");
 
 		if (sel->hmask) {
-			fprintf(f, "\n    hash mask %08x at %d ",
-				(unsigned int)htonl(sel->hmask), sel->hoff);
+			print_nl();
+			print_hex(PRINT_ANY, "hash_mask", "    hash mask %08x ", (unsigned int)htonl(sel->hmask));
+			print_int(PRINT_ANY, "hash_off", "at %d ", sel->hoff);
 		}
 	}
 
 	if (tb[TCA_U32_POLICE]) {
-		fprintf(f, "\n");
+		print_nl();
 		tc_print_police(f, tb[TCA_U32_POLICE]);
 	}
 
 	if (tb[TCA_U32_INDEV]) {
 		struct rtattr *idev = tb[TCA_U32_INDEV];
-
-		fprintf(f, "\n  input dev %s\n", rta_getattr_str(idev));
+		print_nl();
+		print_string(PRINT_ANY, "input_dev", "  input dev %s", rta_getattr_str(idev));
+		print_nl();
 	}
 
 	if (tb[TCA_U32_ACT])
-- 
2.26.3

[PATCH iproute2 v3 2/2] tc: u32: add json support in `print_raw`, `print_ipv4`, `print_ipv6`

[Wen Liang]

Signed-off-by: Wen Liang <liangwen12year@gmail.com>
---
 tc/f_u32.c | 101 +++++++++++++++++++++++++++++++----------------------
 1 file changed, 59 insertions(+), 42 deletions(-)

diff --git a/tc/f_u32.c b/tc/f_u32.c
index be16ba1e..1dd9bfcc 100644
--- a/tc/f_u32.c
+++ b/tc/f_u32.c
@@ -824,23 +824,24 @@ static void print_ipv4(FILE *f, const struct tc_u32_key *key)
 {
 	char abuf[256];
 
+	open_json_object("match");
 	switch (key->off) {
 	case 0:
 		switch (ntohl(key->mask)) {
 		case 0x0f000000:
-			fprintf(f, "\n  match IP ihl %u",
-				ntohl(key->val) >> 24);
+			print_nl();
+			print_uint(PRINT_ANY, "ip_ihl", "  match IP ihl %u", ntohl(key->val) >> 24);
 			return;
 		case 0x00ff0000:
-			fprintf(f, "\n  match IP dsfield %#x",
-				ntohl(key->val) >> 16);
+			print_nl();
+			print_0xhex(PRINT_ANY, "ip_dsfield", "  match IP dsfield %#x", ntohl(key->val) >> 16);
 			return;
 		}
 		break;
 	case 8:
 		if (ntohl(key->mask) == 0x00ff0000) {
-			fprintf(f, "\n  match IP protocol %d",
-				ntohl(key->val) >> 16);
+			print_nl();
+			print_int(PRINT_ANY, "ip_protocol", "  match IP protocol %d", ntohl(key->val) >> 16);
 			return;
 		}
 		break;
@@ -849,11 +850,18 @@ static void print_ipv4(FILE *f, const struct tc_u32_key *key)
 			int bits = mask2bits(key->mask);
 
 			if (bits >= 0) {
-				fprintf(f, "\n  %s %s/%d",
-					key->off == 12 ? "match IP src" : "match IP dst",
-					inet_ntop(AF_INET, &key->val,
-						  abuf, sizeof(abuf)),
-					bits);
+				 if (key->off == 12) {
+				       print_nl();
+				       print_null(PRINT_FP, NULL, "  match IP src ", NULL);
+				       open_json_object("src");
+				 } else {
+				       print_nl();
+				       print_null(PRINT_FP, NULL, "  match IP dst ", NULL);
+				       open_json_object("dst");
+				 }
+				 print_string(PRINT_ANY, "address", "%s", inet_ntop(AF_INET, &key->val, abuf, sizeof(abuf)));
+				 print_int(PRINT_ANY, "prefixlen", "/%d", bits);
+				 close_json_object();
 				return;
 			}
 		}
@@ -862,45 +870,45 @@ static void print_ipv4(FILE *f, const struct tc_u32_key *key)
 	case 20:
 		switch (ntohl(key->mask)) {
 		case 0x0000ffff:
-			fprintf(f, "\n  match dport %u",
-				ntohl(key->val) & 0xffff);
+			print_uint(PRINT_ANY, "dport", "match dport %u", ntohl(key->val) & 0xffff);
 			return;
 		case 0xffff0000:
-			fprintf(f, "\n  match sport %u",
-				ntohl(key->val) >> 16);
+			print_nl();
+			print_uint(PRINT_ANY, "sport", "  match sport %u", ntohl(key->val) >> 16);
 			return;
 		case 0xffffffff:
-			fprintf(f, "\n  match dport %u, match sport %u",
-				ntohl(key->val) & 0xffff,
-				ntohl(key->val) >> 16);
-
+			print_nl();
+			print_uint(PRINT_ANY, "dport", "  match dport %u, ", ntohl(key->val) & 0xffff);
+			print_uint(PRINT_ANY, "sport", "match sport %u", ntohl(key->val) >> 16);
 			return;
 		}
 		/* XXX: Default print_raw */
 	}
+	close_json_object();
 }
 
 static void print_ipv6(FILE *f, const struct tc_u32_key *key)
 {
 	char abuf[256];
 
+	open_json_object("match");
 	switch (key->off) {
 	case 0:
 		switch (ntohl(key->mask)) {
 		case 0x0f000000:
-			fprintf(f, "\n  match IP ihl %u",
-				ntohl(key->val) >> 24);
+			print_nl();
+			print_uint(PRINT_ANY, "ip_ihl", "  match IP ihl %u", ntohl(key->val) >> 24);
 			return;
 		case 0x00ff0000:
-			fprintf(f, "\n  match IP dsfield %#x",
-				ntohl(key->val) >> 16);
+			print_nl();
+			print_0xhex(PRINT_ANY, "ip_dsfield", "  match IP dsfield %#x", ntohl(key->val) >> 16);
 			return;
 		}
 		break;
 	case 8:
 		if (ntohl(key->mask) == 0x00ff0000) {
-			fprintf(f, "\n  match IP protocol %d",
-				ntohl(key->val) >> 16);
+			print_nl();
+			print_int(PRINT_ANY, "ip_protocol", "  match IP protocol %d", ntohl(key->val) >> 16);
 			return;
 		}
 		break;
@@ -909,11 +917,18 @@ static void print_ipv6(FILE *f, const struct tc_u32_key *key)
 			int bits = mask2bits(key->mask);
 
 			if (bits >= 0) {
-				fprintf(f, "\n  %s %s/%d",
-					key->off == 12 ? "match IP src" : "match IP dst",
-					inet_ntop(AF_INET, &key->val,
-						  abuf, sizeof(abuf)),
-					bits);
+				if (key->off == 12) {
+			              print_nl();
+				      print_null(PRINT_FP, NULL, "  match IP src ", NULL);
+				      open_json_object("src");
+				} else {
+			              print_nl();
+				      print_null(PRINT_FP, NULL, "  match IP dst ", NULL);
+				      open_json_object("dst");
+				}
+				print_string(PRINT_ANY, "address", "%s", inet_ntop(AF_INET, &key->val, abuf, sizeof(abuf)));
+				print_int(PRINT_ANY, "prefixlen", "/%d", bits);
+				close_json_object();
 				return;
 			}
 		}
@@ -922,31 +937,33 @@ static void print_ipv6(FILE *f, const struct tc_u32_key *key)
 	case 20:
 		switch (ntohl(key->mask)) {
 		case 0x0000ffff:
-			fprintf(f, "\n  match sport %u",
-				ntohl(key->val) & 0xffff);
+			print_nl();
+			print_uint(PRINT_ANY, "sport", "  match sport %u", ntohl(key->val) & 0xffff);
 			return;
 		case 0xffff0000:
-			fprintf(f, "\n  match dport %u",
-				ntohl(key->val) >> 16);
+			print_uint(PRINT_ANY, "dport", "match dport %u", ntohl(key->val) >> 16);
 			return;
 		case 0xffffffff:
-			fprintf(f, "\n  match sport %u, match dport %u",
-				ntohl(key->val) & 0xffff,
-				ntohl(key->val) >> 16);
+			print_nl();
+			print_uint(PRINT_ANY, "sport", "  match sport %u, ", ntohl(key->val) & 0xffff);
+			print_uint(PRINT_ANY, "dport", "match dport %u", ntohl(key->val) >> 16);
 
 			return;
 		}
 		/* XXX: Default print_raw */
 	}
+	close_json_object();
 }
 
 static void print_raw(FILE *f, const struct tc_u32_key *key)
 {
-	fprintf(f, "\n  match %08x/%08x at %s%d",
-		(unsigned int)ntohl(key->val),
-		(unsigned int)ntohl(key->mask),
-		key->offmask ? "nexthdr+" : "",
-		key->off);
+	open_json_object("match");
+	print_nl();
+	print_hex(PRINT_ANY, "value", "  match %08x", (unsigned int)ntohl(key->val));
+	print_hex(PRINT_ANY, "mask", "/%08x ", (unsigned int)ntohl(key->mask));
+	print_string(PRINT_ANY, "offmask", "at %s", key->offmask ? "nexthdr+" : "");
+	print_int(PRINT_ANY, "off", "%d", key->off);
+	close_json_object();
 }
 
 static const struct {
-- 
2.26.3

Re: [PATCH iproute2 v3 1/2] tc: u32: add support for json output

[Stephen Hemminger]

On Thu,  6 Jan 2022 13:45:51 -0500
Wen Liang <liangwen12year@gmail.com> wrote:

>  	} else if (sel && sel->flags & TC_U32_TERMINAL) {
> -		fprintf(f, "terminal flowid ??? ");
> +		print_bool(PRINT_ANY, "terminal_flowid", "terminal flowid ??? ", true);

This looks like another error (ie to stderr) like the earlier case

>  	if (tb[TCA_U32_LINK]) {
>  		SPRINT_BUF(b1);
> -		fprintf(f, "link %s ",
> -			sprint_u32_handle(rta_getattr_u32(tb[TCA_U32_LINK]),
> +		print_string(PRINT_ANY, "link", "link %s ", sprint_u32_handle(rta_getattr_u32(tb[TCA_U32_LINK]),
>  					  b1));

Break that long line up. Would look better with a temporary variable.

FYI - good test is to run the json output into python's json parser to make sure it is valid.

Re: [PATCH iproute2 v3 1/2] tc: u32: add support for json output

[David Ahern]

On 1/6/22 3:30 PM, Stephen Hemminger wrote:
>>  	if (tb[TCA_U32_LINK]) {
>>  		SPRINT_BUF(b1);
>> -		fprintf(f, "link %s ",
>> -			sprint_u32_handle(rta_getattr_u32(tb[TCA_U32_LINK]),
>> +		print_string(PRINT_ANY, "link", "link %s ", sprint_u32_handle(rta_getattr_u32(tb[TCA_U32_LINK]),
>>  					  b1));
> 
> Break that long line up. Would look better with a temporary variable.

+1 and that comment applies to all lines in both patches.

Re: [PATCH iproute2 v3 1/2] tc: u32: add support for json output

[Andrea Claudi]

On Thu, Jan 06, 2022 at 02:30:13PM -0800, Stephen Hemminger wrote:
> On Thu,  6 Jan 2022 13:45:51 -0500
> Wen Liang <liangwen12year@gmail.com> wrote:
> 
> >  	} else if (sel && sel->flags & TC_U32_TERMINAL) {
> > -		fprintf(f, "terminal flowid ??? ");
> > +		print_bool(PRINT_ANY, "terminal_flowid", "terminal flowid ??? ", true);
> 
> This looks like another error (ie to stderr) like the earlier case
>

Hi Stephen,
Sorry for coming to this so late, but this doesn't look like an error to me.

As far as I can see, TC_U32_TERMINAL is set in this file together with
CLASSID or when "action" or "policy" are used. The latter case should be
the one that this else branch should catch.

Now, "terminal flowid ???" looks to me like a message printed when we
don't actually have a flowid to show, and indeed that is specified when
this flag is set (see the comment at line 1169). As such this is
probably more a useless log message, than an error one.

If this is the case, we can probably maintain this message on the
PRINT_FP output (only to not break script parsing this bit of info out
there), and disregard this bit of info on the JSON output.

What do you think?

Regards,
Andrea

Re: [PATCH iproute2 v3 1/2] tc: u32: add support for json output

[Stephen Hemminger]

On Mon, 24 Jan 2022 19:25:06 +0100
Andrea Claudi <aclaudi@redhat.com> wrote:

> On Thu, Jan 06, 2022 at 02:30:13PM -0800, Stephen Hemminger wrote:
> > On Thu,  6 Jan 2022 13:45:51 -0500
> > Wen Liang <liangwen12year@gmail.com> wrote:
> >   
> > >  	} else if (sel && sel->flags & TC_U32_TERMINAL) {
> > > -		fprintf(f, "terminal flowid ??? ");
> > > +		print_bool(PRINT_ANY, "terminal_flowid", "terminal flowid ??? ", true);  
> > 
> > This looks like another error (ie to stderr) like the earlier case
> >  
> 
> Hi Stephen,
> Sorry for coming to this so late, but this doesn't look like an error to me.
> 
> As far as I can see, TC_U32_TERMINAL is set in this file together with
> CLASSID or when "action" or "policy" are used. The latter case should be
> the one that this else branch should catch.
> 
> Now, "terminal flowid ???" looks to me like a message printed when we
> don't actually have a flowid to show, and indeed that is specified when
> this flag is set (see the comment at line 1169). As such this is
> probably more a useless log message, than an error one.
> 
> If this is the case, we can probably maintain this message on the
> PRINT_FP output (only to not break script parsing this bit of info out
> there), and disregard this bit of info on the JSON output.
> 
> What do you think?
> 
> Regards,
> Andrea
> 

Just always put the same original message on stderr.

Re: [PATCH iproute2 v3 1/2] tc: u32: add support for json output

[Andrea Claudi]

On Mon, Jan 24, 2022 at 10:50:16AM -0800, Stephen Hemminger wrote:
> On Mon, 24 Jan 2022 19:25:06 +0100
> Andrea Claudi <aclaudi@redhat.com> wrote:
> 
> > On Thu, Jan 06, 2022 at 02:30:13PM -0800, Stephen Hemminger wrote:
> > > On Thu,  6 Jan 2022 13:45:51 -0500
> > > Wen Liang <liangwen12year@gmail.com> wrote:
> > >   
> > > >  	} else if (sel && sel->flags & TC_U32_TERMINAL) {
> > > > -		fprintf(f, "terminal flowid ??? ");
> > > > +		print_bool(PRINT_ANY, "terminal_flowid", "terminal flowid ??? ", true);  
> > > 
> > > This looks like another error (ie to stderr) like the earlier case
> > >  
> > 
> > Hi Stephen,
> > Sorry for coming to this so late, but this doesn't look like an error to me.
> > 
> > As far as I can see, TC_U32_TERMINAL is set in this file together with
> > CLASSID or when "action" or "policy" are used. The latter case should be
> > the one that this else branch should catch.
> > 
> > Now, "terminal flowid ???" looks to me like a message printed when we
> > don't actually have a flowid to show, and indeed that is specified when
> > this flag is set (see the comment at line 1169). As such this is
> > probably more a useless log message, than an error one.
> > 
> > If this is the case, we can probably maintain this message on the
> > PRINT_FP output (only to not break script parsing this bit of info out
> > there), and disregard this bit of info on the JSON output.
> > 
> > What do you think?
> > 
> > Regards,
> > Andrea
> > 
> 
> Just always put the same original message on stderr.
> 

Let me phrase my case better: I think the "terminal flowid" message
should not be on stderr, as I don't think this is an error message.

Indeed, "terminal flowid ???" is printed every time we use "action" or
"policy" (see my previous email for details), even when no error is
present and cls_u32 is working ok. In these cases, not having a flowid
is legitimate and not an error.

As this is the case, I think the proper course of action is to have this
message printed out only in non-json output to preserve the same output
of older iproute versions. It would be even better if we decide to
remove this message altogether, as it is not adding any valuable info to
the user.

Re: [PATCH iproute2 v3 1/2] tc: u32: add support for json output

[Stephen Hemminger]

On Mon, 24 Jan 2022 22:30:21 +0100
Andrea Claudi <aclaudi@redhat.com> wrote:

> On Mon, Jan 24, 2022 at 10:50:16AM -0800, Stephen Hemminger wrote:
> > On Mon, 24 Jan 2022 19:25:06 +0100
> > Andrea Claudi <aclaudi@redhat.com> wrote:
> >   
> > > On Thu, Jan 06, 2022 at 02:30:13PM -0800, Stephen Hemminger wrote:  
> > > > On Thu,  6 Jan 2022 13:45:51 -0500
> > > > Wen Liang <liangwen12year@gmail.com> wrote:
> > > >     
> > > > >  	} else if (sel && sel->flags & TC_U32_TERMINAL) {
> > > > > -		fprintf(f, "terminal flowid ??? ");
> > > > > +		print_bool(PRINT_ANY, "terminal_flowid", "terminal flowid ??? ", true);    
> > > > 
> > > > This looks like another error (ie to stderr) like the earlier case
> > > >    
> > > 
> > > Hi Stephen,
> > > Sorry for coming to this so late, but this doesn't look like an error to me.
> > > 
> > > As far as I can see, TC_U32_TERMINAL is set in this file together with
> > > CLASSID or when "action" or "policy" are used. The latter case should be
> > > the one that this else branch should catch.
> > > 
> > > Now, "terminal flowid ???" looks to me like a message printed when we
> > > don't actually have a flowid to show, and indeed that is specified when
> > > this flag is set (see the comment at line 1169). As such this is
> > > probably more a useless log message, than an error one.
> > > 
> > > If this is the case, we can probably maintain this message on the
> > > PRINT_FP output (only to not break script parsing this bit of info out
> > > there), and disregard this bit of info on the JSON output.
> > > 
> > > What do you think?
> > > 
> > > Regards,
> > > Andrea
> > >   
> > 
> > Just always put the same original message on stderr.
> >   
> 
> Let me phrase my case better: I think the "terminal flowid" message
> should not be on stderr, as I don't think this is an error message.
> 
> Indeed, "terminal flowid ???" is printed every time we use "action" or
> "policy" (see my previous email for details), even when no error is
> present and cls_u32 is working ok. In these cases, not having a flowid
> is legitimate and not an error.
> 
> As this is the case, I think the proper course of action is to have this
> message printed out only in non-json output to preserve the same output
> of older iproute versions. It would be even better if we decide to
> remove this message altogether, as it is not adding any valuable info to
> the user.

Agree, I have never used this obscure corner of u32 so will defer to others.
But the existing message is unhelpful and looks like a bug.
The output should be clear and correct for both json and non-json cases;
and any ??? kind of output should be reserved for cases where some bogus
result is being returned by the kernel.  Some version skew, or partial
result of previous operation maybe.

Re: [PATCH iproute2 v3 1/2] tc: u32: add support for json output

[Jamal Hadi Salim]

On 2022-01-24 19:43, Stephen Hemminger wrote:
> On Mon, 24 Jan 2022 22:30:21 +0100
> Andrea Claudi <aclaudi@redhat.com> wrote:
> 
>> On Mon, Jan 24, 2022 at 10:50:16AM -0800, Stephen Hemminger wrote:
>>> On Mon, 24 Jan 2022 19:25:06 +0100
>>> Andrea Claudi <aclaudi@redhat.com> wrote:
>>>    
>>>> On Thu, Jan 06, 2022 at 02:30:13PM -0800, Stephen Hemminger wrote:
>>>>> On Thu,  6 Jan 2022 13:45:51 -0500
>>>>> Wen Liang <liangwen12year@gmail.com> wrote:
>>>>>      
>>>>>>   	} else if (sel && sel->flags & TC_U32_TERMINAL) {
>>>>>> -		fprintf(f, "terminal flowid ??? ");
>>>>>> +		print_bool(PRINT_ANY, "terminal_flowid", "terminal flowid ??? ", true);
>>>>>
>>>>> This looks like another error (ie to stderr) like the earlier case
>>>>>     
>>>>

>>>
>>> Just always put the same original message on stderr.
>>>    
>>
>> Let me phrase my case better: I think the "terminal flowid" message
>> should not be on stderr, as I don't think this is an error message.
>>
>> Indeed, "terminal flowid ???" is printed every time we use "action" or
>> "policy" (see my previous email for details), even when no error is
>> present and cls_u32 is working ok. In these cases, not having a flowid
>> is legitimate and not an error.
>>
>> As this is the case, I think the proper course of action is to have this
>> message printed out only in non-json output to preserve the same output
>> of older iproute versions. It would be even better if we decide to
>> remove this message altogether, as it is not adding any valuable info to
>> the user.
> 
> Agree, I have never used this obscure corner of u32 so will defer to others.

Andrea is correct: it is not an error and not deserving to be in stderr.
And it is _not_ an obscure case just for u32. The classid/flowid is a
classifier concept - in most cases is used to select a queue upon
a filter  match but could also be used to uniquely identify a flow.
Consider it a mini-action which identifies the flow. It is omni-present.
In this case u32 is only reporting it wasnt set. It doesnt affect
the datapath functionality. So u32 is doing the right thing. Flower is
a big culprit in that when you visit the googles hardly any example
shows this field being set and the iproute2 side of flower
code just ignores it.

> But the existing message is unhelpful and looks like a bug.
> The output should be clear and correct for both json and non-json cases;
> and any ??? kind of output should be reserved for cases where some bogus
> result is being returned by the kernel.  Some version skew, or partial
> result of previous operation maybe.

Makes sense in particular if we have formal output format like json.
If this breaks tdc it would be worth to fix tdc (and not be backward
compatible)

So: Since none of the tc maintainers was Cced in this thread, can we
please impose a rule where any changes to tc subdir needs to have tdc
tests run (and hopefully whoever is making the change will be gracious
to contribute an additional testcase)?
Do you need a patch for that in some documentation?

cheers,
jamal

Re: [PATCH iproute2 v3 1/2] tc: u32: add support for json output

[David Ahern]

On 1/26/22 6:52 AM, Jamal Hadi Salim wrote:
> 
> Makes sense in particular if we have formal output format like json.
> If this breaks tdc it would be worth to fix tdc (and not be backward
> compatible)
> 
> So: Since none of the tc maintainers was Cced in this thread, can we
> please impose a rule where any changes to tc subdir needs to have tdc
> tests run (and hopefully whoever is making the change will be gracious
> to contribute an additional testcase)?

I can try to remember to run tdc tests for tc patches. I looked into it
a few days ago and seems straightforward to run tdc.sh. The output of
those tests could be simplified - when all is good you get the one line
summary of the test name with PASS/FAIL with an option to run in verbose
mode to get the details of failures. As it is, the person running the
tests has to wade through a lot of output.

> Do you need a patch for that in some documentation?
> 

How about adding some comments to README.devel?

Re: [PATCH iproute2 v3 1/2] tc: u32: add support for json output

[Jamal Hadi Salim]

On 2022-01-26 10:50, David Ahern wrote:
> On 1/26/22 6:52 AM, Jamal Hadi Salim wrote:
>>
>> Makes sense in particular if we have formal output format like json.
>> If this breaks tdc it would be worth to fix tdc (and not be backward
>> compatible)
>>
>> So: Since none of the tc maintainers was Cced in this thread, can we
>> please impose a rule where any changes to tc subdir needs to have tdc
>> tests run (and hopefully whoever is making the change will be gracious
>> to contribute an additional testcase)?
> 
> I can try to remember to run tdc tests for tc patches. I looked into it
> a few days ago and seems straightforward to run tdc.sh.

Note tdc.sh is meant for the bot. It skips a lot things per Davide's
comment that he was worried the robot will end up spending too many
cycles. Good source at the moment is the README.

> The output of
> those tests could be simplified - when all is good you get the one line
> summary of the test name with PASS/FAIL with an option to run in verbose
> mode to get the details of failures. As it is, the person running the
> tests has to wade through a lot of output.
> 

We are going to put some cycles improving things. Your input is useful.

>> Do you need a patch for that in some documentation?
>>
> 
> How about adding some comments to README.devel?


Sure - but it wont be sufficient IMO.
Best course of action is for the maintainers to remind people to run
tests.

BTW: We found out that Stephen's patches still break the latest -next.

cheers,
jamal

Re: [PATCH iproute2 v3 1/2] tc: u32: add support for json output

[David Ahern]

On 1/31/22 5:54 AM, Jamal Hadi Salim wrote:
>>> Do you need a patch for that in some documentation?
>>>
>>
>> How about adding some comments to README.devel?
> 
> 
> Sure - but it wont be sufficient IMO.
> Best course of action is for the maintainers to remind people to run
> tests.

Above, you said the tests were meant for bots.

> 
> BTW: We found out that Stephen's patches still break the latest -next.
> 

ugh. I committed them after running tdc.sh and not seeing a change in
output. We'll need fixup patches then.

Clearly some work is needed on getting the test suite usable by a wider
audience. I am new to running those tests as well and probably had some
pilot errors running them.

I do wait for ACKs from tc folks, but can't wait forever. Right now
there is the 'skip_hw' and 'skip_sw' patch that Victor sent a Tested-by.
When I apply that v2 patch, I see errors in the tdc.sh output so my
mileage varies from Victor's. There is also the v5 of this set which I
have no applied yet; it could use some acks and tdc testing as well.

Re: [PATCH iproute2 v3 1/2] tc: u32: add support for json output

[Jamal Hadi Salim]

On 2022-01-31 10:50, David Ahern wrote:
> On 1/31/22 5:54 AM, Jamal Hadi Salim wrote:
>>>> Do you need a patch for that in some documentation?
>>>>
>>>
>>> How about adding some comments to README.devel?
>>
>>
>> Sure - but it wont be sufficient IMO.
>> Best course of action is for the maintainers to remind people to run
>> tests.
> 
> Above, you said the tests were meant for bots.
> 

The invocation that tdc.sh makes is targeted for the bots.
It only tests actions and qdiscs.
If you run  ./tdc.py -h you'll see more options.

>>
>> BTW: We found out that Stephen's patches still break the latest -next.
>>
> 
> ugh. I committed them after running tdc.sh and not seeing a change in
> output. We'll need fixup patches then.
> 
> Clearly some work is needed on getting the test suite usable by a wider
> audience. I am new to running those tests as well and probably had some
> pilot errors running them.
> 

One of the issues is sometimes some of the tests take a while to
complete, so some tweaking of the timer may be needed depending on your
setup etc. Probably that is what you may be running into.

> I do wait for ACKs from tc folks, but can't wait forever. Right now
> there is the 'skip_hw' and 'skip_sw' patch that Victor sent a Tested-by.
> When I apply that v2 patch, I see errors in the tdc.sh output so my
> mileage varies from Victor's. There is also the v5 of this set which I
> have no applied yet; it could use some acks and tdc testing as well.

ok. Punting to Victor for testing the v5 - was it posted on the list?
I will Ack Bowen's patch.

cheers,
jamal