netdev - Re: [PATCH ethtool-next 2/5] pause: add --json support

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] [day] [month] [year] [list]

Message-ID: <110b6abf-8317-0a60-56e7-a8d9473d04e6@intel.com>
Date:   Thu, 24 Sep 2020 13:41:18 -0700
From:   Jacob Keller <jacob.e.keller@...el.com>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     mkubecek@...e.cz, netdev@...r.kernel.org
Subject: Re: [PATCH ethtool-next 2/5] pause: add --json support



On 9/24/2020 8:36 AM, Jakub Kicinski wrote:
> On Wed, 23 Sep 2020 17:10:30 -0700 Jacob Keller wrote:
>>> -	printf("RX negotiated: %s\nTX negotiated: %s\n",
>>> -	       rx_status ? "on" : "off", tx_status ? "on" : "off");
>>> +
>>> +	if (is_json_context()) {
>>> +		open_json_object("negotiated");
>>> +		print_bool(PRINT_JSON, "rx", NULL, rx_status);
>>> +		print_bool(PRINT_JSON, "tx", NULL, tx_status);
>>> +		close_json_object();
>>> +	} else {
>>> +		printf("RX negotiated: %s\nTX negotiated: %s\n",
>>> +		       rx_status ? "on" : "off", tx_status ? "on" : "off");
>>> +	}
>>
>> Why not use print_string here like show_bool did?
> 
> Yeah.. I did not come up with a good way of reusing the show_bool code
> so I gave up. Taking another swing at it - how does this look?
> 
> diff --git a/netlink/coalesce.c b/netlink/coalesce.c
> index 65f75cf9a8dd..07a92d04b7a1 100644
> --- a/netlink/coalesce.c
> +++ b/netlink/coalesce.c
> @@ -36,9 +36,9 @@ int coalesce_reply_cb(const struct nlmsghdr *nlhdr, void *data)
>  	if (silent)
>  		putchar('\n');
>  	printf("Coalesce parameters for %s:\n", nlctx->devname);
> -	printf("Adaptive RX: %s  TX: %s\n",
> -	       u8_to_bool(tb[ETHTOOL_A_COALESCE_USE_ADAPTIVE_RX]),
> -	       u8_to_bool(tb[ETHTOOL_A_COALESCE_USE_ADAPTIVE_TX]));
> +	show_bool("rx", "Adaptive RX: %s  ",
> +		  tb[ETHTOOL_A_COALESCE_USE_ADAPTIVE_RX]);
> +	show_bool("tx", "TX: %s\n", tb[ETHTOOL_A_COALESCE_USE_ADAPTIVE_TX]);
>  	show_u32(tb[ETHTOOL_A_COALESCE_STATS_BLOCK_USECS],
>  		 "stats-block-usecs: ");
>  	show_u32(tb[ETHTOOL_A_COALESCE_RATE_SAMPLE_INTERVAL],
> diff --git a/netlink/netlink.h b/netlink/netlink.h
> index 4916d25ed5c0..1012e8e32cd8 100644
> --- a/netlink/netlink.h
> +++ b/netlink/netlink.h
> @@ -98,27 +98,30 @@ static inline void show_u32(const struct nlattr *attr, const char *label)
>  		printf("%sn/a\n", label);
>  }
>  
> -static inline const char *u8_to_bool(const struct nlattr *attr)
> +static inline const char *u8_to_bool(const uint8_t *val)
>  {
> -	if (attr)
> -		return mnl_attr_get_u8(attr) ? "on" : "off";
> +	if (val)
> +		return *val ? "on" : "off";
>  	else
>  		return "n/a";
>  }
>  
> -static inline void show_bool(const char *key, const char *fmt,
> -			     const struct nlattr *attr)
> +static inline void show_bool_val(const char *key, const char *fmt, uint8_t *val)
>  {
>  	if (is_json_context()) {
> -		if (attr) {
> -			print_bool(PRINT_JSON, key, NULL,
> -				   mnl_attr_get_u8(attr));
> -		}
> +		if (val)
> +			print_bool(PRINT_JSON, key, NULL, val);
>  	} else {
> -		print_string(PRINT_FP, NULL, fmt, u8_to_bool(attr));
> +		print_string(PRINT_FP, NULL, fmt, u8_to_bool(val));
>  	}
>  }
>  
> +static inline void show_bool(const char *key, const char *fmt,
> +			     const struct nlattr *attr)
> +{
> +	show_bool_val(key, fmt, attr ? mnl_attr_get_payload(attr) : NULL);
> +}
> +
>  /* misc */
>  
>  static inline void copy_devname(char *dst, const char *src)
> diff --git a/netlink/pause.c b/netlink/pause.c
> index f9dec9fe887a..5395398ba948 100644
> --- a/netlink/pause.c
> +++ b/netlink/pause.c
> @@ -41,8 +41,8 @@ static int pause_autoneg_cb(const struct nlmsghdr *nlhdr, void *data)
>  	struct pause_autoneg_status ours = {};
>  	struct pause_autoneg_status peer = {};
>  	struct nl_context *nlctx = data;
> -	bool rx_status = false;
> -	bool tx_status = false;
> +	uint8_t rx_status = false;
> +	uint8_t tx_status = false;
>  	bool silent;
>  	int err_ret;
>  	int ret;
> @@ -74,15 +74,10 @@ static int pause_autoneg_cb(const struct nlmsghdr *nlhdr, void *data)
>  			tx_status = true;
>  	}
>  
> -	if (is_json_context()) {
> -		open_json_object("negotiated");
> -		print_bool(PRINT_JSON, "rx", NULL, rx_status);
> -		print_bool(PRINT_JSON, "tx", NULL, tx_status);
> -		close_json_object();
> -	} else {
> -		printf("RX negotiated: %s\nTX negotiated: %s\n",
> -		       rx_status ? "on" : "off", tx_status ? "on" : "off");
> -	}
> +	open_json_object("negotiated");
> +	show_bool_val("rx", "RX negotiated: %s\n", &rx_status);
> +	show_bool_val("tx", "TX negotiated: %s\n", &tx_status);
> +	close_json_object();
>  
>  	return MNL_CB_OK;
>  }
> 

This looks good!

Thanks,
Jake