netdev - Re: [PATCH net-next v4 01/10] devlink: Add permanent config parameter get/set operations

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: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CA+Jmh7EeJv_kTRpvtx7q-tR8ugA-E7vQB6pBabggrN-ojB=2Xw@mail.gmail.com>
Date:   Fri, 27 Oct 2017 17:26:52 -0400
From:   Steve Lin <steven.lin1@...adcom.com>
To:     Jiri Pirko <jiri@...nulli.us>
Cc:     Linux Netdev List <netdev@...r.kernel.org>,
        Jiri Pirko <jiri@...lanox.com>,
        "David S . Miller" <davem@...emloft.net>,
        Michael Chan <michael.chan@...adcom.com>,
        John Linville <linville@...driver.com>,
        Andy Gospodarek <gospo@...adcom.com>,
        Yuval Mintz <yuvalm@...lanox.com>
Subject: Re: [PATCH net-next v4 01/10] devlink: Add permanent config parameter
 get/set operations

On Fri, Oct 27, 2017 at 5:04 PM, Jiri Pirko <jiri@...nulli.us> wrote:
> Fri, Oct 27, 2017 at 10:54:05PM CEST, steven.lin1@...adcom.com wrote:
>>Add support for permanent config parameter get/set commands. Used
>>for persistent device configuration parameters.
>>
>>Signed-off-by: Steve Lin <steven.lin1@...adcom.com>
>>Acked-by: Andy Gospodarek <gospo@...adcom.com>
>>---
>> include/net/devlink.h        |   7 ++
>> include/uapi/linux/devlink.h |  25 ++++
>> net/core/devlink.c           | 287 +++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 319 insertions(+)
>>
>>diff --git a/include/net/devlink.h b/include/net/devlink.h
>>index b9654e1..c7dd912 100644
>>--- a/include/net/devlink.h
>>+++ b/include/net/devlink.h
>>@@ -270,6 +270,13 @@ struct devlink_ops {
>>       int (*eswitch_inline_mode_set)(struct devlink *devlink, u8 inline_mode);
>>       int (*eswitch_encap_mode_get)(struct devlink *devlink, u8 *p_encap_mode);
>>       int (*eswitch_encap_mode_set)(struct devlink *devlink, u8 encap_mode);
>>+      int (*perm_config_get)(struct devlink *devlink,
>>+                             enum devlink_perm_config_param param, u8 type,
>>+                             union devlink_perm_config_value *value);
>>+      int (*perm_config_set)(struct devlink *devlink,
>>+                             enum devlink_perm_config_param param, u8 type,
>>+                             union devlink_perm_config_value *value,
>>+                             u8 *restart_reqd);
>
> type should be enum and restart_reqd should be bool.

Throughout devlink/netlink code, NLA_* types are not a named enum;
they're defined like this in netlink.h:

enum {
        NLA_UNSPEC,
        NLA_U8,
        NLA_U16,
        NLA_U32,
        .... additional types follow ....
};

and are, I believe referred to as u8 or u16 in the code.  I could
define a new enum specifically for the config get/set types, but that
seems duplicative with the NLA_* types already defined - is that what
you're suggesting?

I will change restart_reqd to be bool.

>
> [...]
>
>
>>+static int devlink_nl_single_param_set(struct sk_buff *msg,
>>+                                     struct devlink *devlink,
>>+                                     u32 param, u8 type,
>>+                                     union devlink_perm_config_value *value)
>>+{
>>+      const struct devlink_ops *ops = devlink->ops;
>>+      struct nlattr *cfgparam_attr;
>>+      u8 need_restart;
>>+      int err;
>>+
>>+      /* Now set parameter */
>>+      err = ops->perm_config_set(devlink, param, type, value, &need_restart);
>>+      if (err)
>>+              return err;
>>+
>>+      cfgparam_attr = nla_nest_start(msg, DEVLINK_ATTR_PERM_CONFIG);
>>+      /* Update restart reqd - if any param needs restart, should be set */
>>+      if (need_restart) {
>>+              err = nla_put_u8(msg,
>>+                               DEVLINK_ATTR_PERM_CONFIG_RESTART_REQUIRED, 1);
>
> Why don't you just put this flag always? Otherwise it could be NLA_FLAG

Ok, I will change to NLA_FLAG.

Thanks,
Steve