This wasn't a problem, because load_plugins() can only fails
if the settings plugins fail to load, which can only happen
if you have a broken installation.
Don't subscribe twice to the same signal. The more subscribers a
signal has, the more confusing it gets what is happening.
We can handle also the default-wired-connection in the regular
connection-removed signal.
Note how connection_removed() is registered with
g_signal_connect_after(), but that is fine. There are few subscribers
to this signal (that don't do anything that interferes here).
Especially, since all other subscribers subscribe with the same
priority (hence, are unordered). So, moving this task explicitly
to after, does not change any ordering guarantee -- in fact, it
ensures an ordering that was undefined previously. Anyway, it
doesn't matter.
Currently we overwrite the interface rp_filter value with 2 ("loose")
only when it is 1 ("strict") because when it is 0 ("no validation") it
is already more permissive.
So, if the value for the interface is 0 and
net/ipv4/conf/all/rp_filter is 1 (like it happens by default on Fedora
28), we don't overwrite it; since kernel considers the maximum between
{all,$dev}/rp_filter, the effective value remains 'strict'.
We should instead combine the two {all,$dev}/rp_filter, and if it's 1
overwrite the value with 2.
https://bugzilla.redhat.com/show_bug.cgi?id=1565529
There are multiple tests with the same in different directories; add a
unique prefix to test names so that it is clear from the output which
one is running.
Otherwise, if connectivity checking was disabled, we would never
reset the connectivity state and leave it wrongly at UNKNOWN.
nm_device_check_connectivity_update_interval() is already called
during state-changes, so this is the right place. However,
it's far from perfect still, because we might not notice when
a default-route gets added or removed. Also, devices that are not
in ACTIVATED state, are considered with connectivity NONE. Which
might not be correct.
Fixes: 0a62a0e903
The fake states still encode whether the device have a default-route.
So, they are not entirely useless. Also, don't add special handling
of "#if !WITH_CONCHECK" where we don't need it.
There is no fundamental difference between compiling without connectivity
check and disabling connectivity checks.
NMManager very much cares about changes to the connectivity state
of the device and was therefore listening to notify::connectivity
signals. However, property changed signals can be suppressed by
g_object_freeze_notify(). That is something we even encourage for
NMDBusObject instances, because the D-Bus glue makes use of the
property changed notifications, and encourages to combine multiple
changes by freezing the signal.
Using the property changed notifications of NMDBusObject instances is
ugly. Don't do that and instead add a special signal.
It probably was no problem in practice, because very likely the
chunk of memory was aligned already.
Also, drop non helpful comment and fix whitespace.
It might happen, that connectivitiy is lost only for a moment and
returns soon after. Based on that assumption, when we loose connectivity
we want to have a probe interval where we check for returning
connectivity more frequently.
For that, we handle tracking of the timeouts per-device.
The intervall shall start with 1 seconds, and double the interval time until
the full interval is reached. Actually, due to the implementation, it's unlikely
that we already perform the second check 1 second later. That is because commonly
the first check returns before the one second timeout is reached and bumps the
interval to 2 seconds right away.
Also, we go through extra lengths so that manual connectivity check
delay the periodic checks. By being more smart about that, we can reduce
the number of connectivity checks, but still keeping the promise to
check at least within the requested interval.
The complexity of book keeping the timeouts is remarkable. But I think
it is worth the effort and we should try hard to
- have a connectivity state as accurate as possible. Clearly,
connectivity checking means that we probing, so being more intelligent
about timeout and backoff timers can result in a better connectivity
state. The connectivity state is important because we use it for
the default-route penaly and the GUI indicates bad connectivity.
- be intelligent about avoiding redundant connectivity checks. While
we want to check often to get an accurate connectivity state, we
also want to minimize the number of HTTP requests, in case the
connectivity is established and suppossedly stable.
Also, perform connectivity checks in every state of the device.
Even if a device is disconnected, it still might have connectivity,
for example if the user externally adds an IP address on an unmanaged
device.
https://bugzilla.gnome.org/show_bug.cgi?id=792240
The main issue is that `nmcli networking connectivity check` uses
nm_client_check_connectivity(), which has a timeout of 25 seconds.
Using a timeout of 30 seconds server side, means that if the requests
don't complete in time, the client side will time out and abort
with a failure. That is not right.
Fix that by using a shorter timeout server side. 20 seconds is still
plenty for a small HTTP request. If the network takes longer than that,
it's fair to call that LIMITED connectivity.
An asynchronous request should either be cancellable or not keep
the target object alive. Preferably both.
Otherwise, it is impossible to do a controlled shutdown when terminating
NetworkManager. Currently, when NetworkManager is about to terminate,
it just quits the mainloop and essentially leaks everything. That is a
bug. If we ever want to fix that, every asynchronous request must be
cancellable in a controlled way (or it must not prevent objects from
getting disposed, where disposing the object automatically cancels the
callback).
Rework the asynchronous request for connectivity check to
- return a handle that can be used to cancel the operation.
Cancelling is optional. The caller may choose to ignore the handle
because the asynchronous operation does not keep the target object
alive. That means, it is still possible to shutdown, by everybody
giving up their reference to the target object. In which case the
callback will be invoked during dispose() of the target object.
- also, the callback will always be invoked exactly once, and never
synchronously from within the asynchronous start call. But during
cancel(), the callback is invoked synchronously from within cancel().
Note that it's only allowed to cancel an action at most once, and
never after the callback is invoked (also not from within the callback
itself).
- also, NMConnectivity already supports a fake handler, in case
connectivity check is disabled via configuration. Hence, reuse
the same code paths also when compiling without --enable-concheck.
That means, instead of having #if WITH_CONCHECK at various callers,
move them into NMConnectivity. The downside is, that if you build
without concheck, there is a small overhead compared to before. The
upside is, we reuse the same code paths when compiling with or without
concheck.
- also, the patch synchronizes the connecitivty states. For example,
previously `nmcli networking connectivity check` would schedule
requests in parallel, and return the accumulated result of the individual
requests.
However, the global connectivity state of the manager might have have
been the same as the answer to the explicit connecitivity check,
because while the answer for the manual check is waiting for all
pending checks to complete, the global connectivity state could
already change. That is just wrong. There are not multiple global
connectivity states at the same time, there is just one. A manual
connectivity check should have the meaning of ensure that the global
state is up to date, but it still should return the global
connectivity state -- not the answers for several connectivity checks
issued in parallel.
This is related to commit b799de281b
(libnm: update property in the manager after connectivity check),
which tries to address a similar problem client side.
Similarly, each device has a connectivity state. While there might
be several connectivity checks per device pending, whenever a check
completes, it can update the per-device state (and return that device
state as result), but the immediate answer of the individual check
might not matter. This is especially the case, when a later request
returns earlier and obsoletes all earlier requests. In that case,
earlier requests return with the result of the currend devices
connectivity state.
This patch cleans up the internal API and gives a better defined behavior
to the user (thus, the simple API which simplifies implementation for the
caller). However, the implementation of getting this API right and properly
handle cancel and destruction of the target object is more complicated and
complex. But this but is not just for the sake of a nicer API. This fixes
actual issues explained above.
Also, get rid of GAsyncResult to track information about the pending request.
Instead, allocate our own handle structure, which ends up to be nicer
because it's strongly typed and has exactly the properties that are
useful to track the request. Also, it gets rid of the awkward
_finish() API by passing the relevant arguments to the callback
directly.
Add a helper function to cache the current timestamp and return
it. The caching is a performance optimization, but it serves a
much more important purpose: repeatedly getting the timestamp
likely will yield different timings. So, commonly, within a
certain context we want to get the current time once, and stick
to that as "now".
Since commit 78ed0a4a23 (device: add
IPv6 link local address via merge-and-apply) we handle also IPv6 link
local addresses like regular addresses. That is, we also add them during
merge-and-apply and sync them via nm_platform_ip6_address_sync().
ip6-address-sync loops over the platform addresses, to find which
addresses shall be deleted, and which shall be deleted in order to
fix the address order/priority. At that point, we must not ignore
link-local addresses anymore, but handle them too.
Otherwise, during each resync we have link local addresses, and
platform-sync thinks that the address order is wrong. That wrongly
leads to remove most addresses and re-adding them.
Fixes: 78ed0a4a23
For completeness, extend the API to support non-persistant
device. That requires that nm_platform_link_tun_add()
returns the file descriptor.
While NetworkManager doesn't create such devices itself,
it recognizes the IFLA_TUN_PERSIST / IFF_PERSIST flag.
Since ip-tuntap (obviously) cannot create such devices,
we cannot add a test for how non-persistent devices look
in the platform cache. Well, we could instead add them
with ioctl directly, but instead, just extend the platform
API to allow for that.
Also, use the function from test-lldp.c to (optionally) use
nm_platform_link_tun_add() to create the tap device.
Previously, it was not (reliably) possible to use nmtstp_wait_for_link*() to
only look into the platform cache, without trying to poll the netlink
socket for events.
Add this option. Now, if the timeout is specified as zero, we never actually
read the netlink socket.
Currently, there are no callers who make use of this (by passing
a zero timeout). So, this is no change in existing behavior.
Implement nmtstp_assert_wait_for_link() and nmtstp_assert_wait_for_link_until()
as macros, based on nmtst_assert_nonnull().
This way, the assertion will report a more helpful file:line location,
instead of being somewhere nested inside test-common.c.
Due to a bug, the current rc-kernel will emit the first netlink
notification about tun devices before the device is initialized.
Hence, the content of the message is bogus. If the message
looks like to be this case, re-request it right away.
Now that kernel supports providing information about tun/tap devices
via netlink, make use of it.
Also, enable the hack that:
- when we first see a link that has no lnk data, we refetch
it on the assumption, that kernel just didn't send it
the first time.
For old kernels that do not yet support tun properties on netlink,
this means that we will always refetch the link once, the first
time we see it. I think that is acceptable, and the more correct
behavior for newer kernels that do support it.
Rework the code to if-else-if, to not schedule the same
DELAYED_ACTION_TYPE_REFRESH_LINK instance multiple times.
Note that delayed_action_schedule() already would check that
no duplicates are scheduled, but we can avoid that.
It doesn't make sense that NetworkManager adds non-persist tun
devices, likewise, only the type IFF_TUN or IFF_TAP is supported.
Assert that the values are as expected.
Switch from "pi on|off" to optinally printing "pi" to indicate
whether the flag is set. That follows ip-tuntap syntax and is
more familiar:
$ ip tuntap help
Usage: ip tuntap { add | del | show | list | lst | help } [ dev PHYS_DEV ]
[ mode { tun | tap } ] [ user USER ] [ group GROUP ]
[ one_queue ] [ pi ] [ vnet_hdr ] [ multi_queue ] [ name NAME ]
Where: USER := { STRING | NUMBER }
GROUP := { STRING | NUMBER }
Also, print the "persist" flag.
