The values cached in the device may be stale when we start a new
activation because in a disconnected state we might have called
ip_config_merge_and_apply() which cached the main table value.
We no longer add these. If you use Emacs, configure it yourself.
Also, due to our "smart-tab" usage the editor anyway does a subpar
job handling our tabs. However, on the upside every user can choose
whatever tab-width he/she prefers. If "smart-tabs" are used properly
(like we do), every tab-width will work.
No manual changes, just ran commands:
F=($(git grep -l -e '-\*-'))
sed '1 { /\/\* *-\*- *[mM]ode.*\*\/$/d }' -i "${F[@]}"
sed '1,4 { /^\(#\|--\|dnl\) *-\*- [mM]ode/d }' -i "${F[@]}"
Check remaining lines with:
git grep -e '-\*-'
The ultimate purpose of this is to cleanup our files and eventually use
SPDX license identifiers. For that, first get rid of the boilerplate lines.
While at it, rename the "addr" field to "l_address". The term "addr" is
used over and over. Instead we should use distinct names that make it
easier to navigate the code.
When changing the number of VFs the kernel can block for very long
time in the write() to sysfs, especially if autoprobe-drivers is
enabled. Turn the nm_platform_link_set_sriov_params() into an
asynchronous function.
Remove the call-id from the requests hash before invoking the callback.
This prevents the user to cancel the request from within the callback.
Supporting such a use case is not necessary so prevent it and tighten
the callers up.
A guint value can wrap, so we would need to check that we don't allocate duplicate
IDs (which we currently don't, and it's likely never to actually hit).
Just expose the (opaque) pointer of the call-id.
We still keep a "request_id", but that is only for logging purpose.
If we created a software interface it is because we already decided
that it should be managed, and so there is no point in waiting udev to
check that the interface is not udev-unmanaged.
We still wait udev for software interfaces created externally.
- use GDBusConnection instead of GDBusProxy.
- rename "call-id" to "conf-id". It's really not a "call" but
configuration that gets added and NMPacrunnerManager ensures that
the configuration is send to pacrunner.
- let "conf-id" keep a reference to NMPacrunnerManager. For one,
when we remove configurations we need to call DestroyProxyConfiguration
to remove it again. We cannot just abort the requests but must linger
around until our configuration is properly cleaned up. Hence, we
anyway cannot destroy the NMPacrunnerManager earlier.
With respect to fixing shutdown not to leak anything, this merely
means that we must wait (and iterate the main loop) as long as
NMPacrunnerManager singleton still exits (that is anyway the plan
how to fix shutdown).
With these considerations it's also clear that our D-Bus calls must
have a stricter timeout: NM_SHUTDOWN_TIMEOUT_MS.
This is also nice because nm_pacrunner_manager_remove() no longer
needs a manager parameter, it can just rely on having a reference
to the manager.
- for logging the configuration IDs, don't log pointer values.
Logging pointer values should be avoided as it defeats ASLR.
Instead, give them a "log_id" number.
- pacrunner is a D-Bus activatable service. D-Bus activatable services
needs special care. We don't want to start it over and over again.
Instead, we only try to "StartServiceByName" if
- we have any configuration to add
- if pacrunner is currently confirmed not to be running (by watching
name owner changes)
- we didn't try to start it already. That means, only start it
at the beginning and afterwards set a flag to block it. When
we see pacrunner appear on D-Bus we always clear that flag,
that means if pacrunner drops of, we will try to restart it
(once).
Consider the situation in which ipv4.method=auto and there is an
address configured. Also, the DHCP timeout is long and there is no
DHCP server. If the link is brought down temporarily, the prefix route
for the static address is lost and not restored by NM because we
reapply the IP configuration only when the IP state is DONE.
The same can happen also for IPv6, but in that case also static IPv6
addresses are lost.
We should always reapply the IP configuration when the link goes up.
In general, all fields of public NMPlatform* structs must be
plain/simple. Meaning: copying the struct must be possible without
caring about cloning/duplicating memory.
In other words, if there are fields which lifetime is limited,
then these fields cannot be inside the public part NMPlatform*.
That is why
- "NMPlatformLink.kind", "NMPlatformQdisc.kind", "NMPlatformTfilter.kind"
are set by platform code to an interned string (g_intern_string())
that has a static lifetime.
- the "ingress_qos_map" field is inside the ref-counted struct NMPObjectLnkVlan
and not NMPlatformLnkVlan. This field requires managing the lifetime
of the array and NMPlatformLnkVlan cannot provide that.
See also for example NMPClass.cmd_obj_copy() which can deep-copy an object.
But this is only suitable for fields in NMPObject*. The purpose of this
rule is that you always can safely copy a NMPlatform* struct without
worrying about the ownership and lifetime of the fields (the field's
lifetime is unlimited).
This rule and managing of resource lifetime is the main reason for the
NMPlatform*/NMPObject* split. NMPlatform* structs simply have no mechanism
for copying/releasing fields, that is why the NMPObject* counterpart exists
(which is ref-counted and has a copy and destructor function).
This is violated in tc_commit() for the "kind" strings. The lifetime
of these strings is tied to the setting instance.
We cannot intern the strings (because these are arbitrary strings
and interned strings are leaked indefinitely). We also cannot g_strdup()
the strings, because NMPlatform* is not supposed to own strings.
So, just add comments that warn about this ugliness.
The more correct solution would be to move the "kind" fields inside
NMPObjectQdisc and NMPObjectTfilter, but that is a lot of extra effort.
While nm_platform_link_get_ifindex() is documented to return 0 if the device
is not found, don't rely on it. Instead, check that a valid(!) ifindex was
returned, and only then set the ifindex. Otherwise leave it at zero. There
is of course no difference in practice, but we generally treat invalid ifindexes
as <= 0, so it's not immediately clear what nm_platform_link_get_ifindex()
returns to signal no device.
Kernel calls the netlink attribute TCA_FQ_CODEL_MEMORY_LIMIT. Likewise,
iproute2 calls this "memory_limit".
Rename because TC parameters are inherrently tied to the kernel
implementation and we should use the familiar name.
iproute2 uses the special value ~0u to indicate not to set
TCA_FQ_CODEL_CE_THRESHOLD in RTM_NEWQDISC. When not explicitly
setting the value, kernel treats the threshold as disabled.
However note that 0xFFFFFFFFu is not an invalid threshold (as far as
kernel is concerned). Thus, we should not use that as value to indicate
that the value is unset. Note that iproute2 uses the special value ~0u
only internally thereby making it impossible to set the threshold to
0xFFFFFFFFu). But kernel does not have this limitation.
Maybe the cleanest way would be to add another field to NMPlatformQDisc:
guint32 ce_threshold;
bool ce_threshold_set:1;
that indicates whether the threshold is enable or not.
But note that kernel does:
static void codel_params_init(struct codel_params *params)
{
...
params->ce_threshold = CODEL_DISABLED_THRESHOLD;
static int fq_codel_change(struct Qdisc *sch, struct nlattr *opt,
struct netlink_ext_ack *extack)
{
...
if (tb[TCA_FQ_CODEL_CE_THRESHOLD]) {
u64 val = nla_get_u32(tb[TCA_FQ_CODEL_CE_THRESHOLD]);
q->cparams.ce_threshold = (val * NSEC_PER_USEC) >> CODEL_SHIFT;
}
static int fq_codel_dump(struct Qdisc *sch, struct sk_buff *skb)
{
...
if (q->cparams.ce_threshold != CODEL_DISABLED_THRESHOLD &&
nla_put_u32(skb, TCA_FQ_CODEL_CE_THRESHOLD,
codel_time_to_us(q->cparams.ce_threshold)))
goto nla_put_failure;
This means, kernel internally uses the special value 0x83126E97u to indicate
that the threshold is disabled (WTF). That is because
(((guint64) 0x83126E97u) * NSEC_PER_USEC) >> CODEL_SHIFT == CODEL_DISABLED_THRESHOLD
So in kernel API this value is reserved (and has a special meaning
to indicate that the threshold is disabled). So, instead of adding a
ce_threshold_set flag, use the same value that kernel anyway uses.
The memory-limit is an unsigned integer. It is ugly (if not wrong) to compare unsigned
values with "-1". When comparing with the default value we must also use an u32 type.
Instead add a define NM_PLATFORM_FQ_CODEL_MEMORY_LIMIT_UNSET.
Note that like iproute2 we treat NM_PLATFORM_FQ_CODEL_MEMORY_LIMIT_UNSET
to indicate to not set TCA_FQ_CODEL_MEMORY_LIMIT in RTM_NEWQDISC. This
special value is entirely internal to NetworkManager (or iproute2) and
kernel will then choose a default memory limit (of 32MB). So setting
NM_PLATFORM_FQ_CODEL_MEMORY_LIMIT_UNSET means to leave it to kernel to
choose a value (which then chooses 32MB).
See kernel's net/sched/sch_fq_codel.c:
static int fq_codel_init(struct Qdisc *sch, struct nlattr *opt,
struct netlink_ext_ack *extack)
{
...
q->memory_limit = 32 << 20; /* 32 MBytes */
static int fq_codel_change(struct Qdisc *sch, struct nlattr *opt,
struct netlink_ext_ack *extack)
...
if (tb[TCA_FQ_CODEL_MEMORY_LIMIT])
q->memory_limit = min(1U << 31, nla_get_u32(tb[TCA_FQ_CODEL_MEMORY_LIMIT]));
Note that not having zero as default value is problematic. In fields like
"NMPlatformIP4Route.table_coerced" and "NMPlatformRoutingRule.suppress_prefixlen_inverse"
we avoid this problem by storing a coerced value in the structure so that zero is still
the default. We don't do that here for memory-limit, so the caller must always explicitly
set the value.
Only read the keyfile databases once and cache them for the remainder of
the program.
- this avoids the overhead of opening the file over and over again.
- it also avoids the data changing without us expecting it. The state
files are internal and we don't support changing it outside of
NetworkManager. So in the base case we read the same data over
and over. In the worst case, we read different data but are not
interested in handling the changes.
- only write the file when the content changes or before exiting
(normally).
- better log what is happening.
- our state files tend to grow as we don't garbage collect old entries.
Keeping this all in memory might be problematic. However, the right
solution for this is that we come up with some form of garbage
collection so that the state files are reaonsably small to begin with.
When we set the MTU on the link we remember its previous source
(ip-config, parent-device or connection profile) and don't change it
again afterwards to avoid interfering with user's manual changes. The
only exceptions when we change it again are (1) if the parent device
MTU changes and (2) if the new MTU has higher priority than the one
previously set.
To allow a live reapply of the MTU property we also need to clear the
saved source, or the checks described above will prevent setting the
new value.
Fixes: 2f8917237f ('device: rework mtu priority handling')
https://bugzilla.redhat.com/show_bug.cgi?id=1702657
"libnm-core" implements common functionality for "NetworkManager" and
"libnm".
Note that clients like "nmcli" cannot access the internal API provided
by "libnm-core". So, if nmcli wants to do something that is also done by
"libnm-core", , "libnm", or "NetworkManager", the code would have to be
duplicated.
Instead, such code can be in "libnm-libnm-core-{intern|aux}.la".
Note that:
0) "libnm-libnm-core-intern.la" is used by libnm-core itsself.
On the other hand, "libnm-libnm-core-aux.la" is not used by
libnm-core, but provides utilities on top of it.
1) they both extend "libnm-core" with utlities that are not public
API of libnm itself. Maybe part of the code should one day become
public API of libnm. On the other hand, this is code for which
we may not want to commit to a stable interface or which we
don't want to provide as part of the API.
2) "libnm-libnm-core-intern.la" is statically linked by "libnm-core"
and thus directly available to "libnm" and "NetworkManager".
On the other hand, "libnm-libnm-core-aux.la" may be used by "libnm"
and "NetworkManager".
Both libraries may be statically linked by libnm clients (like
nmcli).
3) it must only use glib, libnm-glib-aux.la, and the public API
of libnm-core.
This is important: it must not use "libnm-core/nm-core-internal.h"
nor "libnm-core/nm-utils-private.h" so the static library is usable
by nmcli which couldn't access these.
Note that "shared/nm-meta-setting.c" is an entirely different case,
because it behaves differently depending on whether linking against
"libnm-core" or the client programs. As such, this file must be compiled
twice.
(cherry picked from commit af07ed01c0)
From the files under "shared/nm-utils" we build an internal library
that provides glib-based helper utilities.
Move the files of that basic library to a new subdirectory
"shared/nm-glib-aux" and rename the helper library "libnm-core-base.la"
to "libnm-glib-aux.la".
Reasons:
- the name "utils" is overused in our code-base. Everything's an
"utils". Give this thing a more distinct name.
- there were additional files under "shared/nm-utils", which are not
part of this internal library "libnm-utils-base.la". All the files
that are part of this library should be together in the same
directory, but files that are not, should not be there.
- the new name should better convey what this library is and what is isn't:
it's a set of utilities and helper functions that extend glib with
funcitonality that we commonly need.
There are still some files left under "shared/nm-utils". They have less
a unifying propose to be in their own directory, so I leave them there
for now. But at least they are separate from "shared/nm-glib-aux",
which has a very clear purpose.
(cherry picked from commit 80db06f768)
"shared/nm-utils" contains general purpose utility functions that only
depend on glib (and extend glib with some helper functions).
We will also add code that does not use glib, hence it would be good
if the part of "shared/nm-utils" that does not depend on glib, could be
used by these future projects.
Also, we use the term "utils" everywhere. While that covers the purpose
and content well, having everything called "nm-something-utils" is not
great. Instead, call this "nm-std-aux", inspired by "c-util/c-stdaux".
(cherry picked from commit b434b9ec07)
Next we will need to detect more kernel features. First refactor the
handling of these to require less code changes and be more efficient.
A plain nm_platform_kernel_support_get() only reqiures to access an
array in the common case.
The other important change is that the function no longer requires a
NMPlatform instance. This allows us to check kernel support from
anywhere. The only thing is that we require kernel support to be
initialized before calling this function. That means, an NMPlatform
instance must have detected support before.
(cherry picked from commit ee269b318e)
Currently, if user configuration or settings specify that a software
device is unmanaged, for example:
[device-bond-unmanaged]
match-device=interface-name:bond*
managed=0
or
[keyfile]
unmanaged-devices=interface-name:bond*
and there is a connection for the device with autoconnect=yes, NM
creates the platform link and a realized device in unmanaged
state. Fix this, the device should not be realized if it is unmanaged.
https://bugzilla.redhat.com/show_bug.cgi?id=1679230
nm_device_spec_match_list_full() calls
nm_device_get_permanent_hw_address() which freezes the MAC address, so
currently callers must avoid the function when the device is not
completely platform-initialized.
Instead, use nm_device_get_permanent_hw_address_full() to avoid
freezing the MAC when the device is not platform-initialized. In this
way nm_device_spec_match_list_full() can be called from any state
without side effects.
In general shortcutting state is a no-no. But putting a device to FAILED
state because its master is going down is a crime. It's the wrong state:
the devices should enter it when their connections themselves failed
unexpectedly, and can potentially recover with another actiation.
Otherwise bad things happen,
In particular, the devices automatically enter DISCONNECTED state and
eventually retry autoconnecting. In this case they would attempt to
bring the master back up. Ugh.
This situation happens when a topomost master of multiple levels of
master-slave relationship is deactivated.
Aside from that, shortcutting to DISCONNECTED on unknown change reason
doesn't make sense either. Like, wtf, just traverse through DEACTIVATING
like all the other kids do.
Connection defaults should correspond in range to the per-profile values.
"infiniband.mtu" is required to be not larger than 65520, so we also
need to honor that when parsing the connection default.
... and nm_acd_manager_announce_addresses().
The test will need more information to know why it may fail.
Return a NetworkManager error code, instead of a boolean.
If we surprise-remove the master, slaves would immediately attempt to bring
things up by autoconnecting. Not cool. Policy, however, blocks
autoconnect if the slaves disconnect due to "dependency-failed", and it
indeed seems to be an appropriate reason here:
$ nmcli c add type bridge
$ nmcli c add type dummy ifname dummy0 master bridge autoconnect yes
$ nmcli c del bridge
$
Before:
(nm-bridge): state change: ip-config -> deactivating (reason 'connection-removed')
(nm-bridge): state change: deactivating -> disconnected (reason 'connection-removed')
(nm-bridge): detached bridge port dummy0
(dummy0): state change: activated -> disconnected (reason 'connection-removed')
(nm-bridge): state change: disconnected -> unmanaged (reason 'user-requested')
(dummy0): state change: disconnected -> unmanaged (reason 'user-requested')
policy: auto-activating connection 'bridge-slave-dummy0'
After:
(nm-bridge): state change: ip-config -> deactivating (reason 'connection-removed')
(nm-bridge): state change: deactivating -> disconnected (reason 'connection-removed')
(nm-bridge): detached bridge port dummy0
(dummy0): state change: activated -> deactivating (reason 'dependency-failed')
(nm-bridge): state change: disconnected -> unmanaged (reason 'user-requested')
(dummy0): state change: deactivating -> disconnected (reason 'dependency-failed')
(dummy0): state change: disconnected -> unmanaged (reason 'user-requested')
https://github.com/NetworkManager/NetworkManager/pull/319
When the link goes down the kernel removes IPv6 addresses from the
interface. In update_ext_ip_config() we detect that addresses were
removed externally and drop them from various internal
configurations. Don't do that if the link is down so that those
addresses will be restored again on link up.
(cherry picked from commit 505d2adbc2)
Add a new argument to nm_ip_config_* helpers to also ignore addresses
similarly to what we already do for routes. This will be used in the
next commit; no change in behavior here.
(cherry picked from commit 39b7257208)
We can detect false DAD failures if the link goes down. Don't try to
prevent them, but just reset the counter if the link goes down.
(cherry picked from commit 056470a4ba)
When the interface is down DAD failures becomes irrelevant and we
shouldn't try to add a link-local address even if the configuration
contains other IPv6 addresses.
(cherry picked from commit 72385f363c)
The device type was set to the GType rather than a new value in the
NMDeviceType enum.
Add the corresponding enum entry, fix the device type and set the
routing priority to the same value as generic devices.
(cherry picked from commit 8d9365a973)
