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

[Thomas Haller]

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?
I think you have to preserve the original
  .ao_parse_af             = &bridge_parse_af,
too.





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.

(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",);
         }






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/cb05e593/attachment.sig>