Re: [net-next-2.6 V7 PATCH 1/2] Add netlink support for virtual port management (was iovnl)

[Patrick McHardy <kaber@trash.net>]

Scott Feldman wrote:
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -653,6 +653,26 @@ static inline int rtnl_vfinfo_size(const struct net_device *dev)
>  		return 0;
>  }
>  
> +static size_t rtnl_vf_port_size(const struct net_device *dev)
> +{
> +	size_t vf_port_size = nla_total_size(sizeof(struct nlattr))
> +						     /* VF_PORT_VF */
> +		+ nla_total_size(VF_PORT_PROFILE_MAX)/* VF_PORT_PROFILE */
> +		+ nla_total_size(sizeof(struct ifla_vf_port_vsi))
> +						     /* VF_PORT_VSI_TYPE */
> +		+ nla_total_size(VF_PORT_UUID_MAX)   /* VF_PORT_VSI_INSTANCE */
> +		+ nla_total_size(VF_PORT_UUID_MAX)   /* VF_PORT_HOST_UUID */
> +		+ nla_total_size(1)		     /* VF_PROT_VDP_REQUEST */

Do messages generated by the kernel really contain a request?

> +		+ nla_total_size(2);		     /* VF_PORT_VDP_RESPONSE */
> +
> +	if (!dev->netdev_ops->ndo_get_vf_port || !dev->dev.parent)
> +		return 0;
> +	if (dev_num_vf(dev->dev.parent))
> +		return vf_port_size * dev_num_vf(dev->dev.parent);
> +	else
> +		return vf_port_size;
> +}
> +


> +static int rtnl_vf_port_fill_nest(struct sk_buff *skb, struct net_device *dev,
> +				  int vf)
> +{
> +	struct nlattr *data;
> +	int err;
> +
> +	data = nla_nest_start(skb, IFLA_VF_PORT);

We usually use a top-level attribute to encapsulate lists of identical
attributes. The other iflink attributes may only occur once and are
usually parsed using nla_parse_nested(), which will parse all
IFLA_VF_PORT attributes, but only return the last one.

Something like:

iflink message:
...
[IFLA_VF_PORTS]
  [IFLA_VF_PORT]
    [IFLA_VF_PORT_*], ...
  [IFLA_VF_PORT]
    [IFLA_VF_PORT_*], ...
  ...


> +	if (!data)
> +		return -EMSGSIZE;
> +
> +	if (vf != VF_PORT_VF_NOT_USED)
> +		nla_put_u32(skb, IFLA_VF_PORT_VF, vf);

This should be checking for errors or use NLA_PUT_U32.

> +
> +	err = dev->netdev_ops->ndo_get_vf_port(dev, vf, skb);
> +	if (err) {
> +		nla_nest_cancel(skb, data);
> +		return err;
> +	}
> +
> +	nla_nest_end(skb, data);
> +
> +	return 0;
> +}
> +

>  static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev,
>  			    int type, u32 pid, u32 seq, u32 change,
>  			    unsigned int flags)
> @@ -747,17 +819,23 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev,
>  		goto nla_put_failure;
>  	copy_rtnl_link_stats64(nla_data(attr), stats);
>  
> +	if (dev->dev.parent)
> +		NLA_PUT_U32(skb, IFLA_NUM_VF, dev_num_vf(dev->dev.parent));

Just wondering, is the only case where dev.parent is non-NULL
really when virtual ports are present?

> +
>  	if (dev->netdev_ops->ndo_get_vf_config && dev->dev.parent) {
>  		int i;
>  		struct ifla_vf_info ivi;
>  
> -		NLA_PUT_U32(skb, IFLA_NUM_VF, dev_num_vf(dev->dev.parent));
>  		for (i = 0; i < dev_num_vf(dev->dev.parent); i++) {
>  			if (dev->netdev_ops->ndo_get_vf_config(dev, i, &ivi))
>  				break;
>  			NLA_PUT(skb, IFLA_VFINFO, sizeof(ivi), &ivi);
>  		}
>  	}
> +
> +	if (rtnl_vf_port_fill(skb, dev))
> +		goto nla_put_failure;
> +
>  	if (dev->rtnl_link_ops) {
>  		if (rtnl_link_fill(skb, dev) < 0)
>  			goto nla_put_failure;
> @@ -824,6 +902,7 @@ const struct nla_policy ifla_policy[IFLA_MAX+1] = {
>  				    .len = sizeof(struct ifla_vf_vlan) },
>  	[IFLA_VF_TX_RATE]	= { .type = NLA_BINARY,
>  				    .len = sizeof(struct ifla_vf_tx_rate) },
> +	[IFLA_VF_PORT]		= { .type = NLA_NESTED },
>  };
>  EXPORT_SYMBOL(ifla_policy);
>  
> @@ -832,6 +911,20 @@ static const struct nla_policy ifla_info_policy[IFLA_INFO_MAX+1] = {
>  	[IFLA_INFO_DATA]	= { .type = NLA_NESTED },
>  };
>  
> +static const struct nla_policy ifla_vf_port_policy[IFLA_VF_PORT_MAX+1] = {
> +	[IFLA_VF_PORT_VF]	    = { .type = NLA_U32 },
> +	[IFLA_VF_PORT_PROFILE]	    = { .type = NLA_STRING,
> +					.len = VF_PORT_PROFILE_MAX },
> +	[IFLA_VF_PORT_VSI_TYPE]     = { .type = NLA_BINARY,
> +					.len = sizeof(struct ifla_vf_port_vsi)},
> +	[IFLA_VF_PORT_INSTANCE_UUID]= { .type = NLA_BINARY,
> +					.len = VF_PORT_UUID_MAX },
> +	[IFLA_VF_PORT_HOST_UUID]    = { .type = NLA_STRING,
> +					.len = VF_PORT_UUID_MAX },
> +	[IFLA_VF_PORT_REQUEST]	    = { .type = NLA_U8, },
> +	[IFLA_VF_PORT_RESPONSE]	    = { .type = NLA_U16, },
> +};
> +
>  struct net *rtnl_link_get_net(struct net *src_net, struct nlattr *tb[])
>  {
>  	struct net *net;
> @@ -1028,6 +1121,27 @@ static int do_setlink(struct net_device *dev, struct ifinfomsg *ifm,
>  	}
>  	err = 0;
>  
> +	if (tb[IFLA_VF_PORT]) {
> +		struct nlattr *vf_port[IFLA_VF_PORT_MAX+1];
> +		int vf = VF_PORT_VF_NOT_USED;
> +
> +		err = nla_parse_nested(vf_port, IFLA_VF_PORT_MAX,
> +			tb[IFLA_VF_PORT], ifla_vf_port_policy);
> +		if (err < 0)
> +			goto errout;
> +
> +		if (vf_port[IFLA_VF_PORT_VF])
> +			vf = nla_get_u32(vf_port[IFLA_VF_PORT_VF]);
> +
> +		err = -EOPNOTSUPP;
> +		if (ops->ndo_set_vf_port)
> +			err = ops->ndo_set_vf_port(dev, vf, vf_port);
> +		if (err < 0)
> +			goto errout;
> +		modified = 1;
> +	}
> +	err = 0;
> +
>  errout:
>  	if (err < 0 && modified && net_ratelimit())
>  		printk(KERN_WARNING "A link change request failed with "
>