We want to only check for autoconnect all, if something happend that
makes it possible that we can autoconnect now (while we couldn't
previously).
It's not a real problem to check more often then strictly necessary.
But add a check to rule out a few false-positives to avoid the
overhead of checking all devices for autoconnect.
reset_autoconnect_all() as two callers with only_no_secrets=FALSE:
- sleeping_changed() when NM returns from sleep.
- when device changes to state DISCONNECTED with reason
CARRIER.
In both cases, this should not overwrite a previous decision by
the user that the connection should not autoconnect.
schedule_activate_all() is expensive. It iterates over all devices, and asks
them to autoactivated (which might involve iterating over all connections for
each device). Avoid it if nothing changed.
Extend the enum and API to use flags for the blocked reasons.
A connection is blocked from autoconnect if it has any reason
set.
There is no behavioral change in this patch beyond that, because
where we previously would set blocked-reason NONE, we would still
clear all flags, and not only a particular one.
Later of course, we want to set and clear individual flags
independently.
NMPolicy printed
policy: connection 'a' failed to autoconnect; 1 tries left
settings-connection[0x55a485553b60,ab9f3891-3420-335e-89da-f14c1b94c540]: autoconnect: retries set 0
That is, it claimed there was one more try, when in fact there wasn't.
Our GError codes are mostly meaningless, only the message is interesting.
And our messages should anyway be unique, so that one could understand
which was the corresponding error code (by inspecting the source code).
While at it, use gs_free_error.
Note that for the
if (nm_device_state_reason_check (reason) == NM_DEVICE_STATE_REASON_NO_SECRETS)
case we no longer do the
if (nm_settings_connection_autoconnect_retries_get (connection) == 0)
check. But that is fine, because we only skip schedling a reset_connections_retries()
action. But note, that that previously we also would never actually
scheudle a new timeout, because
- either nm_settings_connection_autoconnect_retries_get (connection) != 0
- or the retries count was zero, in which case we already have a
reset_connections_retries action pending (from the time when we
set it to zero.
So, there is no change in behavior at all except dropping of a redundant
logging line.
NMSettingsConnection has 3 properties that are related to autoconnect:
- autoconnect_retries
- autoconnect_blocked_until
- autoconnect_blocked_reason
autoconnect_blocked_reason is entirely independent from the other two.
A connection have have autoconnect blocked via a blocked-reason, but the
retry count is not affected by that. The retry count is an independent
mechanism, that may additionally prevent autoconnect.
However autoconnect_retries and autoconnect_retries_blocked_until are
strongly related. The latter is set if and only if autoconnect_retries is
at zero.
Rename to reflect that better.
Using CList, we embed the list element in NMActiveConnection struct
itself. That means for example, that you couldn't track a
NMActiveConnection more then once. But we anyway never want that.
The advantage is, that removing an active connection from the list
is O(1), and we safe additional GSlice allocations for each node
element.
We also do this for libnm, where it causes visible changes
in behavior. But if somebody would rely on the hashing implementation
for hash tables, it would be seriously flawed.
When a master connection is deactivated by user, we set the
autoconnect-blocked reason 'user-request' for the connection and we
propagate the same reason to slaves. Doing so prevents the
autoactivation of slaves when the master is manually activated again,
because the only way to override the 'user-request' blocked reason is
through manual activation of slaves.
Instead what should happen is that the manual deactivation of a master
marks slaves as blocked for failed dependencies. When the master
becomes available again, slaves can autoactivate if the profile allows
it.
https://bugzilla.redhat.com/show_bug.cgi?id=1437598
This was added by commit 979b8920b4
(core: move virtual device autoconnect tracking bits out of NMManager)
to avoid autoconnecting software devices repeatedly. That was done,
because disconnecting a software device would delete the NMDevice
instance, and there is no property on a device to prevent autoconnect.
In the meantime, we only unrealize software devices and don't delete
them entirely. Also, the autoconnect-blocked flags of the device are
preserved when the device unrealized.
It was anyway odd, that deactivating one software-device would block
autoconnection for all matching connections.
The flags allow for more then two reasons. Currently the only reasons
for allowing or disallowing autoconnect are "user" and "intern".
It's a bit odd, that NMDeviceAutoconnectBlockedFlags has a negative
meaning. So
nm_device_set_autoconnect_intern (device, FALSE);
gets replaced by
nm_device_set_autoconnect_blocked_set (device, NM_DEVICE_AUTOCONNECT_BLOCKED_INTERN);
and so on.
However, it's chosen this way, because autoconnect shall be allowed,
unless any blocked-reason is set. That is, to check whether autoconnect
is allowed, we do
if (!nm_device_get_autoconnect_blocked (device, NM_DEVICE_AUTOCONNECT_BLOCKED_ALL))
The alternative check would be
if (nm_device_get_autoconnect_allowed (device, NM_DEVICE_AUTOCONNECT_ALLOWED_ALL) == NM_DEVICE_AUTOCONNECT_ALLOWED_ALL)
which seems odd too.
So, add the inverse flags to block autoconnect.
Beside refactoring and inverting the meaning of the autoconnect
settings, there is no change in behavior.
Only NMPolicy should be concerned with handling autoconnect, and
blocking it.
Move the code. Note that there is a slight possible change in
behavior, as the order of when the connection is blocked changes,
based on the different times when the device changed signal gets
executed. But that shouldn't be a problem.
Names like
- nm_settings_connection_get_autoconnect_retries
- nm_settings_connection_set_autoconnect_retries
- nm_settings_connection_reset_autoconnect_retries
are about the same thing, but they are cumbersome to grep
because they share not a common prefix.
Rename them from SUBJECT_VERB_OBJECT to SUBJECT_OBJECT_VERB,
which sounds odd in English, but seems preferred to me.
Now you can grep for "nm_settings_connection_autoconnect_retries_" to
get all accessors of the retry count, or "nm_settings_connection_autoconnect_"
to get all accessors related to autoconnect in general.
- clearify in the manual page that setting retry to 1 means to try
once, without retry.
- log the initially set retry value in nm_settings_connection_get_autoconnect_retries().
- use nm_settings_connection_get_autoconnect_retries() in
nm_settings_connection_can_autoconnect().
In case the connection is blocked because it failed, the availability
of a master is a good reason to unblock it so that it can be tried
again.
Fixes: a1ea422aad
Distinguish between connections blocked from autoconnecting by user
request and connections blocked because they failed (and would fail
again).
Later, the reason will be used to unblock failed connection when some
conditions change.
The return value of g_hash_table_add() was added in GLib 2.40, use the
wrapper to avoid compile error on older versions:
src/nm-policy.c: In function ‘auto_activate_device’:
src/nm-policy.c:1279:7: error: void value not ignored as it ought to be
Fixes: a1ea422aad
When a connection is autoactivated NMPolicy only detects a failure by
watching the device state, or when the activation fails immediately.
If the activation fails after the asynchronus authorization check
before the device enters the PREPARE state, no other connection is
tried.
Let NMPolicy watch the active-connection state to detect early
failures and disconnect the signal handler when we detect that the
device state is progressing.
https://bugzilla.redhat.com/show_bug.cgi?id=1310676
Remove NMDefaultRouteManager. Instead, add the default-route to the
NMIP4Config/NMIP6Config instance.
This basically reverts commit e8824f6a52.
We added NMDefaultRouteManager because we used the corresponding to `ip
route replace` when configuring routes. That would replace default-routes
on other interfaces so we needed a central manager to coordinate routes.
Now, we use the corresponding of `ip route append` to configure routes,
and each interface can configure routes indepdentently.
In NMDevice, when creating the default-route, ignore @auto_method for
external devices. We shall not touch these devices.
Especially the code in NMPolicy regarding selection of the best-device
seems wrong. It probably needs further adjustments in the future.
Especially get_best_ip_config() should be replaced, because this
distinction VPN vs. devices seems wrong to me.
Thereby, remove the @ignore_never_default argument. It was added by
commit bb75026004, I don't think it's
needed anymore.
This brings another change. Now that we track default-routes in
NMIP4Config/NMIP6Config, they are also exposed on D-Bus like regular
routes. I think that makes sense, but it is a change in behavior, as
previously such routes were not exposed there.
Don't rely on manager keeping them alive long enough. E.g.
get-best-device() is used when resetting the best device,
however, it accesses the current device (hence, it relies
on manager removing the device from the list, but keeping
it alive long enough).
We already track the best device as priv->default_device4 / priv->default_device6.
Don't try to look it up again. If the cached values from @priv are invalid/outdated,
that should be fixed instead.
This was already introduced by commit 773c006a4c.
But I don't think it should be done.
Reasons:
- it adds an O(1) lookup index for accessing NMIPxConfig's addresses.
Hence, operations like merge/intersect have now runtime O(n) instead
of O(n^2).
Arguably, we expect low numbers of addresses in general. For low
numbers, the O(n^2) doesn't matter and quite likely in those cases
the previous implementation was just fine -- maybe even faster.
But the simple case works fine either way. It's important to scale
well in the exceptional case.
- the tracked objects can be shared between the various NMPI4Config,
NMIP6Config instances with NMPlatform and everybody else.
- the NMPObject can be treated generically, meaning it enables code to
handle both IPv4 and IPv6, or addresses and routes. See for example
_nm_ip_config_add_obj().
- I want core to evolve to somewhere where we don't keep copies of
NMPlatformIP4Address, et al. instances. Instead they shall all be
shared. I hope this will reduce memory consumption (although tracking a
reference consumes some memory too). Also, it shortcuts nmp_object_equal()
when comparing the same object. Calling nmp_object_equal() on the
identical objects would be a common case after the hash function
pre-evaluates equality.
It serves no purpose, because the function always allocates a new
result and returns it. It would make sense, if the output string
would only be cloned when we need to allocate a new string. But
since that optimization is not done, the argument serves no purpose.
