netdev - RE: [PATCH net-next v2 3/8] devlink: Prepare code to fill multiple port function attributes

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-next>] [day] [month] [year] [list]

Message-ID: <BY5PR12MB4322D4FA0B0ED9E8537C72B3DC3F0@BY5PR12MB4322.namprd12.prod.outlook.com>
Date:   Fri, 18 Sep 2020 03:35:09 +0000
From:   Parav Pandit <parav@...dia.com>
To:     Jacob Keller <jacob.e.keller@...el.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
CC:     Jiri Pirko <jiri@...dia.com>
Subject: RE: [PATCH net-next v2 3/8] devlink: Prepare code to fill multiple
 port function attributes

Hi Jacob,

> From: Jacob Keller <jacob.e.keller@...el.com>
> Sent: Friday, September 18, 2020 12:29 AM
> 
> 
> On 9/17/2020 10:20 AM, Parav Pandit wrote:
> > Prepare code to fill zero or more port function optional attributes.
> > Subsequent patch makes use of this to fill more port function
> > attributes.
> >
> > Signed-off-by: Parav Pandit <parav@...dia.com>
> > Reviewed-by: Jiri Pirko <jiri@...dia.com>
> > ---
> >  net/core/devlink.c | 53
> > +++++++++++++++++++++++++---------------------
> >  1 file changed, 29 insertions(+), 24 deletions(-)
> >
> > diff --git a/net/core/devlink.c b/net/core/devlink.c index
> > e93730065c57..d152489e48da 100644
> > --- a/net/core/devlink.c
> > +++ b/net/core/devlink.c
> > @@ -570,6 +570,31 @@ static int devlink_nl_port_attrs_put(struct sk_buff
> *msg,
> >  	return 0;
> >  }
> >
> > +static int
> > +devlink_port_function_hw_addr_fill(struct devlink *devlink, const struct
> devlink_ops *ops,
> > +				   struct devlink_port *port, struct sk_buff
> *msg,
> > +				   struct netlink_ext_ack *extack, bool
> *msg_updated) {
> > +	u8 hw_addr[MAX_ADDR_LEN];
> > +	int hw_addr_len;
> > +	int err;
> > +
> > +	if (!ops->port_function_hw_addr_get)
> > +		return 0;
> > +
> > +	err = ops->port_function_hw_addr_get(devlink, port, hw_addr,
> &hw_addr_len, extack);
> > +	if (err) {
> > +		if (err == -EOPNOTSUPP)
> > +			return 0;
> > +		return err;
> > +	}
> > +	err = nla_put(msg, DEVLINK_PORT_FUNCTION_ATTR_HW_ADDR,
> hw_addr_len, hw_addr);
> > +	if (err)
> > +		return err;
> > +	*msg_updated = true;
> > +	return 0;
> > +}
> > +
> >  static int
> >  devlink_nl_port_function_attrs_put(struct sk_buff *msg, struct devlink_port
> *port,
> >  				   struct netlink_ext_ack *extack) @@ -577,36
> +602,16 @@
> > devlink_nl_port_function_attrs_put(struct sk_buff *msg, struct devlink_port
> *por
> >  	struct devlink *devlink = port->devlink;
> >  	const struct devlink_ops *ops;
> >  	struct nlattr *function_attr;
> > -	bool empty_nest = true;
> > -	int err = 0;
> > +	bool msg_updated = false;
> > +	int err;
> >
> >  	function_attr = nla_nest_start_noflag(msg,
> DEVLINK_ATTR_PORT_FUNCTION);
> >  	if (!function_attr)
> >  		return -EMSGSIZE;
> >
> >  	ops = devlink->ops;
> > -	if (ops->port_function_hw_addr_get) {
> > -		int hw_addr_len;
> > -		u8 hw_addr[MAX_ADDR_LEN];
> > -
> > -		err = ops->port_function_hw_addr_get(devlink, port, hw_addr,
> &hw_addr_len, extack);
> > -		if (err == -EOPNOTSUPP) {
> > -			/* Port function attributes are optional for a port. If
> port doesn't
> > -			 * support function attribute, returning -EOPNOTSUPP is
> not an error.
> > -			 */
> 
> We lost this comment in the move it looks like. I think it's still useful to keep for
> clarity of why we're converting -EOPNOTSUPP in the return.
You are right. It is a useful comment.
However, it is already covered in include/net/devlink.h in the standard comment of the callback function.
For new driver implementation, looking there will be more useful.
So I guess its ok to drop from here.

> 
> > -			err = 0;
> > -			goto out;
> > -		} else if (err) {
> > -			goto out;
> > -		}
> > -		err = nla_put(msg,
> DEVLINK_PORT_FUNCTION_ATTR_HW_ADDR, hw_addr_len, hw_addr);
> > -		if (err)
> > -			goto out;
> > -		empty_nest = false;
> > -	}
> > -
> > -out:
> > -	if (err || empty_nest)
> > +	err = devlink_port_function_hw_addr_fill(devlink, ops, port, msg,
> extack, &msg_updated);
> > +	if (err || !msg_updated)
> >  		nla_nest_cancel(msg, function_attr);
> >  	else
> >  		nla_nest_end(msg, function_attr);
> >