Re: [PATCH iproute2-next] devlink: add info subcommand

[Jiri Pirko <jiri@resnulli.us>]

Sat, Feb 02, 2019 at 11:06:08PM CET, jakub.kicinski@netronome.com wrote:
>On Sat, 2 Feb 2019 16:59:38 +0100, Jiri Pirko wrote:
>> Sat, Feb 02, 2019 at 01:03:38AM CET, jakub.kicinski@netronome.com wrote:
>> >Add support for reading the device serial number and versions
>> >from the kernel.
>> >
>> >RFCv2:
>> > - make info subcommand of dev.  
>> 
>> Please add some examples of inputs and outputs.
>
>Sorry, yes, will do.
>
>> >Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
>> >---
>> > devlink/devlink.c      | 207 +++++++++++++++++++++++++++++++++++++++++
>> > man/man8/devlink-dev.8 |  36 +++++++
>> > 2 files changed, 243 insertions(+)
>> >
>> >diff --git a/devlink/devlink.c b/devlink/devlink.c
>> >index 3651e90c1159..3ab046ace8f8 100644
>> >--- a/devlink/devlink.c
>> >+++ b/devlink/devlink.c
>> >@@ -199,6 +199,7 @@ static void ifname_map_free(struct ifname_map *ifname_map)
>> > #define DL_OPT_REGION_SNAPSHOT_ID	BIT(22)
>> > #define DL_OPT_REGION_ADDRESS		BIT(23)
>> > #define DL_OPT_REGION_LENGTH		BIT(24)
>> >+#define DL_OPT_VERSIONS_TYPE		BIT(25)  
>> 
>> Why "type"? Confusing.
>
>Right, I think this dates back to day 1 when the whole thing was called
>versions not info.  How about DL_OPT_INFO_VERSION_TYPE?
>
>> > 
>> > struct dl_opts {
>> > 	uint32_t present; /* flags of present items */
>
>> >@@ -943,6 +952,21 @@ static int param_cmode_get(const char *cmodestr,
>> > 	return 0;
>> > }
>> > 
>> >+static int versions_type_get(const char *typestr, int *p_attr)
>> >+{
>> >+	if (strcmp(typestr, "fixed") == 0) {
>> >+		*p_attr = DEVLINK_ATTR_INFO_VERSION_FIXED;
>> >+	} else if (strcmp(typestr, "stored") == 0) {
>> >+		*p_attr = DEVLINK_ATTR_INFO_VERSION_STORED;
>> >+	} else if (strcmp(typestr, "running") == 0) {
>> >+		*p_attr = DEVLINK_ATTR_INFO_VERSION_RUNNING;
>> >+	} else {
>> >+		pr_err("Unknown versions type \"%s\"\n", typestr);
>> >+		return -EINVAL;
>> >+	}
>> >+	return 0;
>> >+}
>> >+
>> > static int dl_argv_parse(struct dl *dl, uint32_t o_required,
>> > 			 uint32_t o_optional)
>> > {
>> >@@ -1178,6 +1202,19 @@ static int dl_argv_parse(struct dl *dl, uint32_t o_required,
>> > 			if (err)
>> > 				return err;
>> > 			o_found |= DL_OPT_REGION_LENGTH;
>> >+		} else if (dl_argv_match(dl, "versions") &&
>> >+			   (o_all & DL_OPT_VERSIONS_TYPE)) {
>> >+			const char *versionstr;
>> >+
>> >+			dl_arg_inc(dl);
>> >+			err = dl_argv_str(dl, &versionstr);
>> >+			if (err)
>> >+				return err;
>> >+			err = versions_type_get(versionstr,
>> >+						&opts->versions_attr);
>> >+			if (err)
>> >+				return err;
>> >+			o_found |= DL_OPT_VERSIONS_TYPE;
>> > 		} else {
>> > 			pr_err("Unknown option \"%s\"\n", dl_argv(dl));
>> > 			return -EINVAL;
>> >@@ -1443,6 +1480,9 @@ static void cmd_dev_help(void)
>> > 	pr_err("       devlink dev param set DEV name PARAMETER value VALUE cmode { permanent | driverinit | runtime }\n");
>> > 	pr_err("       devlink dev param show [DEV name PARAMETER]\n");
>> > 	pr_err("       devlink dev reload DEV\n");
>> >+	pr_err("       devlink dev info [ DEV [ { versions [ VTYPE ] } ] ]\n");
>> >+	pr_err("\n");
>> >+	pr_err("       VTYPE := { fixed | running | stored }\n");  
>> 
>> So you would like to filter according to the version type? Why?
>
>if devlink dev info bus/train versions stored != devlink ... running
>then reboot is needed.  That one of main uses for reporting running vs
>stored in sections, it's nice to be able to just compare the outputs.

I don't know. In the "info", there are more things than versions. Also,
I believe in near future there are going to be even more things there.
Providing a filtering option for one item and not for other seems
incorrect. I think that devlink should not filter in this case. Let it
dump the whole output and let the user take care of it by other tools.
Each tool should do one thing.