[th/connectivity-per-af-fixes] #255
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lkundrak
force-pushed
the
th/connectivity-per-af-fixes
branch
5 times, most recently
from
89d33a6
to
a91a70b
Compare
thom311
force-pushed
the
th/connectivity-per-af-fixes
branch
from
a91a70b
to
aa1c5e8
Compare
lkundrak
force-pushed
the
th/connectivity-per-af-fixes
branch
2 times, most recently
from
67da30a
to
76f4b2a
Compare
thom311
force-pushed
the
th/connectivity-per-af-fixes
branch
3 times, most recently
from
642a9dd
to
5344e88
Compare
lkundrak
force-pushed
the
th/connectivity-per-af-fixes
branch
from
5344e88
to
05e980c
Compare
Fixed by d45eed4 |
lkundrak
force-pushed
the
th/connectivity-per-af-fixes
branch
2 times, most recently
from
77bd11a
to
ddead7c
Compare
Since we determine the connectivity state of each device individually, the global connectivity state is an aggregate of all these states. I am not sure about considering here devices that don't have the (best) default route for their respective address family. But anyway. When we aggregate the best connectivity, we chose the numerical largest value. That is wrong, because PORTAL is numerically smaller than LIMITED. That means, if you have two devices, one with connectivity LIMITED and one with connectivity PORTAL, then LIMITED wrongly wins. Fixes: 6b7e9f9 https://bugzilla.redhat.com/show_bug.cgi?id=1619873
The NMDBusManager owns a reference to the system bus. Expose it, so we can use it. Note that g_bus_get() -- as alternative to get the systembus singleton -- is asynchronous, and g_bus_get_sync() has an API that makes one wonder what it does. Since we already have a reference to the connection object we want to use, expose it.
It just makes sense, that our to-char function can also handle AF_UNSPEC. Unclear which character to return in this case, but "IPvX" seems suitable.
While at it, replace "AF_INET" with "IPv". The connectivity check logging line is already much too long. Save a few characters. Also, I think the meaning of "AF_INET" is less clear (to a novice user) than "IPv4".
In most cases, we don't want the translated string (only marked for translation, and then programatically the caller deciedes whether to translate or not). The few places that always call gettext() can do it explicitly. Now, that our functions are all "no_l10n" by default, rename them.
Each time when enabling/disabling "systemd-resolved" in combination with another plugin (which is unchanged), another pair of signal handlers was connected. That's wrong. Fixes: d4eb4cb
…mes first If the user disabled systemd-resolved, two things seem apparent: - the user does not want us to use systemd-resolved - NetworkManager is not pushing the DNS configuration to systemd-resoved. It seems to me, we should not consult systemd-resolved in that case.
- use cleanup attribute except explicit free/unref. - check that the result has the expected address family. Arguably, it should always, unless there is a bug in systemd-resolved. By that reasoning, we also wouldn't have to check the address length either. - don't use strndup() for values that are later freed by g_free(). We should always agree whether to malloc/free or g_malloc/g_free. - don't use strcasecmp(). Always use the locale independent g_ascii_strcasecmp() instead. - use nm_utils_inet_ntop() instead of inet_ntop(). It's our preferred wrapper, which as a stricter semantic (for example, it cannot fail and it's input arguments are stricter defined). - use nm_clear_g_free() instead of g_clear_pointer().
During a config change notification, we determine a "changed" value, to know whether things significantly changed. Also, we want to log a warning about invalid configuration, only when the config actually changed. Previously, when the URI was invalid, on every reload (SIGHUP) we would log an error message, even if the configuration did not change. There is no need to warn multiple times about the same thing. Keep track of the original URI in priv->uri. Whenever that changed, we know the user reconfigured something. But also, now the URI might be set to an invalid value. That means, we need to remember whether the URI is valid. Also, log a warning if we fail to parse the host and port. Already before, such an URI was considered invalid and we would effectively not to connectivity checking.
… check The settings of NMConnectivity can change any time, by reloading the configuration. When reloading the configration, we don't want to interrupt or cancel the pending reuqests, they should just complete with the old settings with which they started. Note, that NMDevice is smart enough, that when a newer request completes earlier, it invalidates all older, still pending requests. Anyway, that means, we cannot rely on the value to stay alive. Fix that, by adding adding a new ref-counted struct for these parameters. Fixes: 2cec94b
If the URI does not specify a port, we always assumed "80". That is wrong for https. Arguably, https is discouraged for connectivity checking, but we still shouldn't break it. Fixes: 9664f28
When we agregate the connectivity state, only devices that have the best default route should be considered. Since we do connectivity checking per-device, the per-device check does not care whether traffic to the internet is really routed via this device. But when talking about the global connectivity state, we care mostly about the (best) default route. So, we should not allow a device with worse or now default route, to contribute its connectivity state. Fixes: 6b7e9f9
lkundrak
force-pushed
the
th/connectivity-per-af-fixes
branch
from
ddead7c
to
bd5ac3f
Compare
|
merged to master. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.