netdev - Re: [PATCH iproute2] ematch: support JSON output

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <9883d05e-a18b-4b41-8a0d-8b0038aeceb7@kernel.org>
Date: Fri, 15 Mar 2024 09:12:46 -0600
From: David Ahern <dsahern@...nel.org>
To: Stephen Hemminger <stephen@...workplumber.org>, netdev@...r.kernel.org
Cc: Jamal Hadi Salim <jhs@...atatu.com>
Subject: Re: [PATCH iproute2] ematch: support JSON output

On 3/13/24 6:24 PM, Stephen Hemminger wrote:
> diff --git a/tc/em_canid.c b/tc/em_canid.c
> index 228547529134..815ed8c7bce0 100644
> --- a/tc/em_canid.c
> +++ b/tc/em_canid.c
> @@ -154,24 +154,29 @@ static int canid_print_eopt(FILE *fd, struct tcf_ematch_hdr *hdr, void *data,
>  
>  		if (pcfltr->can_id & CAN_EFF_FLAG) {
>  			if (pcfltr->can_mask == (CAN_EFF_FLAG | CAN_EFF_MASK))
> -				fprintf(fd, "eff 0x%"PRIX32,
> -						pcfltr->can_id & CAN_EFF_MASK);
> -			else
> -				fprintf(fd, "eff 0x%"PRIX32":0x%"PRIX32,
> -						pcfltr->can_id & CAN_EFF_MASK,
> -						pcfltr->can_mask & CAN_EFF_MASK);
> +				print_0xhex(PRINT_ANY, "eff", "eff 0x%"PRIX32,
> +					    pcfltr->can_id & CAN_EFF_MASK);
> +			else {
> +				print_0xhex(PRINT_ANY, "eff", "eff 0x%"PRIX32,
> +					    pcfltr->can_id & CAN_EFF_MASK);
> +				print_0xhex(PRINT_ANY, "mask", ":0x%"PRIX32,
> +					    pcfltr->can_mask & CAN_EFF_MASK);
> +			}

if the else branch has {}, the first one should as well.

>  		} else {
> +			

unneeded extra newline

>  			if (pcfltr->can_mask == (CAN_EFF_FLAG | CAN_SFF_MASK))
> -				fprintf(fd, "sff 0x%"PRIX32,
> -						pcfltr->can_id & CAN_SFF_MASK);
> -			else
> -				fprintf(fd, "sff 0x%"PRIX32":0x%"PRIX32,
> -						pcfltr->can_id & CAN_SFF_MASK,
> -						pcfltr->can_mask & CAN_SFF_MASK);
> +				print_0xhex(PRINT_ANY, "sff", "sff 0x%"PRIX32,
> +					    pcfltr->can_id & CAN_SFF_MASK);
> +			else {
> +				print_0xhex(PRINT_ANY, "sff", "sff 0x%"PRIX32,
> +					    pcfltr->can_id & CAN_SFF_MASK);
> +				print_0xhex(PRINT_ANY, "mask", ":0x%"PRIX32,
> +					    pcfltr->can_mask & CAN_SFF_MASK);
> +			}
>  		}
>  
>  		if ((i + 1) < rules_count)
> -			fprintf(fd, " ");
> +			print_string(PRINT_FP, NULL, " ", NULL);
>  	}
>  
>  	return 0;
> diff --git a/tc/em_cmp.c b/tc/em_cmp.c
> index dfd123df1e10..9e2d14077c6c 100644
> --- a/tc/em_cmp.c
> +++ b/tc/em_cmp.c
> @@ -138,6 +138,8 @@ static int cmp_print_eopt(FILE *fd, struct tcf_ematch_hdr *hdr, void *data,
>  			  int data_len)
>  {
>  	struct tcf_em_cmp *cmp = data;
> +	const char *align = NULL;
> +	const char *op = NULL;
>  
>  	if (data_len < sizeof(*cmp)) {
>  		fprintf(stderr, "CMP header size mismatch\n");
> @@ -145,28 +147,36 @@ static int cmp_print_eopt(FILE *fd, struct tcf_ematch_hdr *hdr, void *data,
>  	}
>  
>  	if (cmp->align == TCF_EM_ALIGN_U8)
> -		fprintf(fd, "u8 ");
> +		align = "u8";
>  	else if (cmp->align == TCF_EM_ALIGN_U16)
> -		fprintf(fd, "u16 ");
> +		align = "u16";
>  	else if (cmp->align == TCF_EM_ALIGN_U32)
> -		fprintf(fd, "u32 ");
> +		align = "u32";
>  
> -	fprintf(fd, "at %d layer %d ", cmp->off, cmp->layer);
> +	print_uint(PRINT_JSON, "align", "%u ", cmp->align);
> +	if (align)
> +		print_string(PRINT_FP, NULL, "%s ", align);
> +
> +	print_uint(PRINT_ANY, "offset", "at %u ", cmp->off);
> +	print_uint(PRINT_ANY, "layer", "layer %u ", cmp->layer);
>  
>  	if (cmp->mask)
> -		fprintf(fd, "mask 0x%x ", cmp->mask);
> +		print_0xhex(PRINT_ANY, "mask", "mask 0x%x ", cmp->mask);
>  
>  	if (cmp->flags & TCF_EM_CMP_TRANS)
> -		fprintf(fd, "trans ");
> +		print_null(PRINT_ANY, "trans", "trans ", NULL);
>  
>  	if (cmp->opnd == TCF_EM_OPND_EQ)
> -		fprintf(fd, "eq ");
> +		op = "eq";
>  	else if (cmp->opnd == TCF_EM_OPND_LT)
> -		fprintf(fd, "lt ");
> +		op = "lt";
>  	else if (cmp->opnd == TCF_EM_OPND_GT)
> -		fprintf(fd, "gt ");
> +		op = "gt";
> +
> +	if (op)
> +		print_string(PRINT_ANY, "opnd", "%s ", op);

seems like a change in output which tends to break the tdc suite. Please
cc Jamal on tc patches.

>  
> -	fprintf(fd, "%d", cmp->val);
> +	print_uint(PRINT_ANY, "val", "%u", cmp->val);
>  
>  	return 0;
>  }


> @@ -436,53 +445,51 @@ static inline int print_value(FILE *fd, int type, struct rtattr *rta)
>  	}
>  
>  	switch (type) {
> -		case TCF_META_TYPE_INT:
> -			if (RTA_PAYLOAD(rta) < sizeof(__u32)) {
> -				fprintf(stderr, "meta int type value TLV " \
> -				    "size mismatch.\n");
> -				return -1;
> -			}
> -			fprintf(fd, "%d", rta_getattr_u32(rta));
> -			break;
> +	case TCF_META_TYPE_INT:
> +		if (RTA_PAYLOAD(rta) < sizeof(__u32)) {
> +			fprintf(stderr,
> +				"meta int type value TLV size mismatch.\n");
> +			return -1;
> +		}
> +		print_uint(PRINT_ANY, "value", "%u", rta_getattr_u32(rta));
> +		break;
>  
> -		case TCF_META_TYPE_VAR:
> -			print_binary(fd, RTA_DATA(rta), RTA_PAYLOAD(rta));
> -			break;
> +	case TCF_META_TYPE_VAR:
> +		print_binary(RTA_DATA(rta), RTA_PAYLOAD(rta));
> +		break;
>  	}

whitespace cleanup should be a separate patch.

in general this is a large patch. It would be better as a series

pw-bot: cr