GNOME Bugzilla – Bug 754570
platform refactor to drop _nl_link_parse_info_data() for get_properties()
Last modified: 2015-11-02 13:05:29 UTC
nm-linux-platform.c has _nl_link_parse_info_data(), which is used by infiniband_get_info, macvlan_get_properties, vxlan_get_properties, etc to get link-specific data. They basically refetch the link via netlink and return the type-specific data. It would be better, if our NMPObjectLink would also cache those type specific data. Certain link-types have additional data (like they have in libnl). It should be cached in NMPObjectLink. The advantage is not only better performance due to a saved netlink-request-round-trip. The problem with the explicit request is that it is out of sync with the cache state. For example, the link could have already disappeared on priv->nlh, but it would be still in the cache. Also, we would get change-notifications for those properties too.
Pushed branch: th/platform-parse-nl-bgo754570 It's still WIP.
Ready now: th/platform-parse-nl-bgo754570 (it needs more testing though, and I had the feeling that parsing vxlan is borked).
(In reply to Thomas Haller from comment #2) > Ready now: th/platform-parse-nl-bgo754570 btw, I tested it quite well. It is ready.
I only did a quick review and so far LGTM. Only minor suggestions below. > wifi: reimplement use of libnl-genl-3 library + nlh = nlmsg_put(msg, port, seq, family, GENL_HDRLEN + hdrlen, flags); Missing space between function name and open parenthesis (multiple times in the chunk). > platform: add buffer argument to platform to-string functions - _LOGT (vtable->vt->addr_family, "%3d: STATE: update #%u - %s", ifindex, i_ipx_routes, vtable->vt->route_to_string (cur_ipx_route)); + _LOGT (vtable->vt->addr_family, "%3d: STATE: update #%u - %s", ifindex, i_ipx_routes, vtable->vt->route_to_string (cur_ipx_route, NULL, 0)); Can we try to limit line length? File CONTRIBUTING says: * 80-cols is a guideline, don't make the code uncomfortable in order to fit in less than 80 cols. In general I agree with that and I would break lines around 80-100 if possible. > nmp-object: allow missing implementations for certain virtual functions Be more resilant and allow for missing implementation. resilient? > platform: implement vxlan properties as lnk data + _CMP_FIELD (a, b, parent_ifindex); + _CMP_FIELD (a, b, parent_ifindex); Remove one line. + [NMP_OBJECT_TYPE_LNK_VXLAN - 1] = { + .obj_type = NMP_OBJECT_TYPE_LNK_VXLAN, + .sizeof_data = sizeof (NMPObjectLnkVxlan), + .sizeof_public = sizeof (NMPlatformLnkVxlan), + .obj_type_name = "vlan", "vxlan" (the same applies to other object types).
(In reply to Beniamino Galvani from comment #4) > I only did a quick review and so far LGTM. Only minor suggestions > below. > > > wifi: reimplement use of libnl-genl-3 library > > + nlh = nlmsg_put(msg, port, seq, family, GENL_HDRLEN + hdrlen, flags); > > Missing space between function name and open parenthesis (multiple > times in the chunk). Fixed. Also added a new commit fixing whitespace in the rest of the file. > > platform: add buffer argument to platform to-string functions > > - _LOGT (vtable->vt->addr_family, "%3d: STATE: update #%u - %s", > ifindex, i_ipx_routes, vtable->vt->route_to_string (cur_ipx_route)); > + _LOGT (vtable->vt->addr_family, "%3d: STATE: update #%u - %s", > ifindex, i_ipx_routes, vtable->vt->route_to_string (cur_ipx_route, NULL, 0)); > > Can we try to limit line length? File CONTRIBUTING says: > > * 80-cols is a guideline, don't make the code uncomfortable in order to fit > in > less than 80 cols. > > In general I agree with that and I would break lines around 80-100 if > possible. Changed some long lines. > > nmp-object: allow missing implementations for certain virtual functions > Be more resilant and allow for missing implementation. > > resilient? fixed. > > platform: implement vxlan properties as lnk data > > + _CMP_FIELD (a, b, parent_ifindex); > + _CMP_FIELD (a, b, parent_ifindex); > > Remove one line. Fixed. > + [NMP_OBJECT_TYPE_LNK_VXLAN - 1] = { > + .obj_type = > NMP_OBJECT_TYPE_LNK_VXLAN, > + .sizeof_data = sizeof > (NMPObjectLnkVxlan), > + .sizeof_public = sizeof > (NMPlatformLnkVxlan), > + .obj_type_name = "vlan", > > "vxlan" (the same applies to other object types). Fixed. And repushed. Thanks
> platform: move NMLinkType utils to "nm-platform-utils.h" Hmm, having to put the [25] for the array size in the header is pretty evil; is there no way around that? Could we do a LINK_TYPE_MAX as the sentinal element at the end of the array and rework the loops to "for (i = 0; __linktypes[i] != LINK_TYPE_MAX; i++)" ? At the very least a code comment to make sure that when we add link types we update the array size. > platform: parse netlink messages ourselves without libnl-route-3 > wifi: reimplement use of libnl-genl-3 library I'd like to note here that since you're copying LGPLv2.1 code into a GPL project, you need to be careful about licensing. Probably you've already looked into it; but just to make sure I also did and it turns out you *can* copy LGPLv2.1 code and relicense it as GPLv2+ due to an explicit provision in LGPLv2.1. So it worked out OK this time, but we need to be careful :) > platform: drop nm_platform_veth_get_properties() > platform: remove unused function nmp_utils_ethtool_get_peer_ifindex() veth-peer-in-IFLA_LINK only landed in the 4.1 kernel, so we need to keep the ethtool implementation around for a while.
(In reply to Dan Williams from comment #6) > > platform: move NMLinkType utils to "nm-platform-utils.h" > > Hmm, having to put the [25] for the array size in the header is pretty evil; > is there no way around that? Could we do a LINK_TYPE_MAX as the sentinal > element at the end of the array and rework the loops to "for (i = 0; > __linktypes[i] != LINK_TYPE_MAX; i++)" ? > > At the very least a code comment to make sure that when we add link types we > update the array size. If you *add* link types, you get a compiler error. So the issue is at most, that when removing link-types and forgetting to shrink then array, you wast some bytes. Added a code comment. > > platform: parse netlink messages ourselves without libnl-route-3 > > wifi: reimplement use of libnl-genl-3 library > > I'd like to note here that since you're copying LGPLv2.1 code into a GPL > project, you need to be careful about licensing. Probably you've already > looked into it; but just to make sure I also did and it turns out you *can* > copy LGPLv2.1 code and relicense it as GPLv2+ due to an explicit provision > in LGPLv2.1. So it worked out OK this time, but we need to be careful :) Note that there was already a comment: // Code is stolen from rtnl_link_get_kernel(), nl_pickup(), // and link_msg_parser(). from commit e9f364548a65fd4e26bf22367fe7c28fe127ab41 Indeed, libnl3 is under LGPLv2.1, which can be re-licensed as GPLv2+ https://www.gnu.org/licenses/gpl-faq.en.html#AllCompatibility > > platform: drop nm_platform_veth_get_properties() > > platform: remove unused function nmp_utils_ethtool_get_peer_ifindex() > > veth-peer-in-IFLA_LINK only landed in the 4.1 kernel, so we need to keep the > ethtool implementation around for a while. Fix. Fallback to ethtool restored. Two more fixups and repushed.
(Maybe some of the notes below are obsoleted by your last push, in that case ignore them). > platform: create netlink messages direclty without libnl-route-3 s/direclty/directly/ + /* Return true, because the netlink request succeeded. This doesn't indicate that the + * object is now actuall in the cache, because there could be a race. */ actually -do_change_link (NMPlatform *platform, struct rtnl_link *nlo, gboolean complete_from_cache) +do_change_link_2 (NMPlatform *platform, int ifindex, struct nl_msg *nlmsg) Why the _2 ? > platform: promise that the link lnk is an immutable NMPObject and expose it +static gconstpointer +_get_lnk_gre (NMPlatform *self, int ifindex, NMLinkType link_type, const NMPlatformLink **out_link) The _gre suffix looks wrong here. > platform/vlan: add support for ingress/egress-qos-mappings and changing flags + for (i = 0, j = 0; i < array->len; i++) { + struct nm_ifla_vlan_qos_mapping *map; + + map = array->pdata[i]; + + if ( j > 0 + && list[j - 1].from == map->from) + list[j - 1] = *map; + else + list[j++] = *map; + } Maybe add a comment explaining why this is needed. Can there be duplicated entries? And if so, will @to always be the same when @from is the same? +static gboolean +_vlan_ifla_vlan_qos_mapping_from_nla (struct nlattr *nlattr, + gboolean is_ingress_map, + const struct nm_ifla_vlan_qos_mapping **out_map, + guint *out_n_map) @is_ingress_map is never used (but callers seems to rely on it).
thanks. Addressed all and repushed.
The branch is almost done. I only get random test failures on RHEL7 which needs to be investigated first.
merged to master: http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=07c0a4952eb017e941e55fe601669818d73d021a