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

[Tobias Jungel]

Hi David,

cool work, still I have some comments inline.

/tobi


On Di, 2015-11-24 at 11:56 -0800, David Ahern wrote:
> The encoding of the IFLA_AF_SPEC attribute varies depending on the
> family
> used for the request (RTM_GETLINK) message. For AF_UNSPEC the
> encoding
> has another level of nesting for each address family with the type
> encoded
> first. i.e.,
>     af_spec = nla_nest_start(skb, IFLA_AF_SPEC)
>     for each family:
>         af = nla_nest_start(skb, af_ops->family)
>         af_ops->fill_link_af(skb, dev, ext_filter_mask)
>         nest_end
>     nest_end
> 
> This allows the parser to find the address family by looking at the
> first
> type.
> 
> Whereas AF_BRIDGE encoding is just:
>     af_spec = nla_nest_start(skb, IFLA_AF_SPEC)
>     br_fill_ifvlaninfo{_compressed}(skb, vg)
>     nest_end
> 
> which means the parser can not use the attribute itself to know the
> family
> to which the attribute belongs.
> 
> Signed-off-by: David Ahern <dsa at cumulusnetworks.com>
> ---
>  lib/route/link.c | 81
> +++++++++++++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 68 insertions(+), 13 deletions(-)
> 
> 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.

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

> +	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.

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.


> +		/* parsing of IFLA_AF_SPEC is dependent on the
> family used
> +		 * in the request message which is conveniently
> passed via
> +		 * the nl_parser_param arg.
> +		 */
> +		switch(cache->c_iarg1) {
> +		case AF_BRIDGE:
> +			err = parse_af_spec_bridge(link,
> tb[IFLA_AF_SPEC]);
> +			break;
> +		case AF_UNSPEC:
> +			err = parse_af_spec_unspec(link,
> tb[IFLA_AF_SPEC]);
> +			break;
> +		default:
> +			err = -EINVAL;
> +			NL_DBG(1, "IFLA_AF_SPEC parsing not
> implemented for family %d\n",
> +				family);
>  		}
> -		link->ce_mask |= LINK_ATTR_AF_SPEC;
> +		if (err)
> +			goto errout;
>  	}
>  
>  	if (tb[IFLA_PROMISCUITY]) {