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

[David Ahern]

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.