cnetlink: fetch bridge object with ifindex and address family

[rubensfig]

libnl 3.6.0 has introduced the following https://github.com/thom311/libnl/pull/264 patch. The added group to the dict has an effect of adding a second bridge interface entry to the cache, refering to the inet6 link notification.

By fetching the object with the ifindex and AF_BRIDGE, we filter for the correct object type.

Avoids the behaviour after the following instructions.

baseboxd up -> all ports created
ip link add swbridge type bridge vlan_filtering 1
ip link set port1 master swbridge

Good:

route_link_apply: new link bridge port1 ether 96:cf:87:84:86:23 master swbridge <broadcast,multicast>
link_created: using *bridge* bridge swbridge ether 0a:39:85:5b:25:67 <broadcast,multicast> group 0

Wrong:

link_created: using bridge *inet6* swbridge ether 0a:39:85:5b:25:67 <broadcast,multicast>
baseboxd: ../src/netlink/nl_bridge.cc:61: void basebox::nl_bridge::set_bridge_interface(rtnl_link*): Assertion `rtnl_link_is_bridge(bridge)' failed.

[KanjiMonster]

While the change itself looks good, we have a lot of places where we call get_link_by_ifindex, and I wonder if we should switch them all to get_link() with the family set to what we are looking for.

Or rewrite get_link_by_ifindex to ignore the AF_INET6 object, like (completely untested):

diff --git a/src/netlink/cnetlink.cc b/src/netlink/cnetlink.cc
index f2408049cdf6..4095733152d6 100644
--- a/src/netlink/cnetlink.cc
+++ b/src/netlink/cnetlink.cc
@@ -246,7 +246,22 @@ void cnetlink::destroy_caches() { nl_cache_mngr_free(mngr); }

 std::unique_ptr<struct rtnl_link, decltype(&rtnl_link_put)>
 cnetlink::get_link_by_ifindex(int ifindex) const {
-  rtnl_link *link = rtnl_link_get(caches[NL_LINK_CACHE], ifindex);
+  struct rtnl_link *link = nullptr;
+  std::unique_ptr<rtnl_link, decltype(&rtnl_link_put)> filter(rtnl_link_alloc(),
+                                                              &rtnl_link_put);
+
+  rtnl_link_set_ifindex(filter.get(), ifindex);
+
+  // search link by filter
+  nl_cache_foreach_filter(
+      caches[NL_LINK_CACHE], OBJ_CAST(filter.get()),
+      [](struct nl_object *obj, void *arg) {
+        if (rtnl_link_get_family(LINK_CAST(obj)) != AF_INET6) {
+          *static_cast<nl_object **>(arg) = obj;
+         nl_object_get(obj);
+       }
+      },
+      &link);

   // check the garbage
   if (link == nullptr) {

[rubensfig]

@KanjiMonster I would be partial to your first suggestion, removing the get_link/ get_link_by_ifindex differences in return values, and possibly passing some other filters like the AF or ifindex.

However, that sounds like a larger refactor, which I would probably do not include in this feature. Could you write up a refactor issue?

[KanjiMonster]

@KanjiMonster I would be partial to your first suggestion, removing the get_link/ get_link_by_ifindex differences in return values, and possibly passing some other filters like the AF or ifindex.

After a bit more investigation, I'm not quite sure this will work as expected:

$ ./src/nl-link-list 
lo loopback <loopback,up,running,lowerup> group 0 
enp0s31f6 ether e8:6a:64:5f:ab:74 <broadcast,multicast,up,running,lowerup> group 0 
wlp3s0 ether ba:55:f0:af:94:08 <broadcast,multicast> group 0 
bridge lxcbr0 ether 00:16:3e:00:00:00 <broadcast,multicast,up> group 0 
bridge docker0 ether 02:42:33:40:a9:ea <broadcast,multicast,up> group 0 

$ ./src/nl-link-list --family=bridge
bridge lxcbr0 ether 00:16:3e:00:00:00 master lxcbr0 <broadcast,multicast,up> 
bridge docker0 ether 02:42:33:40:a9:ea master docker0 <broadcast,multicast,up> 

$ ./src/nl-link-list --family=unspec
lo loopback <loopback,up,running,lowerup> group 0 
enp0s31f6 ether e8:6a:64:5f:ab:74 <broadcast,multicast,up,running,lowerup> group 0 
wlp3s0 ether ba:55:f0:af:94:08 <broadcast,multicast> group 0 

so there is no AF_ family where objects exist for both bridges and non-bridges (well, there is now, with AF_INET6, if inet6 is enabled ;P).

So unless we know beforehand if we want a bridge or a non-bridge link object, we would rather need to patch get_link_by_ifindex.

Might also be possible to argue that this is a bug in libnl, as it now breaks the assumption that get_link() will return either an AF_UNSPEC or a AF_BRIDGE object, depending on the link type.

[KanjiMonster]

Looks better now. Will give it a test.