netdev - Re: [PATCH net-next 1/9] net: ndo_bridge

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]
Date:   Wed, 12 Dec 2018 14:26:48 +0200
From:   Nikolay Aleksandrov <nikolay@...ulusnetworks.com>
To:     Petr Machata <petrm@...lanox.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Cc:     Jiri Pirko <jiri@...lanox.com>, Ido Schimmel <idosch@...lanox.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "ivecera@...hat.com" <ivecera@...hat.com>
Subject: Re: [PATCH net-next 1/9] net: ndo_bridge_setlink: Add extack

On 12/12/2018 13:52, Petr Machata wrote:
> Drivers may not be able to implement a VLAN addition or reconfiguration.
> In those cases it's desirable to explain to the user that it was
> rejected (and why).
> 
> To that end, add extack argument to ndo_bridge_setlink. Adapt all users
> to that change.
> 
> Following patches will use the new argument in the bridge driver.
> 
> Signed-off-by: Petr Machata <petrm@...lanox.com>
> Acked-by: Jiri Pirko <jiri@...lanox.com>
> ---
>  drivers/net/ethernet/broadcom/bnxt/bnxt.c     | 2 +-
>  drivers/net/ethernet/emulex/benet/be_main.c   | 2 +-
>  drivers/net/ethernet/intel/i40e/i40e_main.c   | 4 +++-
>  drivers/net/ethernet/intel/ice/ice_main.c     | 3 ++-
>  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 3 ++-
>  include/linux/netdevice.h                     | 6 ++++--
>  net/bridge/br_netlink.c                       | 3 ++-
>  net/bridge/br_private.h                       | 3 ++-
>  net/core/rtnetlink.c                          | 6 ++++--
>  9 files changed, 21 insertions(+), 11 deletions(-)
> 
[snip]
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index fc6ba71513be..d8046696d27b 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -1165,7 +1165,7 @@ struct dev_ifalias {
>   *	entries to skb and update idx with the number of entries.
>   *
>   * int (*ndo_bridge_setlink)(struct net_device *dev, struct nlmsghdr *nlh,
> - *			     u16 flags)
> + *			     u16 flags, struct netlink_ext_ack *extack)
>   * int (*ndo_bridge_getlink)(struct sk_buff *skb, u32 pid, u32 seq,
>   *			     struct net_device *dev, u32 filter_mask,
>   *			     int nlflags)
> @@ -1390,7 +1390,9 @@ struct net_device_ops {
>  
>  	int			(*ndo_bridge_setlink)(struct net_device *dev,
>  						      struct nlmsghdr *nlh,
> -						      u16 flags);
> +						      u16 flags,
> +						      struct netlink_ext_ack *
> +						      extack);

IMO it's better to have struct netlink_ext_ack and extack on the same
line even if it goes a little over 80 here, it is normal for netdevice
ops and wouldn't be the first (check below or above). At first I thought
the arg name is missing. :)

>  	int			(*ndo_bridge_getlink)(struct sk_buff *skb,
>  						      u32 pid, u32 seq,
>  						      struct net_device *dev,
> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
> index ff2c10d47529..f9be70b26091 100644
> --- a/net/bridge/br_netlink.c
> +++ b/net/bridge/br_netlink.c
> @@ -850,7 +850,8 @@ static int br_setport(struct net_bridge_port *p, struct nlattr *tb[])
>  }
>  
>  /* Change state and parameters on port. */
> -int br_setlink(struct net_device *dev, struct nlmsghdr *nlh, u16 flags)
> +int br_setlink(struct net_device *dev, struct nlmsghdr *nlh, u16 flags,
> +	       struct netlink_ext_ack *extack)
>  {
>  	struct net_bridge *br = (struct net_bridge *)netdev_priv(dev);
>  	struct nlattr *tb[IFLA_BRPORT_MAX + 1];
> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> index 5719b4d3e466..090dfacdc438 100644
> --- a/net/bridge/br_private.h
> +++ b/net/bridge/br_private.h
> @@ -1138,7 +1138,8 @@ int br_netlink_init(void);
>  void br_netlink_fini(void);
>  void br_ifinfo_notify(int event, const struct net_bridge *br,
>  		      const struct net_bridge_port *port);
> -int br_setlink(struct net_device *dev, struct nlmsghdr *nlmsg, u16 flags);
> +int br_setlink(struct net_device *dev, struct nlmsghdr *nlmsg, u16 flags,
> +	       struct netlink_ext_ack *extack);
>  int br_dellink(struct net_device *dev, struct nlmsghdr *nlmsg, u16 flags);
>  int br_getlink(struct sk_buff *skb, u32 pid, u32 seq, struct net_device *dev,
>  	       u32 filter_mask, int nlflags);
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index c9c0407a7ee0..3b6e551f9e69 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -4332,7 +4332,8 @@ static int rtnl_bridge_setlink(struct sk_buff *skb, struct nlmsghdr *nlh,
>  			goto out;
>  		}
>  
> -		err = br_dev->netdev_ops->ndo_bridge_setlink(dev, nlh, flags);
> +		err = br_dev->netdev_ops->ndo_bridge_setlink(dev, nlh, flags,
> +							     extack);
>  		if (err)
>  			goto out;
>  
> @@ -4344,7 +4345,8 @@ static int rtnl_bridge_setlink(struct sk_buff *skb, struct nlmsghdr *nlh,
>  			err = -EOPNOTSUPP;
>  		else
>  			err = dev->netdev_ops->ndo_bridge_setlink(dev, nlh,
> -								  flags);
> +								  flags,
> +								  extack);
>  		if (!err) {
>  			flags &= ~BRIDGE_FLAGS_SELF;
>  
>