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

[Thomas Haller]

On Wed, 2015-12-02 at 16:15 +0100, Tobias Jungel wrote:
> 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.


ah, I see.

+    .ao_parse_af             = &bridge_parse_af,

was only recently added (by David's patch). master currently doesn't
parse it at all. Good then.





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


If you'd expose it as internal API via include/netlink-private/ there
wouldn't be a real problem.




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


I tried to do what I was talking about. It actually applied more to
David's patch then yours. See my other email and
https://github.com/thom311/libnl/commit/bridge-vlan


Thomas
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part
URL: <http://lists.infradead.org/pipermail/libnl/attachments/20151202/58c66bff/attachment.sig>