Also, in nm_platform_routing_rule_cmp() always compare the routing
table field, also if l3mdev is set. For kernel, we cannot set table and
l3mdev together, hence such rules don't really exist (or if we try to
configure it, it will be rejected by kernel). But as far as
nm_platform_routing_rule_cmp() is concerned, if the table is set,
always compare it.
For routes and routing rules, kernel uses a certain (not stictly defined) set
of attributes to decide whether to routes/rules are identical.
That is a problem, as different kernel versions disagree on whether
two routes/rules are the same (EEXIST) or not.
Note that when NetworkManager tries to add a rule with protocol set to
anything but RTPROT_UNSPEC, then kernel will ignore the attribute if it
doesn't have support for it. Meaning: the added rule will have a
different protocol setting then intended.
Note that NMPRulesManager will add a rule if it doesn't find it in the
platform cache so far. That means, when looking into the platform cache
we must ignore or honor the protocol like kernel does.
This does not only affect FRA_PROTOCOL, but all attributes where kernel
and NetworkManager disagrees. But the protocol is the most prominent
one, because the rules tracked by nmp_rules_manager_track_default()
specify the protocol.
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.
We don't need GPtrArray to construct an array of fixed side.
Actually, we also don't need to malloc each NMPlatformBridgeVlan
element individually. Just allocate one buffer and append them
to the end.
In some cases it is convenient to specify ranges of bridge vlans, as
already supported by iproute2 and natively by kernel. With this commit
it becomes possible to add a range in this way:
nmcli connection modify eth0-slave +bridge-port.vlans "100-200 untagged"
vlan ranges can't be PVIDs because only one PVID vlan can exist.
https://bugzilla.redhat.com/show_bug.cgi?id=1652910
CC src/settings/plugins/ifcfg-rh/src_settings_plugins_ifcfg_rh_libnms_ifcfg_rh_core_la-nms-ifcfg-rh-reader.lo
In file included from ../shared/nm-default.h:280:0,
from ../src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c:21:
../src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c: In function read_routing_rules_parse:
../src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c:4309:27: error: logical not is only applied to the left hand side of comparison [-Werror=logical-not-parentheses]
nm_assert (!key_is_ipv4 == NM_STR_HAS_PREFIX (key, "ROUTING_RULE6_"));
^
../shared/nm-utils/nm-macros-internal.h:1793:7: note: in definition of macro __NM_G_BOOLEAN_EXPR_IMPL
if (expr) \
^
/usr/include/glib-2.0/glib/gmacros.h:376:43: note: in expansion of macro _G_BOOLEAN_EXPR
#define G_LIKELY(expr) (__builtin_expect (_G_BOOLEAN_EXPR((expr)), 1))
^
/usr/include/glib-2.0/glib/gtestutils.h:116:49: note: in expansion of macro G_LIKELY
if G_LIKELY (expr) ; else \
^
../shared/nm-utils/nm-macros-internal.h:973:40: note: in expansion of macro g_assert
#define nm_assert(cond) G_STMT_START { g_assert (cond); } G_STMT_END
^
../src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c:4309:3: note: in expansion of macro nm_assert
nm_assert (!key_is_ipv4 == NM_STR_HAS_PREFIX (key, "ROUTING_RULE6_"));
^
Fixes: 4d46804437
Policy routing rules are global, and unlike routes not tied to an interface by ifindex.
That means, while we take full control over all routes of an interface during a sync,
we need to consider that multiple parties can contribute to the global set of rules.
That might be muliple connection profiles providing the same rule, or rules that are added
externally by the user. NMPRulesManager mediates for that.
This is done by NMPRulesManager "tracking" rules.
Rules that are not tracked by NMPRulesManager are completely ignored (and
considered externally added).
When tracking a rule, the caller provides a track-priority. If multiple
parties track a rule, then the highest (absolute value of the) priority
wins.
If the highest track-priority is positive, NMPRulesManager will add the rule if
it's not present.
When the highest track-priority is negative, then NMPRulesManager will remove the
rule if it's present (enforce its absence).
The complicated part is, when a rule that was previously tracked becomes no
longer tracked. In that case, we need to restore the previous state.
If NetworkManager added the rule earlier, then untracking the rule
NMPRulesManager will remove the rule again (restore its previous absent
state).
By default, if NetworkManager had a negative tracking-priority and removed the
rule earlier (enforced it to be absent), then when the rule becomes no
longer tracked, NetworkManager will not restore the rule.
Consider: the user adds a rule externally, and then activates a profile that
enforces the absence of the rule (causing NetworkManager to remove it).
When deactivating the profile, by default NetworkManager will not
restore such a rule! It's unclear whether that is a good idea, but it's
also unclear why the rule is there and whether NetworkManager should
really restore it.
Add weakly tracked rules to account for that. A tracking-priority of
zero indicates such weakly tracked rules. The only difference between an untracked
rule and a weakly tracked rule is, that when NetworkManager earlier removed the
rule (due to a negative tracking-priority), it *will* restore weakly
tracked rules when the rules becomes no longer (negatively) tracked.
And it attmpts to do that only once.
Likewise, if the rule is weakly tracked and already exists when
NMPRulesManager starts posively tracking the rule, then it would not
remove again, when no longer positively tracking it.
- fix the argument type to be "gint32" and not "int".
- assert in nmp_rules_manager_track_default() for the input
arguments.
- use boolean bitfield in private data.
The name "priority" is overused. Also rules have a "priority", but that'
something else.
Rename the priority of how rules are tracked by NMPRuleManager to
"track_priority".
All that setting track-default does, is calling nmp_rules_manager_track_default()
when the rules are first accessed.
That is not right API. Since nmp_rules_manager_track_default() is already public
API (good), every caller that wishes this behavior should track these routes explicitly.
We already have code that parses exactly this kinds of string:
nm_utils_parse_inaddr_prefix_bin(). Use it.
Also, it doesn't use g_strsplit_set() to separate a string at the first
'/'. Total overkill.
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.
- use nm_utils_strsplit_set_full() instead of g_strsplit_set() to avoid allocating
a full strv array.
- refactor the code to return early and use cleanup attribute for freeing
memory.
- return TRUE/FALSE from process_dhclient_rfc3442_route(). It's simpler to
understand than returning the moved pointer and a success output variable.
The caller should make a conscious decision which delimiters to use.
Unfortunately, there is a variety of different demiters in use. This
should be unitfied and the callers should use one of a few specific
set of delimiters.
This could be unified by (re)using a define as delimiters, like
strv = nm_utils_strsplit_set_full (value, MULTILIST_WITH_ESCAPE_CHARS, NM_UTILS_STRSPLIT_SET_FLAGS_ALLOW_ESCAPING);
where MULTILIST_WITH_ESCAPE_CHARS has a particular meaning that should
be reused for similar uses.
However, leaving the delimiter at NULL is not good because it's unclear who
wants that default behavior (and what the default should be). Don't allow that.
There are almost no callers that relied on this default anyway.
If NM fails to connect to teamd, it currently just sets the device
state to FAILED and waits that deactivate() is called later. However,
the 5 seconds timeout on teamd process start can hit in the meantime,
which fails with an assertion "nm_device_is_activating (device)".
Clean up the device state when the connection to teamd fails.
https://bugzilla.redhat.com/show_bug.cgi?id=1697900
We call GetConnectionUnixProcessID and GetConnectionUnixUser *a lot*.
And we do so synchronously. Both is a problem.
To avoid the first problem, cache the last few requests with each cached
value being valid for one second.
On a quick test, this saves 98% of the requests:
59 GetConnectionUnixProcessID(*)
3201 GetConnectionUnixProcessID(*) (served from cache)
59 GetConnectionUnixUser(*)
3201 GetConnectionUnixUser(*) (served from cache)
Note that now as we serve requests from the cache, it might be the case
that the D-Bus endpoint already disconnected. Previously, the request would
have failed but now we return the cached user-id and process-id. This
problem is mitigated by only caching the values for up to one second.
Also, it's not really a problem because we cache sender names. Those
are supposed to be unique and not repeat. So, even if the peer already
disconnected, it is still true that the corresponding PID/UID was as
we have cached it. We don't use this API for checking whether the peer
is still connected, but what UID/PID it has/had. That answer is still
correct for the cached value after the peer disconnected.
The proxy does nothing for us, except overhead.
We can directly subscribe to "NameOwnerChanged" signals on the
GDBusConnection. Also, instead of asynchronously creating the
GDBusProxy, asynchronously call "GetNameOwner". That's what the
proxy does anyway.
GDBusConnection is actually a decent API. We don't need another layer on
top of that, for functionality that we don't use.
Also, don't use G_BUS_TYPE_SYSTEM, but use the GDBusConnection that
also the bus-manager uses. For all practical purposes, that is the
connection was want to use also in NMDnsSystemdResolved.
