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

[PATCH iproute2 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 | 138 ++++++++++++++++++++++++-----------------------------
 1 file changed, 62 insertions(+), 76 deletions(-)

-- 
2.26.3

[PATCH iproute2 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 | 66 ++++++++++++++++++++++++++----------------------------
 1 file changed, 32 insertions(+), 34 deletions(-)

diff --git a/tc/f_u32.c b/tc/f_u32.c
index a5747f67..136fb740 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, "??? ");
+		print_string(PRINT_ANY, NULL, "%s", "??? ");
 	}
 	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_lluint(PRINT_ANY, "rule hit", "(rule hit %llu ", (unsigned long long) pf->rcnt);
+		print_lluint(PRINT_ANY, "success", "success %llu)", (unsigned long long) pf->rhit);
+	}
 
 	if (tb[TCA_U32_MARK]) {
 		struct tc_u32_mark *mark = RTA_DATA(tb[TCA_U32_MARK]);
@@ -1286,8 +1283,9 @@ 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_0xhex(PRINT_ANY, "fwmark_value", "\n  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 +1296,38 @@ 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_lluint(PRINT_ANY, "success", " (success %llu ) ", (unsigned long long) 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_string(PRINT_ANY, NULL, "%s", "\n    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_hex(PRINT_ANY, "hash mask", "\n    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_string(PRINT_ANY, "input dev", "\n  input dev %s\n", rta_getattr_str(idev));
 	}
 
 	if (tb[TCA_U32_ACT])
-- 
2.26.3

[PATCH iproute2 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 | 72 +++++++++++++++++++++++-------------------------------
 1 file changed, 30 insertions(+), 42 deletions(-)

diff --git a/tc/f_u32.c b/tc/f_u32.c
index 136fb740..8558ab6d 100644
--- a/tc/f_u32.c
+++ b/tc/f_u32.c
@@ -828,19 +828,16 @@ static void print_ipv4(FILE *f, const struct tc_u32_key *key)
 	case 0:
 		switch (ntohl(key->mask)) {
 		case 0x0f000000:
-			fprintf(f, "\n  match IP ihl %u",
-				ntohl(key->val) >> 24);
+			print_uint(PRINT_ANY, "match IP ihl", "\n  match IP ihl %u", ntohl(key->val) >> 24);
 			return;
 		case 0x00ff0000:
-			fprintf(f, "\n  match IP dsfield %#x",
-				ntohl(key->val) >> 16);
+			print_0xhex(PRINT_ANY, "match IP dsfield", "\n  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_int(PRINT_ANY, "match IP protocol", "\n  match IP protocol %d", ntohl(key->val) >> 16);
 			return;
 		}
 		break;
@@ -849,11 +846,12 @@ 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_string(PRINT_ANY, "match IP", "\n  %s ", "src");
+				else
+					print_string(PRINT_ANY, "match IP", "\n  %s ", "dst");
+				print_string(PRINT_ANY, "IP Addr", "%s", inet_ntop(AF_INET, &key->val, abuf, sizeof(abuf)));
+				print_int(PRINT_ANY, "IP Addr prefix", "/%d", bits);
 				return;
 			}
 		}
@@ -862,18 +860,14 @@ 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, "match dport", "match dport %u", ntohl(key->val) & 0xffff);
 			return;
 		case 0xffff0000:
-			fprintf(f, "\n  match sport %u",
-				ntohl(key->val) >> 16);
+			print_uint(PRINT_ANY, "match sport", "\n  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_uint(PRINT_ANY, "match dport", "\n  match dport %u, ", ntohl(key->val) & 0xffff);
+			print_uint(PRINT_ANY, "match sport", "match sport %u", ntohl(key->val) >> 16);
 			return;
 		}
 		/* XXX: Default print_raw */
@@ -888,19 +882,16 @@ static void print_ipv6(FILE *f, const struct tc_u32_key *key)
 	case 0:
 		switch (ntohl(key->mask)) {
 		case 0x0f000000:
-			fprintf(f, "\n  match IP ihl %u",
-				ntohl(key->val) >> 24);
+			print_uint(PRINT_ANY, "match IP ihl", "\n  match IP ihl %u", ntohl(key->val) >> 24);
 			return;
 		case 0x00ff0000:
-			fprintf(f, "\n  match IP dsfield %#x",
-				ntohl(key->val) >> 16);
+			print_0xhex(PRINT_ANY, "match IP dsfield", "\n  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_int(PRINT_ANY, "match IP protocol", "\n  match IP protocol %d", ntohl(key->val) >> 16);
 			return;
 		}
 		break;
@@ -909,11 +900,12 @@ 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_string(PRINT_ANY, "match IP", "\n  %s ", "src");
+				else
+					print_string(PRINT_ANY, "match IP", "\n  %s ", "dst");
+				print_string(PRINT_ANY, "IP Addr", "%s", inet_ntop(AF_INET, &key->val, abuf, sizeof(abuf)));
+				print_int(PRINT_ANY, "IP Addr prefix", "/%d", bits);
 				return;
 			}
 		}
@@ -922,17 +914,14 @@ 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_uint(PRINT_ANY, "match sport", "\n  match sport %u", ntohl(key->val) & 0xffff);
 			return;
 		case 0xffff0000:
-			fprintf(f, "\n  match dport %u",
-				ntohl(key->val) >> 16);
+			print_uint(PRINT_ANY, "match 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_uint(PRINT_ANY, "match sport", "\n  match sport %u, ", ntohl(key->val) & 0xffff);
+			print_uint(PRINT_ANY, "match dport", "match dport %u", ntohl(key->val) >> 16);
 
 			return;
 		}
