Subject: Re: [iproute2-next v2 3/3] devlink: add dry run attribute support to devlink flash

From: Jiri Pirko <[email-masked]>
Date: Fri, 22 Jul 2022 08:22:00 +0200

Thu, Jul 21, 2022 at 11:14:51PM CEST, [email-masked] wrote:
>Recent versions of the kernel support the DEVLINK_ATTR_DRY_RUN attribute
>which allows requesting a dry run of a command. A dry run is simply
>a request to validate that a command would work, without performing any
>destructive changes.
>
>The attribute is supported by the devlink flash update as a way to
>validate an update, including potentially the binary image, without
>modifying the device.
>
>Add a "dry_run" option to the command line parsing which will enable
>this attribute when requested.
>
>To avoid potential issues, only allow the attribute to be added to
>commands when the kernel recognizes it. This is important because some
>commands do not perform strict validation. If we were to add the
>attribute without this check, an old kernel may silently accept the
>command and perform an update even when dry_run was requested.
>
>Before adding the attribute, check the maximum attribute from the
>CTRL_CMD_GETFAMILY and make sure that the kernel recognizes the
>DEVLINK_ATTR_DRY_RUN attribute.
>
>Signed-off-by: Jacob Keller <[email-masked]>
>---
>Changes since v1
>* Make dl_kernel_supports_dry_run more generic by passing attribute
>
> devlink/devlink.c | 45 +++++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 43 insertions(+), 2 deletions(-)
>
>diff --git a/devlink/devlink.c b/devlink/devlink.c
>index 1e2cfc3d4285..24f1a70a9656 100644
>--- a/devlink/devlink.c
>+++ b/devlink/devlink.c
>@@ -296,6 +296,7 @@ static void ifname_map_free(struct ifname_map *ifname_map)
> #define DL_OPT_PORT_FN_RATE_PARENT	BIT(51)
> #define DL_OPT_LINECARD		BIT(52)
> #define DL_OPT_LINECARD_TYPE	BIT(53)
>+#define DL_OPT_DRY_RUN			BIT(54)
> 
> struct dl_opts {
> 	uint64_t present; /* flags of present items */
>@@ -372,6 +373,8 @@ struct dl {
> 	bool verbose;
> 	bool stats;
> 	bool hex;
>+	bool max_attr_valid;
>+	uint32_t max_attr;
> 	struct {
> 		bool present;
> 		char *bus_name;
>@@ -701,6 +704,7 @@ static const enum mnl_attr_data_type devlink_policy[DEVLINK_ATTR_MAX + 1] = {
> 	[DEVLINK_ATTR_LINECARD_STATE] = MNL_TYPE_U8,
> 	[DEVLINK_ATTR_LINECARD_TYPE] = MNL_TYPE_STRING,
> 	[DEVLINK_ATTR_LINECARD_SUPPORTED_TYPES] = MNL_TYPE_NESTED,
>+	[DEVLINK_ATTR_DRY_RUN] = MNL_TYPE_FLAG,
> };
> 
> static const enum mnl_attr_data_type
>@@ -1522,6 +1526,30 @@ static int dl_args_finding_required_validate(uint64_t o_required,
> 	return 0;
> }
> 
>+static void dl_get_max_attr(struct dl *dl)
>+{
>+	if (!dl->max_attr_valid) {

if (dl->max_attr_valid)
	return;

and then you can drop the indent.

>+		uint32_t max_attr;
>+		int err;
>+
>+		err = mnlg_socket_get_max_attr(&dl->nlg, &max_attr);
>+		if (err) {
>+			pr_err("Unable to determine maximum supported devlink attribute\n");
>+			return;
>+		}
>+
>+		dl->max_attr = max_attr;
>+		dl->max_attr_valid = true;
>+	}
>+}
>+
>+static bool dl_kernel_supports_attr(struct dl *dl, enum devlink_attr attr)
>+{
>+	dl_get_max_attr(dl);
>+
>+	return (dl->max_attr_valid && dl->max_attr >= attr);

Return is not a function. Drop the "()" here.

>+}
>+
> static int dl_argv_parse(struct dl *dl, uint64_t o_required,
> 			 uint64_t o_optional)
> {
>@@ -2037,6 +2065,16 @@ static int dl_argv_parse(struct dl *dl, uint64_t o_required,
> 			dl_arg_inc(dl);
> 			opts->linecard_type = "";
> 			o_found |= DL_OPT_LINECARD_TYPE;
>+		} else if (dl_argv_match(dl, "dry_run") &&
>+			   (o_all & DL_OPT_DRY_RUN)) {
>+
>+			if (!dl_kernel_supports_attr(dl, DEVLINK_ATTR_DRY_RUN)) {
>+				pr_err("Kernel does not support dry_run attribute\n");
>+				return -EOPNOTSUPP;
>+			}
>+
>+			dl_arg_inc(dl);
>+			o_found |= DL_OPT_DRY_RUN;
> 		} else {
> 			pr_err("Unknown option \"%s\"\n", dl_argv(dl));
> 			return -EINVAL;
>@@ -2115,6 +2153,8 @@ static void dl_opts_put(struct nlmsghdr *nlh, struct dl *dl)
> 		mnl_attr_put_strz(nlh, DEVLINK_ATTR_RATE_NODE_NAME,
> 				  opts->rate_node_name);
> 	}
>+	if (opts->present & DL_OPT_DRY_RUN)
>+		mnl_attr_put(nlh, DEVLINK_ATTR_DRY_RUN, 0, NULL);
> 	if (opts->present & DL_OPT_PORT_TYPE)
> 		mnl_attr_put_u16(nlh, DEVLINK_ATTR_PORT_TYPE,
> 				 opts->port_type);
>@@ -2326,7 +2366,7 @@ static void cmd_dev_help(void)
> 	pr_err("       devlink dev reload DEV [ netns { PID | NAME | ID } ]\n");
> 	pr_err("                              [ action { driver_reinit | fw_activate } ] [ limit no_reset ]\n");
> 	pr_err("       devlink dev info [ DEV ]\n");
>-	pr_err("       devlink dev flash DEV file PATH [ component NAME ] [ overwrite SECTION ]\n");
>+	pr_err("       devlink dev flash DEV file PATH [ component NAME ] [ overwrite SECTION ] [ dry_run ]\n");
> }
> 
> static bool cmp_arr_last_handle(struct dl *dl, const char *bus_name,
>@@ -3886,7 +3926,8 @@ static int cmd_dev_flash(struct dl *dl)
> 			       NLM_F_REQUEST | NLM_F_ACK);
> 
> 	err = dl_argv_parse_put(nlh, dl, DL_OPT_HANDLE | DL_OPT_FLASH_FILE_NAME,
>-				DL_OPT_FLASH_COMPONENT | DL_OPT_FLASH_OVERWRITE);
>+				DL_OPT_FLASH_COMPONENT | DL_OPT_FLASH_OVERWRITE |
>+				DL_OPT_DRY_RUN);
> 	if (err)
> 		return err;
> 
>-- 
>2.36.1
>