We now have a cached list of NMSettingsConnection instances,
sorted by their autoconnect priority.
However, the sort order nm_settings_connection_cmp_autoconnect_priority()
depends on various properties of the connection:
- "connection.autoconnect" and "connection.autoconnect-priority"
- the timestamp
- "connection.uuid"
These properties almost never change, so it's a waste that every call
to nm_settings_get_connections_sorted_by_autoconnect_priority() needs
to check whether the sort order is still satisfied.
We can do better by tracking when the sort order might have been
destroyed and only check in those (much fewer) cases.
Note that we end up calling nm_settings_get_connections_sorted_by_autoconnect_priority()
a lot, so this makes a difference.
The GetSettings() call is not the only place where we convert a
NMConnection to D-Bus. However it is one of the most prominent ones
with a measurable performance overhead.
The connection seldom changes, so it makes sense to cache it.
Note that GetSettings() is the only caller that specifies an option,
thus it's the only caller that converts a NMConnection to variant
in this particular way. That means, other callers don't benefit from
this caching and we could not cache the variant in the NMConnection
instance itself, because those callers use different parameters.
Turns out, we call nm_settings_get_connection_clone() *a lot* with sort order
nm_settings_connection_cmp_autoconnect_priority_p_with_data().
As we cache the (differently sorted) list of connections, also cache
the presorted list. The only complication is that every time we still
need to check whether the list is still sorted, because it would be
more complicated to invalidate the list when an entry changes which
affects the sort order. Still, such a check is usually successful
and requires "only" N-1 comparisons.
Since commit 207cf3d5d4 ('libnm: normalize "connection.uuid"') the
"connection.uuid" is normalized to be a valid UUID and all lower case.
That means, if we have .nmmeta files on disk with a previously valid,
but now invalid UUID, the meta file is no longer going to match.
Reject such file outright as invalid. If we really wanted to preserve
backward compatibility, then we would have to also normalize the
filename when we read it. However, that means, that suddenly we might
have any number of compatible .nmmeta files that normalize to the same
UUID, like the files
71088c75dec54119ab41be71bc10e736aaaabbbb.nmmeta
F95D40B4-578A-5E68-8597-39392249442B.nmmeta
f95d40b4-578a-5e68-8597-39392249442b.nmmeta
Having multiple places for the nmmeta file is complicated to handle.
Also, we often have the connection profile (and the normalized UUID)
first, and then check whether it has a .nmmeta file. If we would support
those unnormalized file names, we would have to visit all file names and
try to normalize it, to find those with a matching UUID.
Non-normalized UUIDs really should not be used and they already are not
working anymore for the .nmmeta file. This commit only outright rejects
them. This is a change in behavior, but the behavior change happened
earlier when we started normalizing "connection.uuid".
"uuid" is returned from nms_keyfile_nmmeta_check_filename(),
and contains "$UUID.nmmeta". We must compare only the first
"uuid_len" bytes.
Fixes: 064544cc07 ('settings: support storing "shadowed-storage" to .nmmeta files')
Most of the time, we care about whether we have a normalized UUID.
nm_uuid_is_valid_full() only exists for a particular case where we want
to use the function in a header, without including "nm-uuid.h". In that
case, we actually also care about normalized UUIDs.
If the TC setting contains no qdiscs and filters, it is lost after a
write-read cycle. Fix this by adding a new property to indicate the
presence of the (empty) setting.
In the past, the UUID was only loosely validate and would accept
forms that are not valid. This was fixed by commit 207cf3d5d4 ('libnm:
normalize "connection.uuid"'). Now the UUID is always strictly valid
and lower case.
Thus, don't use the fuzzy nm_utils_is_uuid() from libnm but the exact
check nm_uuid_is_valid_full().
Note that this is only used for assertions in the header file. We thus
don't want to drag in "libnm-glib-aux/nm-uuid.h". Instead, we forward
declare the function.
lgtm.com warns about declarations are block scope, so fix that too by
moving the declaration at file scope.
lgtm.com warns about function declarations inside blocks.
*sigh*. I think it's well understood what this code means, and it is not
done by accident. Still, let's make the tool happy in this case.
Previously, we would allocate a buffer of the worst case, that is,
4 times the number of bytes, in case all of them require octal escaping.
Coverity doesn't like _escape_ansic() for another reason:
Error: NULL_RETURNS (CWE-476): [#def298]
NetworkManager-1.31.5/src/core/settings/plugins/ifcfg-rh/shvar.c:161: returned_null: "g_malloc" returns "NULL".
NetworkManager-1.31.5/src/core/settings/plugins/ifcfg-rh/shvar.c:161: alias: Assigning: "q" = "dest = g_malloc(strlen(source) * 4UL + 1UL + 3UL)". Both pointers are now "NULL".
NetworkManager-1.31.5/src/core/settings/plugins/ifcfg-rh/shvar.c:163: dereference: Incrementing a pointer which might be null: "q".
# 161| q = dest = g_malloc(strlen(source) * 4 + 1 + 3);
# 162|
# 163|-> *q++ = '$';
# 164| *q++ = '\'';
# 165|
It doesn't recognize that g_malloc() shouldn't return NULL (because
we never request zero bytes).
I am not sure how to avoid that, but let's rework the code to first count
how many characters we exactly need. It think that should also help with
the coverity warning.
Doing exact allocation requires first to count the number of required
bytes. It still might be worth it, because we might keep the allocated
strings a bit longer around.
When the ACCEPT_ALL_MAC_ADDRESSES key is found by the wired reader, the
wired setting was not being created.
Fixes: d946aa0c50 ('wired-setting: add support to accept-all-mac-addresses')
Signed-off-by: Fernando Fernandez Mancera <ffmancera@riseup.net>
D-Bus 1.3.1 (2010) introduced the standard "PropertiesChanged" signal
on "org.freedesktop.DBus.Properties". NetworkManager is old, and predates
this API. From that time, it still had it's own PropertiesChanged signal
that are emitted together with the standard ones. NetworkManager
supports the standard PropertiesChanged signal since it switched to
gdbus library in version 1.2.0 (2016).
These own signals are deprecated for a long time already ([1], 2016), and
are hopefully not used by anybody anymore. libnm-glib was using them and
relied on them, but that library is gone. libnm does not use them and neither
does plasma-nm.
Hopefully no users are left that are affected by this API break.
[1] 6fb917178a
Introducing ethtool PAUSE support with:
* ethtool.pause-autoneg on/off
* ethtool.pause-rx on/off
* ethtool.pause-tx on/off
Limitations:
* When `ethtool.pause-autoneg` is set to true, the `ethtool.pause-rx`
and `ethtool.pause-tx` will be ignored. We don't have warning for
this yet.
Unit test case included.
Signed-off-by: Gris Ge <fge@redhat.com>
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/829
It's not the task of the writer to mangle/normalize profiles. If a profile
for a virtual device can have an [ethernet] setting, then unsuitable values
like s390 options must be either rejected by nm_connection_verify() or normalized
by nm_connection_normalize(). In no way it's right that the writer simple
pretends they are not set.
"string" is leaked in the error case. But in practice, this cannot
happen because nm_bridge_vlan_to_str() cannot fail.
While at it, replace GString by NMStrBuf.
Thanks Coverity:
Error: RESOURCE_LEAK (CWE-772):
NetworkManager-1.31.3/src/core/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c:1565: alloc_fn: Storage is returned from allocation function "g_string_new".
NetworkManager-1.31.3/src/core/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c:1565: var_assign: Assigning: "string" = storage returned from "g_string_new("")".
NetworkManager-1.31.3/src/core/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c:1572: leaked_storage: Variable "string" going out of scope leaks the storage it points to.
# 1570| vlan_str = nm_bridge_vlan_to_str(vlan, error);
# 1571| if (!vlan_str)
# 1572|-> return FALSE;
# 1573| if (string->len > 0)
# 1574| g_string_append(string, ",");
Apparently moving secrets between priv->connection and
priv->system_secrets in the various places throughout
NMSettingsConnection is no longer needed and has no effect on the
state of the D-Bus object or the gobject visible from outside. It
seems that it was needed for the secrets handling in NMDevice
subclasses before the introduction of the applied connection concept
but now nm_connection_need_secrets() is called in those subclasses
directly on the applied connection object and the secrets obtained
from multiple nm_settings_connection_get_secrets calls are also
collected directly in the applied connection's settings.
Drop the system secrets cache mechanism as a cause of a minor memory
overhead, some code overhead and also a source of some unneeded gobject
signals as the connection settings were being updated.
Note: the NM_SETTINGS_CONNECTION_UPDATE_REASON_CLEAR_SYSTEM_SECRETS and
NM_SETTINGS_CONNECTION_UPDATE_REASON_RESET_SYSTEM_SECRETS flags in the
SettingsConnection update signals appear to only have been used by the
SettingsConnection internally to keep priv->system_secrets up to date.
They can have potential other uses in the handlers of those signals so I
kept them. Unlike some of the other NMSettingsConnectionUpdateReason
values these are actual update *reasons* and not flags telling the
settings backends how to handle a specific change in the settings.
All usages of nm_settings_connection_clear_secrets() outside of the
NMSettingsConnection implementation were setting the
clear_cached_system_secrets parameter to FALSE which meant that the
operation was a no-op since the system-secrets cache kept a copy of the
secrets being cleared and any access to the SettingsConnection through
the D-Bus API or the class methods would behave the same as without the
call, with the exception of directly reading the settings from the
result of nm_settings_connection_get_connection(). The calls would
still generate D-Bus and gobject signals however, which were redundant.
Drop the method and its calls from the rest of NM code as not needed and
potentially confusing. The comments preceding these calls implied that
they were needed so that the next activation attempt would be forced to
use nm_settings_connection_get_secrets() but this was the case probably
only before the applied connection concept was introduced.
Also drop two nm_active_connection_clear_secrets() uses in
NMVpnConnection, right before the teardown of the active connection,
that could only possibly have any effect if they affected the
NMSettingsConnection, but as mentioned earlier the
nm_settings_connection_clear_secrets() use inside
nm_active_connection_clear_secrets() didn't do anything and is now
removed.
The one internal use of nm_active_connection_clear_secrets() in the
D-Bus ClearSecrets() implementation is inlined.
This patch is introducing the wired setting accept-all-mac-addresses
property. The value corresponds to the kernel flag IFF_PROMISC.
When accept-all-mac-address is enabled, the interface will accept all
the packets without checking the destination mac address.
Signed-off-by: Fernando Fernandez Mancera <ffmancera@riseup.net>
Extend nm_utils_file_set_contents to be able to optionally set the last
access + last modification times on the file being created, in addition
to the mode.
Along with NM_SETTINGS_CONNECTION_UPDATE_REASON_RESET_SYSTEM_SECRETS
and NM_SETTINGS_CONNECTION_UPDATE_REASON_RESET_AGENT_SECRETS, which can
be used in the NMSettingConnection's "updated" handlers to track secrets
updates, add NM_SETTINGS_CONNECTION_UPDATE_REASON_UPDATE_NON_SECRET so
that the handlers can tell when something other than secrets has been
updated in the connection.
It can also potentially be used in _connection_changed_update in
src/core/settings/nm-settings.c to stop emitting the
NetworkManager.Settings.Connection.Updated() dbus signal if only secrets
are being updated (on agent queries etc.) if it is deemed to be correct.
Review and replace usages of the two nm_connection_to_dbus() flags
marked deprecated in commit 84648e562c98 ('libnm: Refactor
NM_CONNECTION_SERIALIZE_* flags'):
NM_CONNECTION_SERIALIZE_NO_SECRETS and
NM_CONNECTION_SERIALIZE_ONLY_SECRETS.
Use the new nm_connection_to_dbus() flags to filter secrets instead of
cloning connections and using
_nm_connection_clear_secrets_by_secret_flags() and then serializing all
secrets in NMSettingsConnection. Fix a related comment.
initscripts don't support "$VLAN_ID". They actually support "$VID",
which NetworkManager doesn't.
"$VLAN_ID" was introduced by commit 10b32be37b ('ifcfg-rh: various VLAN
cleanups'). It has a comment about "backward compatibility" for the case
where the reader would ignore "$VLAN_ID" if "$DEVICE"'s name contains
a suffix that is parsable as VLAN ID.
That is wrong. If a new feature gets introduce (like NetworkManager
supporting "$VLAN_ID"), then there is no way that an older version of the
tool -- which doesn't know the new feature yet (initscripts) -- supports it.
This is not what backward compatibility means. Backward compatibility
means that if a user has an old ifcfg-file without "$VLAN_ID", then we
continue parsing it as before.
Consider, when a user (or NetworkManager) writes a configuration
DEVICE=vlan9
PHYSDEV=eth0
VLAN_ID=10
then it makes no sense to ignore VLAN_ID=10 and use "9" instead.
Otherwise the user (or NetworkManager) should not have written the
file this way.
Also, NetworkManager profiles support "connection.interface-name=vlan9"
together with "vlan.id=10". Such a configuration is valid and must be
expressible in ifcfg-rh format. The ifcfg-rh writer code did not somehow
restrict the setting of "$VLAN_ID" to account for this odd behavior. Whenever
NetworkManager in the past wrote VLAN_ID variable to file, it really meant
it.
https://bugzilla.redhat.com/show_bug.cgi?id=1907960https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/794
Asserting against user input is not nice, because it always requires the
caller to check the value first. Don't do that.
Also, don't even check. You can set NM_SETTING_WIRED_S390_OPTIONS
property to any values (except duplicated keys). The C add function
should not be more limited than that. This is also right because
we have verify() which checks for valid settings. And it does so beyond
only checking the keys.
So you could set NM_SETTING_WIRED_S390_OPTIONS properties to invalid
keys. And you could use nm_setting_wired_add_s390_option() to set
invalid values. No need to let nm_setting_wired_add_s390_option() check
for valid keys.
glib requires G_LOG_DOMAIN defined so that log messages are labeled
to belong to NetworkManager or libnm.
However, we don't actually want to use glib logging. Our library libnm
MUST not log anything, because it spams the user's stdout/stderr.
Instead, a library must report notable events via its API. Note that
there is also LIBNM_CLIENT_DEBUG to explicitly enable debug logging,
but that doesn't use glib logging either.
Also, the daemon does not use glib logging instead it logs to syslog.
When run with `--debug`.
Hence, it's not useful for us to define different G_LOG_DOMAIN per
library/application, because none of our libraries/applications should
use glib logging.
It also gets slightly confusing, because we have the static library like
`src/libnm-core-impl`, which is both linked into `libnm` (the library)
and `NetworkManager` (the daemon). Which logging domain should they use?
Set the G_LOG_DOMAIN to "nm" everywhere. But no longer do it via `-D`
arguments to the compiler.
See-also: https://developer.gnome.org/glib/stable/glib-Message-Logging.html#G-LOG-DOMAIN:CAPS
