OVS system interfaces can start to connect even before the ovsdb is
ready. However, the connection attempt is doomed to fail and the
NMSettingsConnection gets blocked with reason FAILED.
Unblock them once the ovsdb is ready.
Ideally, NMPolicy should subscribe to the NMOvsdb::ready signal,
however NMOvsdb is in a plugin, so it's easier if NMOvsdb directly
calls a function of the core.
These typedefs are defined by some libc headers, and we drag
them in by including some other standard headers.
It's not clear which headers we exactly need for them, and that
all libcs provide them the same.
Instead, just avoid them.
When a connection is reactivated, there could be two active
connections tracked by the manager: the deactivating one and the new
one. Ensure that we first return the most recent one so that slaves
will pick the right master.
Fixes-test: @iptunnel_gretap_doc_procedure
Fixes: dc6ec6ce7b ('core: reverse the order of active connections in the manager')
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/697
When a new active connection is created, it gets added at the
beginning of manager's list. This means that the list contains most
recently activated connections first. Since the list is doubly-linked,
it is possible to efficiently iterate in both directions, so the order
of the list is mostly a matter of convention.
I think it is preferable to have oldest active connections at the
beginning of the list; let's reverse the order.
In most places where the list is iterated, the order doesn't
matter. Where it does, use the *_prev() variant to maintain the old
iteration order.
If an NMSettingsConnection with the VOLATILE or EXTENRAL flags is created
and passed to nm_manager_activate_connection, it's immediately scheduled
for deletion in an idle callback and will likely be deleted before the
authorization step in nm_manager_activate_connection finishes and the
connection will be aborted. This is because there's no
NMActiveConnection in priv->active_connection_lst_head referencing it
until _internal_activate_device(). Change
active_connection_find_by_connection to also look for connections in
priv->async_op_lst_head.
New _delete_volatile_connection_do() calls are added. Previously it
would be called when an active connection may have been removed from
priv->active_connection_lst_head, now also call it when an active
connection may have been removed from priv->async_op_lst_head without
being added to priv->active_connection_lst_head.
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/671
nm_auth_chain_new_subject() cannot return %NULL, so these checks are only
noise. Also, there are already calls that correctly rely on the fact that
this function cannot fail.
We use glib, where memory allocation by definition cannot fail. That means,
a lot of functions simply cannot fail in our code base. This is a very nice
property (to have an functions that cannot fail), so don't add error
checking that is not useful.
This results in the args of 'nm_utils_user_data_unpack'
containing random data potentially also from the
previous stack-frame which is really really bad.
Signed-off-by: Antonio Cardace <acardace@redhat.com>
Fixes: b50702775f ('device: implement auth-request as async operation nm_manager_device_auth_request()')
Run:
./contrib/scripts/nm-code-format.sh -i
./contrib/scripts/nm-code-format.sh -i
Yes, it needs to run twice because the first run doesn't yet produce the
final result.
Signed-off-by: Antonio Cardace <acardace@redhat.com>
A profile can configure "connection.wait-device-timeout" to indicate
that startup complete is blocked until a suitable device around.
This is useful for NetworkManager-wait-online and initrd mode.
Previously, we looked at NMPlatform whether a link with matching
interface-name was present. That is wrong because it cannot handle
profiles that rely on "ethernet.mac-address" setting or other "match"
settings. Also, the mere presence of the link does not yet mean
that the NMDevice was created and ready. In fact, there is a race here:
NMPlatform indicates that the device is ready (unblocking NMSettings),
but there is no corresponding NMDevice yet which keeps NetworkManager
busy to block startup complete.
Rework this. Now, only check whether there is a compatible device for
the profile.
Since we wait for compatible devices, it works now not only for the
interface name. Note that we do some optimizations so that we don't have
to re-evaluate all profiles (w.r.t. all devices) whenever something on the
device changes: we only care about this when all devices finally become
ready.
Also, we no longer start the timeout for "connection.wait-device-timeout"
when the profile appears. Instead, there is one system-wide start time
(NMSettingsPrivate.startup_complete_start_timestamp_msec). That simplifies
code and makes sense: we start waiting when NetworkManager is starting, not
when the profile gets added. Also, we wait for all profiles to become
ready together.
NMSettings needs access to the list of all devices, which is tracked
by NMManager. Of course, this ties NMSettings and NMManager closer
together. Note that NMManager already owns a reference to NMSettings,
so they are in fact related.
The alternatives of just letting NMSettings reference NMManager (and
vice versa) would be more complicated, and likely not help to simplify
the code (on the contrary).
If the device state change (to disconnected or unmanaged) triggered by
a sleep event happens after the wake, the devices becomes wrongly
unmanaged and it's necessary to manually manage it again, or restart
NM.
During the wake event we should disconnect the device_sleep_cb()
callback for all devices because we don't want to react to state
changes anymore; in particular we don't need to detect when the device
becomes disconnected to unmanage it.
Userspace cannot add IPv6 routes with metric 0. Trying to do that, will
be coerced by kernel to route metric 1024. For IPv4 this is different,
and metric zero is commonly allowed.
However, kernel itself can add IPv6 routes with metric zero:
# ip -6 route show table local
local fe80::2029:c7ff:fec9:698a dev v proto kernel metric 0 pref medium
That means, we must not treat route metric zero special for most cases.
Only, when we want to add routes (based on user configuration), we must
coerce a route metric of zero to 1024.
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/563
The commit breaks many nmstate CI tests. It also breaks the
autoconnect-slaves functionality: if the master gets reactivated and
the slave was active, the slave is not reconnected.
A different solution is needed for the original issue.
This reverts commit 024e983c8e.
Autoconnect-slaves currently forces an activation of all slaves, even
if there is already an active connection for them. This is bad because
at boot slaves first try to autoconnect, then the autoconnect-slaves
of the master kicks in and disconnects/reactivates them.
The only reason why the forceful reactivation was added was to fix
[1]; in that scenario, a slave connection is already active as
non-slave; then it is updated to be a slave; later, the master with
autoconnect-slaves is manually activated. NetworkManager should detect
that the slave connection must now be activated by autoconnect-slaves.
Add a specific check for such situation, instead of always
reactivating all slaves.
[1] https://bugzilla.redhat.com/show_bug.cgi?id=1845018
Fixes: 4985ca5ada ('manager: allow autoconnect-slaves to reconnect the same connection')
When a device is not marked as unmanaged, but also not actively managed
by NetworkManager, then NetworkManager will generate an in-memory
profile to represent the active state, if the device is up and
configured (with an IP address).
Such profiles are commonly named like "eth0", and they are utterly
confusing to users, because they look as if NetworkManager actually
manages the device, when it really just shows that somebody else configures
the device.
We should express this better in the UI, hence add flags to indicate
that.
In practice, such profiles are UNSAVED, NM_GENERATED, and VOLATILE. But
add an explicit flag to represent that.
https://bugzilla.redhat.com/show_bug.cgi?id=1816202
../src/nm-manager.c: In function periodic_update_active_connection_timestamps:
../src/nm-manager.c:7358:43: error: t may be used uninitialized in this function [-Werror=maybe-uninitialized]
nm_settings_connection_update_timestamp (nm_active_connection_get_settings_connection (ac),
^
Fixes: 6f3ae8a563 ('core: in periodic_update_active_connection_timestamps() use same timestamp')
We don't need the mask argument. If the caller wants to check only for certain
flags, she can do that right away with
NM_FLAGS_ANY (nm_settings_connection_autoconnect_blocked_reason_get (sett_con), flags)
When performing a synchronous action together (like iterating
over all settings and set the current timestamp), it's nicer
to pretend that all this would happen instantaneously. That means,
ensure we use the same timestamp throughout.
On a minor point, there really is no need to call time() multiple times.
lgtm.com warns:
int nm_owned:3;
>> Bit field nm_owned of type int should have explicitly unsigned integral, explicitly signed integral, or enumeration type.
So make it a NMTernary instead. It's nicer anyway.
GObject signals only complicate the code and are less efficient.
Also, NM_DEVICE_AUTH_REQUEST signal really invoked an asynchronous
request. Of course, fundamentally emitting a signal *is* the same as
calling a method. However, implementing this as signal is really not
nice nor best practice. For one, there is a (negligible) overhead emitting
a GObject signal. But what is worse, GObject signals are not as strongly
typed and make it harder to understand what happens.
The signal had the appearance of providing some special decoupling of
NMDevice and NMManager. Of course, in practice, they were not more
decoupled (both forms are the same in nature), but it was harder to
understand how they work together.
Add and call a method nm_manager_device_auth_request() instead. This
has the notion of invoking an asynchronous method. Also, never invoke
the callback synchronously and provide a cancellable. Like every asynchronous
operation, it *must* be cancellable, and callers should make sure to
provide a mechanism to abort.
g_clear_pointer() would always cast the destroy notify function
pointer to GDestroyNotify. That means, it lost some type safety, like
GPtrArray *ptr_arr = ...
g_clear_pointer (&ptr_arr, g_array_unref);
Since glib 2.58 ([1]), g_clear_pointer() is also more type safe. But
this is not used by NetworkManager, because we don't set
GLIB_VERSION_MIN_REQUIRED to 2.58.
[1] f9a9902aac
We have nm_clear_pointer() to avoid this issue for a long time (pre
1.12.0). Possibly we should redefine in our source tree g_clear_pointer()
as nm_clear_pointer(). However, I don't like to patch glib functions
with our own variant. Arguably, we do patch g_clear_error() in
such a manner. But there the point is to make the function inlinable.
Also, nm_clear_pointer() returns a boolean that indicates whether
anything was cleared. That is sometimes useful. I think we should
just consistently use nm_clear_pointer() instead, which does always
the preferable thing.
Replace:
sed 's/\<g_clear_pointer *(\([^;]*\), *\([a-z_A-Z0-9]\+\) *)/nm_clear_pointer (\1, \2)/g' $(git grep -l g_clear_pointer) -i
Otherwise, we only prune unused files when the service terminates.
Usually, NetworkManager service doesn't get restarted before shutdown
of the system (nor should it be). That means, if you create (and
destroy) a large number of software devices, the state files pile
up.
From time to time, go through the files on disk and delete those that
are no longer relevant.
In this case, "from time to time" means after we write/update state
files 100 times.
nm_manager_write_device_state() writes the device state to a file. The ifindex
is here important, because that is the identifier for the device and is also
used as file name. Return the ifindex that was used, instead of letting the
caller reimplement the knowledge which ifindex was used.
The advantage is that the API is now the same for IPv4 and IPv6: it's
all nm_dhcp_config_*() and we can (easier) treat the address family
generically.
We still need two distinct GObject types, mainly because of the
glue code for exposing the object on D-Bus as NMDBusObject. Of course,
that could be solved differently, but as it is, it's quite nice.
When AddConnection() or Update() terminate, the (unrealized) virtual
device should be already be available, otherwise an activation attempt
of that connection can fail.
https://bugzilla.redhat.com/show_bug.cgi?id=1804350
This reverts commit c163207b07.
It is redundant. Drop it.
Also, we don't need to cancel and reschedule the idle source in
nm_manager_start(). First of all, it really shouldn't be pending.
Second, contrary to a timer, when an idle source is already pending,
then the pending one will run at the right time.
The _GET_PRIVATE() macros are all implemented based on
_NM_GET_PRIVATE(). That macro tries to be more type safe and uses
_Generic() to do the right thing. Explicitly casting is not only
unnecessary, it defeats these (static) type checks.
Don't do that.
Several macros are used to define function. They had a "_STATIC" variant,
to define the function as static.
I think those macros should not try to abstract entirely what they do.
They should not accept the function scope as argument (or have two
variants per scope). This also because it might make sense to add
additional __attribute__(()) to the function. That only works, if
the macro does not pretend to *not* define a plain function.
Instead, embrace what the function does and let the users place the
function scope as they see fit.
This also follows what is already done with
static NM_CACHED_QUARK_FCN ("autoconnect-root", autoconnect_root_quark)
Move it to shared as it's useful for clients as well.
Move and rename nm_dbus_manager_new_auth_subject_from_context() and
nm_dbus_manager_new_auth_subject_from_message() in nm-dbus-manager.c
as they're needed there.
