[PATCH 2/2] route/link: handle RTEXT_FILTER_BRVLAN_COMPRESSED

[Tobias Jungel]

On Mi, 2015-12-02 at 15:21 +0100, Thomas Haller wrote:
> On Thu, 2015-11-26 at 16:47 +0100, Tobias Jungel wrote:
> > notifications from the kernel regarding vlan ids are now handled
> > ---
> >  lib/route/link.c        | 12 +++-----
> >  lib/route/link/bridge.c | 82
> > ++++++++++++++++++++++++++++++++++++++++++++-----
> >  2 files changed, 78 insertions(+), 16 deletions(-)
> 
> Hi Tobias,
> 
> 
> 
> before your patch, there were three implementations of ao_af_parse:
> 
>   lib/route/link/bridge.c:     .ao_parse_af             =
> &bridge_parse_af,
>   lib/route/link/inet.c:  .ao_parse_af             = &inet_parse_af,
>   lib/route/link/inet6.c: .ao_parse_af             =
> &inet6_parse_protinfo,
> 
> and two callers in lib/route/link.c:
>   parse_af_spec_unspec()
>   parse_af_spec_bridge()
> 
> 
> with the change:
> 
> -    .ao_parse_af             = &bridge_parse_af,
> +    .ao_parse_af_full        = &bridge_parse_af,
> 
> 
> parse_af_spec_unspec() can no longer parse AF_BRIDGE as a nested
> attribute (AF_BRIDGE can then only be parsed as top-level attribute
> via
> parse_af_spec_bridge()).
> Is parsing AF_BRIDGE as nested in parse_af_spec_unspec() not
> necessary?

No, its not necessary. The kernel nests bridge attributes directly in
AF_SPEC.

> I think you have to preserve the original
>   .ao_parse_af             = &bridge_parse_af,
> too.
> 

No, bridge_parse_af deals now with all nested attributes.

I had the discussion earlier with David, if we should
overload ao_parse_af or create separate call in this thread:

http://lists.infradead.org/pipermail/libnl/2015-November/002045.html


> 
> 
> 
> 
> Second:
> 
> 
> The patch "Handle family-based parsing of IFLA_AF_SPEC attribute"
> adds:
> 
> [1]
>        switch(family) {
>                 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;
> 
> thus making the link_msg_parser() aware of how to parse AF_BRIDGE.
> 
> 
> 
> At this point you can do one of two things:
> 
> (a) have parse_af_spec_bridge() call directly to bridge_parse_af()
> without
>     the intermediate step of introducing ao_parse_af_full().
>     You already *know* that you parse a AF_BRIDGE. There is no point
> in using
>     the virtual function table to invoke the bridge parser.

This implies to expose bridge_parse_af. Is this a good idea?

> 
> (b) Or better: change [1] to be actually generic:
> 
>    if (tb[IFLA_AF_SPEC]) {
>          /* parsing of IFLA_AF_SPEC is dependent on the family used
>           * in the request message.
>           */
>          if (family == AF_UNSPEC)
>              err = parse_af_spec_unspec(link, tb[IFLA_AF_SPEC]);
>          else if (af_ops && af_ops->ao_parse_af_full) {
>              /* call to ao_parse_af_full, as you did in
> parse_af_spec_bridge().
>                 possibly renaming parse_af_spec_bridge() to
> parse_af_spec_full()
>                 or better just copy the content
> of parse_af_spec_bridge() to here. */
>          } else {
>              NL_DBG(1, "IFLA_AF_SPEC parsing not implemented for
> family %d\n",);
>          }

I would rather go for this solution. If there are not any further
issues in using ao_parse_af_full, I will do this tomorrow.

/tobi

> 
> 
> 
> 
> 
> 
> Thomas