[PATCH ethtool-next] features: add --json support

[PATCH ethtool-next] features: add --json support

[Denys Fedoryshchenko]

Normal text output remain as before, adding json to make ethtool 
more machine friendly.

Signed-off-by: Denys Fedoryshchenko <denys.f@collabora.com>
---
 ethtool.c          |  1 +
 netlink/features.c | 36 +++++++++++++++++++++++++++++-------
 2 files changed, 30 insertions(+), 7 deletions(-)

diff --git a/ethtool.c b/ethtool.c
index 064bc69..c4905f0 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -5729,6 +5729,7 @@ static const struct option args[] = {
 		.opts	= "-k|--show-features|--show-offload",
 		.func	= do_gfeatures,
 		.nlfunc	= nl_gfeatures,
+		.json	= true,
 		.help	= "Get state of protocol offload and other features"
 	},
 	{
diff --git a/netlink/features.c b/netlink/features.c
index 2a0899e..cbe3edc 100644
--- a/netlink/features.c
+++ b/netlink/features.c
@@ -77,13 +77,23 @@ static void dump_feature(const struct feature_results *results,
 	}
 
 	if (!feature_on(results->hw, idx) || feature_on(results->nochange, idx))
-		suffix = " [fixed]";
+		suffix = "[fixed]";
 	else if (feature_on(results->active, idx) !=
 		 feature_on(results->wanted, idx))
 		suffix = feature_on(results->wanted, idx) ?
-			" [requested on]" : " [requested off]";
-	printf("%s%s: %s%s\n", prefix, name,
+			"[requested on]" : "[requested off]";
+
+	if (is_json_context()) {
+		char *name_suffix = malloc(strlen(name)+strlen("-opt")+1);
+
+		sprintf(name_suffix, "%s-opt", name);
+		print_bool(PRINT_JSON, name, NULL, feature_on(results->active, idx));
+		print_string(PRINT_JSON, name_suffix, NULL, suffix);
+		free(name_suffix);
+	} else {
+		printf("%s%s: %s %s\n", prefix, name,
 	       feature_on(results->active, idx) ? "on" : "off", suffix);
+	}
 }
 
 /* this assumes pattern contains no more than one asterisk */
@@ -153,9 +163,14 @@ int dump_features(const struct nlattr *const *tb,
 					feature_on(results.active, j);
 			}
 		}
-		if (n_match != 1)
-			printf("%s: %s\n", off_flag_def[i].long_name,
+		if (n_match != 1) {
+			if (is_json_context()) {
+				print_bool(PRINT_JSON, off_flag_def[i].long_name, NULL, flag_value);
+			} else {
+				printf("%s: %s\n", off_flag_def[i].long_name,
 			       flag_value ? "on" : "off");
+			}
+		}
 		if (n_match == 0)
 			continue;
 		for (j = 0; j < results.count; j++) {
@@ -210,8 +225,10 @@ int features_reply_cb(const struct nlmsghdr *nlhdr, void *data)
 
 	if (silent)
 		putchar('\n');
-	printf("Features for %s:\n", nlctx->devname);
+	open_json_object(NULL);
+	print_string(PRINT_ANY, "ifname", "Features for %s:\n", nlctx->devname);
 	ret = dump_features(tb, feature_names);
+	close_json_object();
 	return (silent || !ret) ? MNL_CB_OK : MNL_CB_ERROR;
 }
 
@@ -232,9 +249,14 @@ int nl_gfeatures(struct cmd_context *ctx)
 	ret = nlsock_prep_get_request(nlsk, ETHTOOL_MSG_FEATURES_GET,
 				      ETHTOOL_A_FEATURES_HEADER,
 				      ETHTOOL_FLAG_COMPACT_BITSETS);
+
 	if (ret < 0)
 		return ret;
-	return nlsock_send_get_request(nlsk, features_reply_cb);
+	new_json_obj(ctx->json);
+	ret = nlsock_send_get_request(nlsk, features_reply_cb);
+
+	delete_json_obj();
+	return ret;
 }
 
 /* FEATURES_SET */
-- 
2.32.0

Re: [PATCH ethtool-next] features: add --json support

[Jakub Kicinski]

On Sun, 16 Jan 2022 13:31:00 +0200 Denys Fedoryshchenko wrote:
> Normal text output remain as before, adding json to make ethtool 
> more machine friendly.

