GNOME Bugzilla – Bug 726273
[review] th/bgo726273_platform_delete_ip4_route_workaround
Last modified: 2014-05-27 20:03:27 UTC
When deleting an IPv4 route via netlink, the scope in the RTM_DELADDR message must match -- unless the scope is set to RT_SCOPE_NOWHERE. However, current libnl will always overwrite the route scope if it is set to RT_SCOPE_NOWHERE. This is fixed recently in https://github.com/thom311/libnl/commit/85ec9c7ad80c60f4f619472f2bb9d9595da93b26 NM tries to workaround this by looking at cached routes, but this still might fail. Recent libnl adds a function nl_has_capability() that allows NM to check if libnl has the broken behaviour. Therefore we can omit the workaround in that case. Also note, that we no longer use the @cached_object in delete_object() but instead always use the created one. Using the cached one, was only necessary because of this issue with deleting IPv4 routes.
Is there any reason you use dlopen() instead of g_module_open()? (I am fine with that, just asking). Put a space after dlopen/dlsym and the opening "(".
(In reply to comment #1) > Is there any reason you use dlopen() instead of g_module_open()? (I am fine > with that, just asking). > > Put a space after dlopen/dlsym and the opening "(". No particular reason. I think g_module_open() is mainly useful for portability, which we don't need in this case. dlopen() is convenient enough. Repushed (after rebasing to master + fixing white space + improving commit message).
I don't like the whole dynamic thing. We don't do that for anything else, we shouldn't do it here. Just add a configure test and decide at compile time. If the user is building from source then they'll end up rebuilding at some point after upgrading libnl anyway, and if they're using packages, then the packager should deal.
(In reply to comment #3) > I don't like the whole dynamic thing. We don't do that for anything else, we > shouldn't do it here. Just add a configure test and decide at compile time. If > the user is building from source then they'll end up rebuilding at some point > after upgrading libnl anyway, and if they're using packages, then the packager > should deal. I really like it. Compile time dependencies are so annoying, and should be avoided where it can be done easily. It's also annoying for packaging, because you are probably not going to rebuild NM because libnl3 package gets updated. This solution just works, at a few extra lines of code (more then a autoconf/makefile change??). And if we later happen to use another libnl function dynamically, it will amortize.
Shouldn't the libnl capability stuff go into nm-linux-platform instead of the generic platform, which doesn't have any knowledge of libnl?
(In reply to comment #5) > Shouldn't the libnl capability stuff go into nm-linux-platform instead of the > generic platform, which doesn't have any knowledge of libnl? Possible. I was thinking, that we might add more functions here later (that we load dynamically, with fallback wrappers). So, the nm_platform_nl_* functions would be general compatibility wrappers around libnl. They could be useful outside of nm-linux-platform... for example wifi-utils-nl80211.c uses libnl too... ok, this file will soon move into the platform directory, but even then it should not include nm-linux-platform.h, but nm-platform.h.
I'm just thinking about the fake platform and making sure that we keep most of the libnl stuff encapsulated inside the linux implementation, if we can. If just for testability. Should we keep them in the linux implementation for now, and if we do need them outside later, we can figure out how best to do that?
(In reply to comment #7) > I'm just thinking about the fake platform and making sure that we keep most of > the libnl stuff encapsulated inside the linux implementation, if we can. If > just for testability. Should we keep them in the linux implementation for now, > and if we do need them outside later, we can figure out how best to do that? I think that those nm_platform_nl_* functions in "nm-platform.h" should be simply wrappers around libnl functions. E.g. functions loaded dynamically with dlopen and offering a way to workaround for non-available functions. Certainly they should not be complex or modify internal state (as the handling inside of nm-linux-platform often does). As such, they can be useful outside of nm-linux-platform. E.g. src/platform/wifi/wifi-utils-nl80211.c which uses libnl too.
The *could* be, but they aren't used anywhere else yet that I can see, and they'll let some of the Linux-specific stuff leak through. I don't have a problem with the dlopening stuff, just that the code is "public" to the rest of NM but nothing outside the platform actually uses it yet. If we have something in the future that does use it, then we could move it out. But I think until that happens, we should keep it "private".
Rebased on master and repushed. All changes are now inside nm-linux-platform.h
Looks good to me!
Merged as: http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=e3221e6a1243abd59dfa6952f8ed599fbe2bf111
The nm-platform was meant to abstract from stuff like libnl. In my opinion, any dependency of nm-platform on libnl just breaks work already done. I believe anything linux and libnl specific should go to nm-linux-platform instead.
(In reply to comment #13) > The nm-platform was meant to abstract from stuff like libnl. In my opinion, any > dependency of nm-platform on libnl just breaks work already done. I believe > anything linux and libnl specific should go to nm-linux-platform instead. All changes of this branch are contained entirely inside nm-linux-platform.c.
(In reply to comment #14) > (In reply to comment #13) > > The nm-platform was meant to abstract from stuff like libnl. In my opinion, any > > dependency of nm-platform on libnl just breaks work already done. I believe > > anything linux and libnl specific should go to nm-linux-platform instead. > > All changes of this branch are contained entirely inside nm-linux-platform.c. Then everything's alright. Thanks.
Previously merged branch added a regression. Fixed now with commit http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=f0daf90298d1bd9cafac7b9c02dc905327e0b85a