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

[Davide Caratti <dcaratti@redhat.com>]

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