@@ -942,11 +931,10 @@ static void print_ipv6(FILE *f, const struct tc_u32_key *key)
 
 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);
+	print_hex(PRINT_ANY, "match value", "\n  match %08x", (unsigned int)ntohl(key->val));
+	print_hex(PRINT_ANY, "match mask", "/%08x ", (unsigned int)ntohl(key->mask));
+	print_string(PRINT_ANY, "match offmask", "at %s", key->offmask ? "nexthdr+" : "");
+	print_int(PRINT_ANY, "match off", "%d", key->off);
 }
 
 static const struct {
-- 
2.26.3

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

[Davide Caratti]

On Mon, Sep 06, 2021 at 09:57:50PM -0400, Wen Liang wrote:
> 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>


hello Wen,

this series does not break TDC selftests for the u32 classifier (well,
on net-next those tests are temporarily broken because it still misses
[1]. However, the fix should propagate soon - and I verified that the
tests keep passing after applying [1] and your series).

> ---
>  tc/f_u32.c | 66 ++++++++++++++++++++++++++----------------------------
>  1 file changed, 32 insertions(+), 34 deletions(-)
> 
> diff --git a/tc/f_u32.c b/tc/f_u32.c
> index a5747f67..136fb740 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]));

the JSON specification [2] does not forbid spaces in names (if they are
enclosed in double quotes, like it happens in the the iproute2 case),
however I think that "ht_divisor" should sound better than "ht divisor"
in the JSON name.

Look at this example (done on my fedora that uses fq_codel):

 $ tc -j qdisc show | jq '.[1].options.memory limit'
 jq: error: syntax error, unexpected IDENT, expecting $end (Unix shell
 quoting issues?) at <top-level>, line 1:
 .[1].options.memory limit                    
 jq: 1 compile error
 $ tc -j qdisc show | jq '.[1].options.memory_limit'
 33554432
 $ tc -j -j qdisc show | jq '.[1].options."memory limit"'
 
 $ 

Since it's a "memory_limit", and not a "memory limit", I can forget
about quotes and my jq is happy either ways. Do you think it's worth
respinning your patches with those names converted to avoid whitespaces
in names? the same applies for other elements in your series.

(Sorry if this comment might sound nit-picking, but the iproute2 output is
known to be used thoroughly in scripts. So, maybe it's better to do a
robust design)

thanks!
-- 
davide

[1] https://lore.kernel.org/netdev/20210804091828.3783-1-phil@nwl.cc/
[2] https://www.ecma-international.org/wp-content/uploads/ECMA-404_2nd_edition_december_2017.pdf

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

[Stephen Hemminger]

On Mon,  6 Sep 2021 21:57:50 -0400
Wen Liang <liangwen12year@gmail.com> wrote:

>  	} else {
> -		fprintf(f, "??? ");
> +		print_string(PRINT_ANY, NULL, "%s", "??? ");
>  	}

This would be better handled by printing a real error message
on stderr, rather than continuing this confusing message.

+		print_lluint(PRINT_ANY, "rule hit", "(rule hit %llu ", (unsigned long long) pf->rcnt);
+		print_lluint(PRINT_ANY, "success", "success %llu)", (unsigned long long) pf->rhit);
+	}

There is print_u64 which is better than doing these casts.

+				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);

Space is not valid in JSON tag.

Please test by running the output from your changes into a JSON parser.
Example:
     tc -j ... | python3 -m json.tool

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

[Thorsten Glaser]

On Tue, 7 Sep 2021, Stephen Hemminger wrote:

> Space is not valid in JSON tag.

They are valid. Any strings are valid.

bye,
//mirabilos
-- 
Infrastrukturexperte • tarent solutions GmbH
Am Dickobskreuz 10, D-53121 Bonn • http://www.tarent.de/
Telephon +49 228 54881-393 • Fax: +49 228 54881-235
HRB AG Bonn 5168 • USt-ID (VAT): DE122264941
Geschäftsführer: Dr. Stefan Barth, Kai Ebenrett, Boris Esser, Alexander Steeg

                        ****************************************************
/⁀\ The UTF-8 Ribbon
╲ ╱ Campaign against      Mit dem tarent-Newsletter nichts mehr verpassen:
 ╳  HTML eMail! Also,     https://www.tarent.de/newsletter
╱ ╲ header encryption!
                        ****************************************************

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

[Stephen Hemminger]

On Tue, 7 Sep 2021 21:32:41 +0200 (CEST)
Thorsten Glaser <t.glaser@tarent.de> wrote:

> On Tue, 7 Sep 2021, Stephen Hemminger wrote:
> 
> > Space is not valid in JSON tag.  
> 
> They are valid. Any strings are valid.

Your right the library is quoting them, but various style guidelines
do not recommend doing this.

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

[David Ahern]

On 9/7/21 2:55 PM, Stephen Hemminger wrote:
> On Tue, 7 Sep 2021 21:32:41 +0200 (CEST)
> Thorsten Glaser <t.glaser@tarent.de> wrote:
> 
>> On Tue, 7 Sep 2021, Stephen Hemminger wrote:
>>
>>> Space is not valid in JSON tag.  
>>
>> They are valid. Any strings are valid.
> 
> Your right the library is quoting them, but various style guidelines
> do not recommend doing this.
> 

And Davide pointed out a good example of why.