diff mbox series

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

Message ID 5c4108ba5d3c30a0366ad79b49e1097bd9cc96e1.1630978600.git.liangwen12year@gmail.com
State New
Headers show
Series [iproute2,1/2] tc: u32: add support for json output | expand

Commit Message

Wen Liang Sept. 7, 2021, 1:57 a.m. UTC 
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(-)

Comments

Davide Caratti Sept. 7, 2021, 3:46 p.m. UTC | #1 
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!
Stephen Hemminger Sept. 7, 2021, 7:29 p.m. UTC | #2 
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
Thorsten Glaser Sept. 7, 2021, 7:32 p.m. UTC | #3 
On Tue, 7 Sep 2021, Stephen Hemminger wrote:

> Space is not valid in JSON tag.

They are valid. Any strings are valid.

bye,
//mirabilos
Stephen Hemminger Sept. 7, 2021, 8:55 p.m. UTC | #4 
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.
David Ahern Sept. 8, 2021, 1:18 a.m. UTC | #5 
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.
diff mbox series

Patch

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])