> -----Original Message-----
> From: Jiri Pirko <jiri@xxxxxxxxxxx>
> Sent: Thursday, July 21, 2022 11:22 PM
> To: Keller, Jacob E <jacob.e.keller@xxxxxxxxx>
> Cc: netdev@xxxxxxxxxxxxxxx; Jonathan Corbet <corbet@xxxxxxx>; Jiri Pirko
> <jiri@xxxxxxxxxx>; David S. Miller <davem@xxxxxxxxxxxxx>; Eric Dumazet
> <edumazet@xxxxxxxxxx>; Jakub Kicinski <kuba@xxxxxxxxxx>; Paolo Abeni
> <pabeni@xxxxxxxxxx>; Nguyen, Anthony L <anthony.l.nguyen@xxxxxxxxx>;
> David Ahern <dsahern@xxxxxxxxxx>; Stephen Hemminger
> <stephen@xxxxxxxxxxxxxxxxxx>; linux-doc@xxxxxxxxxxxxxxx; intel-wired-
> lan@xxxxxxxxxxxxxxxx
> Subject: Re: [iproute2-next v2 3/3] devlink: add dry run attribute support to
> devlink flash
>
> Thu, Jul 21, 2022 at 11:14:51PM CEST, jacob.e.keller@xxxxxxxxx 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 <jacob.e.keller@xxxxxxxxx>
> >---
> >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.
>
Yep, makes sense.
>
> >+ 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.
>
Will fix.
>
> >+}
> >+
> > 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
> >
