netdev - Re: [PATCH v2 iproute2 2/6] rdma: use standard flag for json

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <874jft6v4b.fsf@nvidia.com>
Date: Thu, 04 Jan 2024 13:07:38 +0100
From: Petr Machata <me@...chata.org>
To: Stephen Hemminger <stephen@...workplumber.org>
Cc: leon@...nel.org, netdev@...r.kernel.org
Subject: Re: [PATCH v2 iproute2 2/6] rdma: use standard flag for json

Stephen Hemminger <stephen@...workplumber.org> writes:

> The other iproute2 utils use variable json as flag.

Some do, some don't. dcb, devlink, rdma do not, the rest do.

Personally I'm not a fan of global state, and consider this patch to be
a step back in terms of best practices. I do realize this is how it's
done in iproute2. To the point that utils.h actually carries external
declarations for these variables. But to me those are inevitable warts,
not something to embrace and extend.

Objectively... whatever. There's only one instance of the context
structure anyway. It's just the overtones of a quick hack that irk me.

Anyway, the mechanical parts of the conversion look OK. But:

> diff --git a/rdma/stat.c b/rdma/stat.c
> index 28b1ad857219..6a3f8ca44892 100644
> --- a/rdma/stat.c
> +++ b/rdma/stat.c
> @@ -208,7 +208,7 @@ int res_get_hwcounters(struct rd *rd, struct nlattr *hwc_table, bool print)
>  
>  		nm = mnl_attr_get_str(hw_line[RDMA_NLDEV_ATTR_STAT_HWCOUNTER_ENTRY_NAME]);
>  		v = mnl_attr_get_u64(hw_line[RDMA_NLDEV_ATTR_STAT_HWCOUNTER_ENTRY_VALUE]);
> -		if (rd->pretty_output && !rd->json_output)
> +		if (rd->pretty_output)
>  			newline_indent(rd);

newline_indent() issues close_json_object(). Previously it wouldn't be
invoked in JSON mode, now it will be. Doesn't this break JSON output?
Same question for the hunk below.

>  		res_print_u64(rd, nm, v, hw_line[RDMA_NLDEV_ATTR_STAT_HWCOUNTER_ENTRY_NAME]);
>  	}
> @@ -802,7 +802,7 @@ static int do_stat_mode_parse_cb(const struct nlmsghdr *nlh, void *data,
>  			} else {
>  				print_string(PRINT_FP, NULL, ",", NULL);
>  			}
> -			if (rd->pretty_output && !rd->json_output)
> +			if (rd->pretty_output)
>  				newline_indent(rd);
>  
>  			print_string(PRINT_ANY, NULL, "%s", name);