Re: [net-next-2.6 PATCH v3 4/5] rtnetlink: Add VF config code to rtnetlink

[Patrick McHardy <kaber@trash.net>]

Jeff Kirsher wrote:
> @@ -665,6 +677,17 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev,
>  	stats = dev_get_stats(dev);
>  	copy_rtnl_link_stats(nla_data(attr), stats);
>  
> +	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);

We usually encapsulate lists of the same attribute type in another
top-level attribute. Check out the IFLA_VLAN_*_QOS attributes for
an example.

The interface should also be symetrical, IOW you should dump the
same attributes used in the userspace->kernel direction instead
of a combined "info" attribute.

> +		}
> +	}

> @@ -898,6 +927,44 @@ static int do_setlink(struct net_device *dev, struct ifinfomsg *ifm,
>  		write_unlock_bh(&dev_base_lock);
>  	}
>  
> +	if (tb[IFLA_VF_MAC]) {
> +		struct ifla_vf_mac *ivm;
> +		ivm = nla_data(tb[IFLA_VF_MAC]);
> +		write_lock_bh(&dev_base_lock);

It dev_base_lock really correct here? This is running under the RTNL, so
changes to the device list can't happen.

> +		if (ops->ndo_set_vf_mac)
> +			err = ops->ndo_set_vf_mac(dev, ivm->vf, ivm->mac);

Shouldn't this indicate an error if the attributes aren't supported?

> +		write_unlock_bh(&dev_base_lock);
> +		if (err < 0)
> +			goto errout;
> +		modified = 1;
> +	}
> +
> +	if (tb[IFLA_VF_VLAN]) {
> +		struct ifla_vf_vlan *ivv;
> +		ivv = nla_data(tb[IFLA_VF_VLAN]);
> +		write_lock_bh(&dev_base_lock);
> +		if (ops->ndo_set_vf_vlan)
> +			err = ops->ndo_set_vf_vlan(dev, ivv->vf,
> +						   (u16)ivv->vlan,
> +						   (u8)ivv->qos);

The casts aren't necessary. But why does struct ifla_vf_vlan use u32
for the vlan in the first place?

> +		write_unlock_bh(&dev_base_lock);
> +		if (err < 0)
> +			goto errout;
> +		modified = 1;
> +	}
> +	err = 0;
> +
> +	if (tb[IFLA_VF_TX_RATE]) {
> +		struct ifla_vf_tx_rate *ivt;
> +		ivt = nla_data(tb[IFLA_VF_TX_RATE]);
> +		write_lock_bh(&dev_base_lock);
> +		if (ops->ndo_set_vf_tx_rate)
> +			err = ops->ndo_set_vf_tx_rate(dev, ivt->vf, ivt->rate);
> +		write_unlock_bh(&dev_base_lock);
> +		if (err < 0)
> +			goto errout;
> +		modified = 1;
> +	}
>  	err = 0;
>  
>  errout:
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>