Differential D51238
bridge: add a netlink interface Authored by ivy on Thu, Jul 10, 9:49 PM. Tags None Referenced Files
Details For easier review of the basic scaffolding, only two commands are Update ifconfig to use netlink for these operations. The netlink structure used for add member is called "modifymember",
Diff Detail
Event TimelineComment Actions i've never really used netlink before and there appears to be very little documention on it, so this might be completely wrong; it's pieced together from various other examples of netlink in sys/.
Comment Actions Thank you for working on that! Would it be possible to be compatible with Linux? Their version has generic way of marking an interface a "child" interface (aka IFLA_PARENT_DEV_NAME attribute in RTM_NEWLINK in rtnetlink ) and an extensive set of bridge member attributes: https://github.com/torvalds/linux/blob/379f604cc3dc2c865dc2b13d81faa166b6df59ec/include/uapi/linux/if_link.h#L467 / https://github.com/CumulusNetworks/ifupdown2/blob/7034177eeb034a0b1ea9c6792cd3af59f1fc39d0/ifupdown2/addons/bridge.py#L462 Comment Actions i don't think we can be compatible with Linux due to semantic differences. to take two examples i noticed, their version of VLAN filtering says if it's disabled, the bridge won't process VLANs at all, but our bridge does (it just doesn't filter them), and their vlan protocol mode (802.1q/802.1ad) seems to be for the bridge as a whole, while ours is per-interface. i think trying to match the Linux API might just end up being more confusing, since it'll appear to work but actually not work in subtle ways. Comment Actions Would you mind considering whether to add version numbers to your netlink APIs before you start landing them? Comment Actions i assume you mean setting the third argument, family_version, when calling genl_register_family? i notice i've set this is 2 here which is probably wrong. do we need to define a constant like BRIDGE_NL_VERSION_1 to use as the family version? Comment Actions move ifunit_ref() to bridge_cmd_add() make bridge_cmd_add return the new bridge_iflist since Netlink will need this, Comment Actions cache the bridge netlink socket across calls do this in ifbridge rather than putting it in if_ctx since it's specific to it seems to me that every call to snl_init_writer() here will leak memory, Comment Actions It's a little deceptive. snl_allocz() (as called in snl_init_writer()) doesn't actually allocate memory. It uses the buffer from the snl_state, so if we free the snl_state (snl_free()) we also free the writer's memory. Perhaps we could call bridge_init_netlink() in bridge_ctor(). There's no equivalent destructor, but it may be fine to not bother for ifconfig. That's always short-lived anyway, and memory leaks stop when the missile explo^W^W^W the process stops. Comment Actions There are multiple "levels" of compatibility to consider. I'd start with general interface configuration. Bridge interface is not significantly different from any other interface - and I don't think we should be creating generic netlink interface for each network interface we convert. In that sense I'd really prefer we follow the current netlink model, where we have per-interface custom properties (see vlan(4) for partial implementation) in rtnetlink. The semantics indeed differ. I don't think we need to be fully compatible - however, it's worth reusing the already existing bits. The overall goal for netlink is to make larger app infrastructure support FreeBSD easier. Having the similar model, but with some custom variables makes it really low-cost to support FreeBSD. Having totally incompatible interface won't be much different from existing ioctl mess. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||