[PATCH 2/3] Handle family-based parsing of IFLA_AF_SPEC attribute

[Tobias Jungel]

On Mi, 2015-11-25 at 09:43 -0700, David Ahern wrote:
> On 11/25/15 8:47 AM, Tobias Jungel wrote:
> > > diff --git a/lib/route/link.c b/lib/route/link.c
> > > index 3cb62df95d6a..b8e333c31a3a 100644
> > > --- a/lib/route/link.c
> > > +++ b/lib/route/link.c
> > > @@ -489,6 +489,56 @@ int rtnl_link_info_parse(struct rtnl_link
> > > *link,
> > > struct nlattr **tb)
> > >   	return 0;
> > >   }
> > > 
> > > +static int parse_af_spec_unspec(struct rtnl_link *link, struct
> > > nlattr *attr)
> > > +{
> > > +	struct nlattr *af_attr;
> > > +	int remaining;
> > > +	int err = 0;
> > > +
> > > +	nla_for_each_nested(af_attr, attr, remaining) {
> > > +		struct rtnl_link_af_ops *af_ops;
> > > +
> > > +		af_ops = af_lookup_and_alloc(link,
> > > nla_type(af_attr));
> > > +		if (af_ops && af_ops->ao_parse_af) {
> > > +			char *af_data = link-
> > > > l_af_data[nla_type(af_attr)];
> > > +
> > > +			err = af_ops->ao_parse_af(link, af_attr,
> > > af_data);
> > > +			if (err < 0) {
> > > +				goto errout;
> > > +			}
> > > +		}
> > > +
> > > +	}
> > > +
> > > +	link->ce_mask |= LINK_ATTR_AF_SPEC;
> > > +errout:
> > 
> > since af_lookup_and_alloc is called, I guess a rtnl_link_af_ops_put
> > is
> > missing here.
> 
> This function will return an error, the caller sees that and jumps to
> the current errout which does the rtnl_link_af_ops_put.
> 
> > 
> > > +	return err;
> > > +}
> > > +
> > > +static int parse_af_spec_bridge(struct rtnl_link *link, struct
> > > nlattr *attr)
> > > +{
> > > +	struct rtnl_link_af_ops *af_ops;
> > > +	struct nlattr *af_attr;
> > > +	char *af_data;
> > > +	int remaining;
> > > +	int err = 0;
> > > +
> > > +	af_ops = af_lookup_and_alloc(link, AF_BRIDGE);
> > > +	af_data = link->l_af_data[AF_BRIDGE];
> > > +
> > > +	if (af_ops && af_ops->ao_parse_af) {
> > > +		nla_for_each_nested(af_attr, attr, remaining) {
> > > +			err = af_ops->ao_parse_af(link, af_attr,
> > > af_data);
> > > +			if (err < 0)
> > > +				goto errout;
> > > +		}
> > > +	}
> > > +
> > > +	link->ce_mask |= LINK_ATTR_AF_SPEC;
> > > +errout:
> > 
> > same as above
> 
> same response -- caller did the alloc and caller handles the put on
> error.
> 
> > 
> > > +	return err;
> > > +}
> > > +
> > >   static int link_msg_parser(struct nl_cache_ops *ops, struct
> > > sockaddr_nl *who,
> > >   			   struct nlmsghdr *n, struct
> > > nl_parser_param *pp)
> > >   {
> > > @@ -601,21 +651,26 @@ static int link_msg_parser(struct
> > > nl_cache_ops
> > > *ops, struct sockaddr_nl *who,
> > >   	}
> > > 
> > >   	if (tb[IFLA_AF_SPEC]) {
> > > -		struct nlattr *af_attr;
> > > -		int remaining;
> > > -
> > > -		nla_for_each_nested(af_attr, tb[IFLA_AF_SPEC],
> > > remaining) {
> > > -			af_ops = af_lookup_and_alloc(link,
> > > nla_type(af_attr));
> > > -			if (af_ops && af_ops->ao_parse_af) {
> > > -				char *af_data = link-
> > > > l_af_data[nla_type(af_attr)];
> > > -
> > > -				err = af_ops->ao_parse_af(link,
> > > af_attr, af_data);
> > > -				if (err < 0)
> > > -					goto errout;
> > > -			}
> > > +		struct nl_cache *cache = (struct nl_cache *) pp-
> > > > pp_arg;
> > > 
> > 
> > I do not think this is valid all the time. For instance if you
> > call nl_msg_parse pp_arg is a struct dp_xdata. The same goes for
> > using
> > the cache manager.
> 
> Hmmmm.... when I traced it code paths leading to link_msg_parser have
> pp_arg set to the cache. But ...
> 
> > 
> > Is there a way to determine the origin, that you can make sure you
> > can
> > use nl_cache? Otherwise I guess you have to stick with the family
> > of
> > the nlmsg as I proposed in my patch.
> 
> The AF_SPEC has to be parsed based on the family used for the request
> message. The local family variable is overridden processing the 
> IFLA_LINKINFO attribute. If you do a link list with AF_UNSPEC you
> want 
> to parse AF_SPEC with family = 0 even though the link kind is a
> bridge.
> 
> Looking at the code again the simplest thing to do is leave family
> set 
> based on ifi_family. ie., don't need the pp_arg cast.
> 

oh, I haven't seen the override in IFLA_LINKINFO processing. Hence I
agree fully on ifi_family.

/tobi