Although IFA_F_TEMPORARY is numerically equal to IFA_F_SECONDARY,
their meaning is different. One applies to IPv6 temporary addresses,
and the other to IPv4 secondary addresses.
During _addr_array_clean_expired() we want to ignore and clear
IPv6 temporary addresses, but not IPv4 secondary addresses.
Fixes: f2c4720bca
While the numerical values of IFA_F_SECONDARY and IFA_F_TEMPORARY
are identical, their meaning is not.
IFA_F_SECONDARY is only relevant for IPv4 addresses, while
IFA_F_TEMPORARY is only relevant for IPv6 addresses.
IFA_F_TEMPORARY is automatically set by kernel for the addresses
that it generates as part of IFA_F_MANAGETEMPADDR. It cannot be
actively set by user-space.
IFA_F_SECONDARY is automatically set by kernel depending on the order
in which the addresses for the same subnet are added.
This essentially reverts 8b4f11927 (core: avoid IFA_F_TEMPORARY alias for
IFA_F_SECONDARY).
In device_ipx_changed() we remember the addresses for which it appears
that DAD failed. Later, on an idle handler, we process them during
queued_ip6_config_change().
Note that nm_plaform_ip6_address_sync() might very well decide to remove
some or all addresses and re-add them immidiately later. It might do so,
to get the address priority/ordering right. At that point, we already
emit platform signals that the device disappeared, and track them in
dad6_failed_addrs.
Hence, later during queued_ip6_config_change() we must check again
whether the address is really not there and not still doing DAD.
Otherwise, we wrongly claim that DAD failed and remove the address,
generate a new one, and the same issue might happen again.
dad6_failed_addrs is populated with addresses from the platform cache.
Inside the cache, all addresses have addr_source NM_IP_CONFIG_SOURCE_KERNEL,
because addr_source property for addresses is only a property that is
used NetworkManager internally.
NMPObjects are never modified after being put into the cache.
Hence, it is safe and encouraged to just keep a reference to them,
instead of cloning them.
Interestingly, NMPlatform's change signals have a platform_object
pointer, which is not the pointer to the NMPObjects itself, but
down-cast to the NMPlatformObject instance. It does so, because commonly
callers want to have a pointer to the NMPlatformObject instance, instead
of the outer NMPObjects. However, NMP_OBJECT_UP_CAST() is guaranteed
to work one would expect.
The order in which we add addresses to dad6_failed_addrs does not
matter. Hence, use g_slist_prepend() which is O(1), instead
g_slist_append() with O(n).
We want to add addresses in a particular order so that source address
selection works.
Note that @known_addresses contains the desired addresses in order of
least-important first, while @plat_addresses contains them in opposite
order. Previously, this inverted order was not considered, and we
essentially ended up removing and re-adding all addresses every time.
Fix that. While at it, get rid of the O(n^2) runtime complexity, and
make it O(n) by iterating both lists simultaneously.
Temporary addresses (RFC4941) are not handled by NetworkManager directly, but by
kernel. If they are in the @known_addresses list, clear them out early.
They shall be ignored.
Often, we want in API that an input argument is read-only and not modified
by the function call. Not modifying input arguments is a good
convention.
However, in this case there are only two callers, and both clearly do
not care whether the @known_addresses array will be modified.
Clear out addresses that are already expired and enforce that there are
no duplicate addresses. Basically, use @known_addresses for bookkeeping
which addresses are to be ignored.
We do a pre-run that constructs an index of all addresses and drops
addresses that are already expired.
Move this code to a separate function, it will be reused for IPv6.
Also, note that nm_platform_ip4_address_sync() has only 2 callers. Both
callers make sure to not pass duplicate known addresses, because the
addresses also come from a cache. Make that a requirement and assert
against unique addresses. If we would allow duplicate addresses, we would
have to handle them in a defined way (like, dropping the ones with lower
priority). That would be more complicated, and since no caller is
supposed to provide duplicate addresses, don't bother but assert.
nm_utils_lifetime_get() already has so many arguments.
Essentially, the function returned %TRUE if and only if the
lifetime was greater then zero.
Combine the return value and the output argument for the lifetime.
It also matches better the function name: to get the lifetime.
Originally, the platform cache did not preserve any stable order.
We added that during the large cache rework. However, we still
would only care about a particular ordering for route's BY_WEAK_ID
index. For all other indexes, it was sufficient to have the
object in some arbitrary order, not necessarily the one as indicated by
kernel.
However, for addresses we actually care about the order (at least,
regarding the the OBJECT_BY_IFINDEX index, which is considered by
platform's address sync).
During a dump we get all objects in the right order. That means,
as we (re) insert the objects into the cache, we must forcefully move
them to the end of their list.
If the object didn't actually change, previously we would not have
updated their position in the cache. Fix that now.
Add a function that allows to re-request all objects of a certain type.
Usually, the cache is supposed to keep itself in a consistent state and
this function is not useful.
It is however useful during testing and debugging to explicitly reload
an object type.
If you ever think to need this function in non-testing code, then
something else is probably wrong with the cache implementation.
In ndisc_set_router_config(), we initialize NMNDiscAddress based on
NMPlatformIP6Address instances. Note that their handling of timestamps
is not entirely identical.
For convenience of the user, NMPlatformIP6Address allows to not specify
any timestamp. On the contrary, for convenience of implementation does
NMNDiscAddress always require fully specified timestamps.
Properly convert one representation into the other.
Previously we always updated resolv.conf on quit. When we are using
systemd-resolved the update is not necessary because the resolver on
127.0.0.53 would still be reachable after NM quits. Also, when NM
manages resolv.conf directly there is no need to update the file
again. Let's rewrite resolv.conf only when using dnsmasq.
https://bugzilla.redhat.com/show_bug.cgi?id=1541031
This makes its prototype compatible with GDestroyNotify so that GCC 8.0
won't warn.
The return value is not used anywhere and the unref() functions typically
don't return any.
Fixes the following error when building with gcc 4.8.5 and address
sanitizer:
src/dns/nm-dns-dnsmasq.c: In function 'update':
src/dns/nm-dns-dnsmasq.c:506:44: error: 'first_prio' may be used uninitialized in this function [-Werror=maybe-uninitialized]
} else if (first_prio < 0 && first_prio != prio)
^
Print the "tentative" flags as last. Most other flags, have more the character of
a user configured attribute, while "tentative" reflects the current state of the address.
Previously, we would log
secondary,tentative
and
tentative,mngtmpaddr,noprefixroute
Print the "tenative" flag last. This way, the flag that commonly
will flip by kernel's decision, is consistently printed last.
Previously, we would directly log get_expiry(), which is the absolute timestamp
inn nm_utils_get_monotonic_timestamp_s() scale. This time scale starts counting
somewhere around the time when the NetworkManager process starts, hence it is not
very intuitive to look at.
Instead, print the remaining time that is left counting from now. Since
we anyway only track timeouts with a granularity of whole seconds,
printing up to 4 decimal places is sufficiently precise.
We commonly only allow tabs at the beginning of a line, not
afterwards. The reason for this style is so that the code
looks formated right with tabstop=4 and tabstop=8.
I think we should avoid non-trailing tabs in source code.
Allowing unescaped tab characters in string literals, adds
noise when searching the code for non-trailing tabs.
Also, depending on the editor configuration, it might be
non-obvious that tabs are used. And while I dislike tabs in general,
I think they are especially bad, when they have actual meaning
in code.
Setting the state of NMActiveConnection results in invoking callbacks
in NMManager. Hence, it might be far-reaching. Clear
priv->queued_act_request before invoking the callbacks.
There is a small change in behavior:
Previously, the DEACTIVATING/DEACTIVATED states were set if and only if
the previous state was less or equal then ACTIVATED. For example,
if the state was already DEACTIVATING, it would have done nothing.
Now, nm_active_connection_set_state_fail() transitions the states
depending on the previous state. E.g. it would only set DEACTIVATING
state, if the previous state was ACTIVATING/ACTIVATED. On the other hand,
it would always progress the state to DEACTIVATED.
The new behavior makes more sense to me, although I doubt that there is
a visible difference.
unmanaged_to_disconnected() is supposed to mark the device as managed.
However, it may easily be unable to do so, for example if the device
is unmanaged by NM_UNMANAGED_USER_SETTINGS.
Shortly before actually enqueuing the activation request, check and
error out. Otherwise, we might hit an assertion later in
_device_activate().
Note how recheck_assume_connection() called:
nm_exported_object_export (NM_EXPORTED_OBJECT (active));
active_connection_add (self, active);
nm_device_queue_activation (device, NM_ACT_REQUEST (active));
That differs from the order during _internal_activate_generic(), where
we would end up with:
nm_exported_object_export (NM_EXPORTED_OBJECT (active));
nm_device_queue_activation (device, NM_ACT_REQUEST (active));
active_connection_add (self, active);
It makes more sense to me to *first* add the connection, and only then
starting the activation with nm_device_queue_activation().
Also, let active_connection_add() always export the new active
connection object, if it is not already exported. All callers of
active_connection_add() ensured that the new object is already
exported.
NM_DEVICE_OVS_INTERFACE_GET_PRIVATE() is implemented via the _NM_GET_PRIVATE()
macro. This macro uses C11's _Generic() to provide additional compiler checks
when casting from an incompatible pointer type.
As such,
NMDevice *device = ...;
NMDeviceOvsInterfacePrivate *priv;
priv = NM_DEVICE_OVS_INTERFACE_GET_PRIVATE (device);
causes a compilation error:
error: ‘_Generic’ selector of type ‘NMDevice * {aka struct _NMDevice *}’ is not compatible with any association
One workaround would be to cast the pointer first:
priv = NM_DEVICE_OVS_INTERFACE_GET_PRIVATE ((NMDeviceOvsInterface *) device);
A better fix is to mark NMDevice as a compatible pointer in _NM_GET_PRIVATE(),
which this patch does.
Previously, this went unnoticed, because due to bug "a43bf3388 build: fix configure
check for CC support of _Generic() and __auto_type", we failed to detect support
for _Generic() when compiling with -Werror. That essentially disables this check,
and NM_DEVICE_OVS_INTERFACE_GET_PRIVATE() would do a direct cast.
A workaround for this build failure might be to build with -Werror, which accidentally
results in not using _Generic().
https://bugzilla.gnome.org/show_bug.cgi?id=793183
Fixes: 8ad310f8e3
OvsInterface can postpone the stage3_ip[46]_config until the link
actually appears. It ought to restart the stage only when the link
appears, not upon further changes to it (which would trip an assertion
when starting the DHCP client while one already exists).
https://bugzilla.redhat.com/show_bug.cgi?id=1540063
Must always initialize cleanup variable, to be able to build with
"-fexceptions".
make[2]: Entering directory './contrib/fedora/rpm/NetworkManager.20180124-060444.C5tHCi/BUILD/NetworkManager-1.11.1'
CC src/devices/wifi/src_devices_wifi_libnm_device_plugin_wifi_la-nm-device-iwd.lo
In file included from ./shared/nm-utils/nm-glib.h:27:0,
from ./shared/nm-utils/nm-macros-internal.h:60,
from ./shared/nm-default.h:257,
from src/devices/wifi/nm-device-iwd.c:21:
src/devices/wifi/nm-device-iwd.c: In function ‘deactivate_async_finish’:
./shared/nm-utils/gsystem-local-alloc.h:37:8: error: ‘variant’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
if (*(Type*)v) \
^
src/devices/wifi/nm-device-iwd.c:405:29: note: ‘variant’ was declared here
gs_unref_variant GVariant *variant;
^~~~~~~
Fixes: d0c1e1a62a
XXX was used to either raise attention (NOTE) or to indicate
that this is ugly code that should be fixed (FIXME). The usage
was inconsistent.
Let's avoid XXX and use either NOTE or FIXME.
We can disable/enable configuration snippets per NetworkManager
version. But we must compare it against the current version
that we build, not the current API version.
In a previous patch I added deactivate_async to make sure that NM
auto re-connect waits for the IWD state to changed from "disconnecting"
to "disconnected" before starting a new activation when the user wants
to switch from one profile to another. This doesn't account for when
IWD itself goes into "disconnecting" because of a connect failure.
When IWD goes into the "disconnecting" state we call
nm_device_state_changed (NM_DEVICE_STATE_FAILED,
NM_DEVICE_STATE_REASON_SUPPLICANT_DISCONNECT)
immediately to give feedback to user as soon as possible. We will
return FALSE from get_autoconnect_allowed for the period the
"disconnecting" state.