For the review it's useful to see the outputs in the commit message,
please add (you can trim it down to just a handful of features if 
it's long).

> Signed-off-by: Denys Fedoryshchenko <denys.f@collabora.com>

> diff --git a/ethtool.c b/ethtool.c
> index 064bc69..c4905f0 100644
> --- a/ethtool.c
> +++ b/ethtool.c
> @@ -5729,6 +5729,7 @@ static const struct option args[] = {
>  		.opts	= "-k|--show-features|--show-offload",
>  		.func	= do_gfeatures,
>  		.nlfunc	= nl_gfeatures,
> +		.json	= true,
>  		.help	= "Get state of protocol offload and other features"
>  	},
>  	{
> diff --git a/netlink/features.c b/netlink/features.c
> index 2a0899e..cbe3edc 100644
> --- a/netlink/features.c
> +++ b/netlink/features.c
> @@ -77,13 +77,23 @@ static void dump_feature(const struct feature_results *results,
>  	}
>  
>  	if (!feature_on(results->hw, idx) || feature_on(results->nochange, idx))
> -		suffix = " [fixed]";
> +		suffix = "[fixed]";
>  	else if (feature_on(results->active, idx) !=
>  		 feature_on(results->wanted, idx))
>  		suffix = feature_on(results->wanted, idx) ?
> -			" [requested on]" : " [requested off]";
> -	printf("%s%s: %s%s\n", prefix, name,
> +			"[requested on]" : "[requested off]";
> +
> +	if (is_json_context()) {
> +		char *name_suffix = malloc(strlen(name)+strlen("-opt")+1);
> +
> +		sprintf(name_suffix, "%s-opt", name);
> +		print_bool(PRINT_JSON, name, NULL, feature_on(results->active, idx));
> +		print_string(PRINT_JSON, name_suffix, NULL, suffix);

Maybe it's better to make each feature an object with
wanted/active/nochange etc attributes?

> +		free(name_suffix);
> +	} else {
> +		printf("%s%s: %s %s\n", prefix, name,
>  	       feature_on(results->active, idx) ? "on" : "off", suffix);

nit: please re-align the continuation line to start after the '(' on the
line above.

> +	}
>  }
>  
>  /* this assumes pattern contains no more than one asterisk */
> @@ -153,9 +163,14 @@ int dump_features(const struct nlattr *const *tb,
>  					feature_on(results.active, j);
>  			}
>  		}
> -		if (n_match != 1)
> -			printf("%s: %s\n", off_flag_def[i].long_name,
> +		if (n_match != 1) {
> +			if (is_json_context()) {
> +				print_bool(PRINT_JSON, off_flag_def[i].long_name, NULL, flag_value);
> +			} else {
> +				printf("%s: %s\n", off_flag_def[i].long_name,
>  			       flag_value ? "on" : "off");
> +			}
> +		}
>  		if (n_match == 0)
>  			continue;
>  		for (j = 0; j < results.count; j++) {
> @@ -210,8 +225,10 @@ int features_reply_cb(const struct nlmsghdr *nlhdr, void *data)
>  
>  	if (silent)
>  		putchar('\n');
> -	printf("Features for %s:\n", nlctx->devname);
> +	open_json_object(NULL);
> +	print_string(PRINT_ANY, "ifname", "Features for %s:\n", nlctx->devname);
>  	ret = dump_features(tb, feature_names);
> +	close_json_object();
>  	return (silent || !ret) ? MNL_CB_OK : MNL_CB_ERROR;
>  }
>  
> @@ -232,9 +249,14 @@ int nl_gfeatures(struct cmd_context *ctx)
>  	ret = nlsock_prep_get_request(nlsk, ETHTOOL_MSG_FEATURES_GET,
>  				      ETHTOOL_A_FEATURES_HEADER,
>  				      ETHTOOL_FLAG_COMPACT_BITSETS);
> +

nit: this new line looks spurious

>  	if (ret < 0)
>  		return ret;

..perhaps add a new line here

> -	return nlsock_send_get_request(nlsk, features_reply_cb);
> +	new_json_obj(ctx->json);
> +	ret = nlsock_send_get_request(nlsk, features_reply_cb);
> +

.. but not here

> +	delete_json_obj();

.. and add one here?

> +	return ret;
>  }
>  
>  /* FEATURES_SET */

Thanks!!