After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 702515 - [review] dcbw/ping-gateway2
[review] dcbw/ping-gateway2
Status: RESOLVED FIXED
Product: NetworkManager
Classification: Platform
Component: IP and DNS config
git master
Other Linux
: Normal normal
: ---
Assigned To: NetworkManager maintainer(s)
NetworkManager maintainer(s)
Depends on:
Blocks:
 
 
Reported: 2013-06-17 21:05 UTC by Dan Williams
Modified: 2013-06-21 21:18 UTC
See Also:
GNOME target: ---
GNOME version: ---



Description Dan Williams 2013-06-17 21:05:40 UTC
There are a number of RH bugs open about connectivity problems after we've got IP set up, especially in static configurations.  These usually stem from things out of NM's control, like switches that don't actually enable a port for traffic for a short period of time after the link comes up.  There's not a lot we can do about this other than implement a link delay.

There are actually two alternate branches: dcbw/ping-gateway2 and dcbw/ping-gateway.  gateway2 puts the new property in NMSettingConnection, while gateway puts individual properties in NMSettingIP4Config and NMSettingIP6Config.  I feel like the NMSettingConnection one (gateway2) is cleaner, and I can't really think of a reason somebody would want to ping an IPv4 gateway but not an IPv6 one in the same connection.  So I'd prefer the gateway2, but if somebody can think of a reason the properties should be split, please comment on that.
Comment 1 Pavel Simerda 2013-06-18 09:32:24 UTC
(In reply to comment #0)
> I feel like the NMSettingConnection one (gateway2) is
> cleaner, and I can't really think of a reason somebody would want to ping an
> IPv4 gateway but not an IPv6 one in the same connection.  So I'd prefer the
> gateway2, but if somebody can think of a reason the properties should be split,
> please comment on that.

No strong opinion on that.

I think that a good idea would be to delay default gateway setting *after* the default gateway was checked. The rationale to do that is to prevent any running applications from using the default gateway before it is checked, whether or not they specifically support NetworkManager.

Also, we have nm_ip6_config_get_gateway() function and we could just as well add nm_ip4_config_get_gateway() for the same purpose. For the beginning it could just do the cycling through address records, then it could be changed to do caching and finally the gateway could be decoupled.
Comment 2 Dan Williams 2013-06-21 03:45:22 UTC
(In reply to comment #1)
> (In reply to comment #0)
> > I feel like the NMSettingConnection one (gateway2) is
> > cleaner, and I can't really think of a reason somebody would want to ping an
> > IPv4 gateway but not an IPv6 one in the same connection.  So I'd prefer the
> > gateway2, but if somebody can think of a reason the properties should be split,
> > please comment on that.
> 
> No strong opinion on that.
> 
> I think that a good idea would be to delay default gateway setting *after* the
> default gateway was checked. The rationale to do that is to prevent any running
> applications from using the default gateway before it is checked, whether or
> not they specifically support NetworkManager.

Actually it looks like we do that already.  The ping is done in the IP_CHECK state, but a device is only a candidate for being default in the SECONDARIES and ACTIVATED states.  See get_best_ip6_device() and get_best_ip4_device().

> Also, we have nm_ip6_config_get_gateway() function and we could just as well
> add nm_ip4_config_get_gateway() for the same purpose. For the beginning it
> could just do the cycling through address records, then it could be changed to
> do caching and finally the gateway could be decoupled.

Done.  Changes pushed as dcbw/ping-gateway.  Look OK?
Comment 3 Pavel Simerda 2013-06-21 12:31:44 UTC
Looks good.
Comment 4 Dan Williams 2013-06-21 21:18:13 UTC
Merged.