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

[Arnd Bergmann <arnd@arndb.de>]

On Friday 14 May 2010, Patrick McHardy wrote:
> 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?

Yes, the request field of the VDP message shows the status (e.g. associated or
disassociated).

> > +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_*], ...
>   ...

Ah, I was wondering about this already. Does this mean that IFLA_VFINFO
does this incorrectly as well?

> >  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?

No, but if parent is NULL, we must not call dev_num_vf(). The way that enic
needs the attributes, they can be either for the VF of dev->dev.parent (the
PCI PF), or for the PF itself, even if it does not have VFs, in which case
it would be interesting to have IFLA_NUM_VF = 0 in the output.

Maybe a better structure would be to separate the two cases, also allowing
a port profile to be associated with both the PF and with each of its VFs?

Something like this:

[IFLA_NUM_VF]
[IFLA_VF_PORTS]
  [IFLA_VF_PORT]
    [IFLA_VF_PORT_*], ...
  [IFLA_VF_PORT]
    [IFLA_VF_PORT_*], ...
[IFLA_PORT_SELF]
  [IFLA_VF_PORT_*], ...

	Arnd