GNOME Bugzilla – Bug 740443
[review] th/bgo740443_device_ip_changes - IP configuration to show actually configured addresses and routes
Last modified: 2015-01-24 17:52:02 UTC
On platform changes, we call update_ip_config(), which eventually calls ip4_config_merge_and_apply() with commit=FALSE. In this case we construct priv->ip4_config based on settings that we expect (such as content from nm_setting_ip_config()). This is not necessarily the same that is actually configured on the physical interface. See https://bugzilla.gnome.org/show_bug.cgi?id=735512#c51 for a longer discussion
Pushed branch for review: th/bgo740443_device_ip_changes This contains yet another major change to default routes :) But this time it is great. NM will now gladly accept any default route you configure externally. Even for managed, non-assumed interfaces.
> core: add intersect() functions to NMIP?Config Should the intersect logic for 'gateway' be a bit more complete? eg if (!nm_ip4_config_get_num_addresses (dst) || (nm_ip4_config_get_gateway (src) != nm_ip4_config_get_gateway (dst))) nm_ip4_config_set_gateway (dst, 0); (same for IPv6) > device: refactor ipx_config_merge_and_apply() This conflicts with some of the VPN routing changes in git master so it'll need a careful rebase. Also there are some double-newlines in ip6_merge_and_apply() around the has_direct_route bit. > device: better accept external IP changes I would call it "ensure_con_ip_configs()" to better match other places that do the same type of thing. I'd also put short comments into update_ip_config() about both the intersection and the subtraction, about what exactly the intended result of each operation is supposed to be. For example the intersection will remove any IP configuration from priv->XXX_ip4_config that is not actually present in the kernel. The subtraction will remove any auto-or-settings-sourced configuration from priv->ext_ip4_config, leaving only external changes in priv->ext_ip4_config. > device: always pickup externally configured default routes for a device Commit message has "new configuraiton tobe" -> "new configuration to be" This comment is now wrong: + /* In the (!has_direct_route && !commit) case it is not clear whether + * adding the default route will succeed. Still give it a try and add it */ because this code path is never encountered for !commit cases. Which means this "commit" is also dead code because at this point commit == TRUE : + if (!has_direct_route && commit) { Same for IPv6. Rest looks good.
(In reply to comment #2) > > core: add intersect() functions to NMIP?Config > > Should the intersect logic for 'gateway' be a bit more complete? eg > > if (!nm_ip4_config_get_num_addresses (dst) || > (nm_ip4_config_get_gateway (src) != nm_ip4_config_get_gateway (dst))) > nm_ip4_config_set_gateway (dst, 0); > > (same for IPv6) done. > > device: refactor ipx_config_merge_and_apply() > > This conflicts with some of the VPN routing changes in git master so it'll need > a careful rebase. > > Also there are some double-newlines in ip6_merge_and_apply() around the > has_direct_route bit. the double-newlines are there to mirror the v4 code. I like to see them side by side to confirm that they mostly identical -- up to s/4/6/ > > device: better accept external IP changes > > I would call it "ensure_con_ip_configs()" to better match other places that do > the same type of thing. > > I'd also put short comments into update_ip_config() about both the intersection > and the subtraction, about what exactly the intended result of each operation > is supposed to be. > > For example the intersection will remove any IP configuration from > priv->XXX_ip4_config that is not actually present in the kernel. The > subtraction will remove any auto-or-settings-sourced configuration from > priv->ext_ip4_config, leaving only external changes in priv->ext_ip4_config. done > > device: always pickup externally configured default routes for a device > > Commit message has "new configuraiton tobe" -> "new configuration to be" > > This comment is now wrong: > > + /* In the (!has_direct_route && !commit) case it is not clear whether > + * adding the default route will succeed. Still give it a try and add it > */ > > because this code path is never encountered for !commit cases. > > Which means this "commit" is also dead code because at this point commit == > TRUE > : > > + if (!has_direct_route && commit) { > > Same for IPv6. done. Rebased on master.
Rebased on master
LGTM, though lets re-test it again to make sure it still has the behavior we expect. FYI it rebases on git master without problems.
It tests as expected to me after fixing a few bugs. Added two minor new commits: >> device: require a direct route for IPv6 gateway >> default-route: improve logging format for default route entries Rebased on master. IMO it's good to go now
New commits LGTM
merged to master as: http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=0c136c1e2cee1e8ec00d18f071562cc846a766a5