Sometimes these function may set errno to unexpected values like EAGAIN.
This causes confusion. Avoid that by using our own wrappers that retry
in that case. For example, in rhbz#1797915 we have failures like:
errno = 0;
v = g_ascii_strtoll ("10", 0, &end);
if (errno != 0)
g_assert_not_reached ();
as g_ascii_strtoll() would return 10, but also set errno to EAGAIN.
Work around that by using wrapper functions that retry. This certainly
should be fixed in glib (or glibc), but the issues are severe enough to
warrant a workaround.
Note that our workarounds are very defensive. We only retry 2 times, if
we get an unexpected errno value. This is in the hope to recover from
a spurious EAGAIN. It won't recover from other errors.
https://bugzilla.redhat.com/show_bug.cgi?id=1797915
Be more graceful and allow whitespaces around the floating point number
for DEVTIMEOUT. Note that _nm_utils_ascii_str_to_int64() is already graceful
against whitespace, so also be it with the g_ascii_strtod() code path.
Scenario:
- have ethernet connection as unmanaged.
- create (or have) a suitable profile for the connection.
- make the device as managed. No default wired connection gets
created.
- delete the profile.
Note that NMManager does in manager_device_state_changed():
»···if (NM_IN_SET (new_state,
»··· NM_DEVICE_STATE_UNAVAILABLE,
»··· NM_DEVICE_STATE_DISCONNECTED))
»···»···nm_settings_device_added (priv->settings, device);
that means, when the device the next time goes through
UNAVAILABLE/DISCONNECTED states, we will suddenly create the
default "Wired connection 1" profile.
That doesn't seem right. When a device is suitable to have a
default-wired connection, we should only check once whether to
create it. We should not retry that later. The !no-auto-default
mechanism exists so we can start NetworkManager without a profile for
the device. It doesn't mean that we later one (after previously deciding
not to create a profile), we still create it.
https://bugzilla.redhat.com/show_bug.cgi?id=1687937https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/450
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
I think it's preferable to use nm_clear_g_free() instead of
g_clear_pointer(, g_free). The reasons are not very strong,
but I think it is overall preferable to have a shorthand for this
frequently used functionality.
sed 's/\<g_clear_pointer *(\([^;]*\), *\(g_free\) *)/nm_clear_g_free (\1)/g' $(git grep -l g_clear_pointer) -i
Since ifcfg-rh doesn't write out to file the 'connection.timestamp' property
let's add it before comparing an updated connection with the plugin's reread
one otherwise the comparison operation would always fail.
The fix is not necessary for the keyfile plugin, because the reader/writer
correctly reads/writes the connection timestamp.
Don't do a linear search through all names, but use binary search.
Upside: calling nms_ifcfg_rh_utils_get_ethtool_by_name() in a loop
(once over all 60 names) is 75% faster.
Downside: when adding a new feature, we have yet another line that we
need to add. Previously, adding a new feature required adding 7 lines,
not it is 8. But we didn't add a single feature since this was added,
so that happens very seldom.
Possible downside: is this code harder to read? Now we track both how to
convert the ID to name and back. This is redundant (and thus harder to
maintain). But it's really just one extra line per feature, for which there
is a unit test. So, when adding a new NMEthtoolID it would be pretty
hard to mess this up, because of all the tests and assertions.
So, maybe it's slightly harder to read. On the other hand, it unifies
handling for ethtool and kernel names, and the code has less logic
and is more descriptive. I don't think this is actually harder to maintain
and it should be easy to see that it is correct (readability).
Maybe the reader should not try to add its own validation. It
could just read the value, set it in the profile, and let
nm_connection_verify() handle it.
However:
- in this form the code only logs a warning about invalid setting.
If we let it come to nm_connection_verify(), the connection profile
will be entirely rejected. I think this makes sense, because ifcfg
files may be edited by the user and we don't know what is out there.
- it's nicer to show a warning that specifically mentions the DEVICE=
variable. There error message we get from nm_connection_verify()
is no longer aware of ifcfg peculiarities.
Instead: use the appropriate validation function.
Internally, the options are tracked in a hash table and of undefined
sort order. However, nm_setting_bond_get_option() always returns a stable
(sorted) order.
Move "mode" as first, because that is usually the most interesting option.
The effect is:
$ nmcli -o connection show "$BOND_PROFILE"
...
-bond.options: arp_interval=5,arp_ip_target=192.168.7.7,arp_validate=active,mode=balance-rr,use_carrier=0
+bond.options: mode=balance-rr,arp_interval=5,arp_ip_target=192.168.7.7,arp_validate=active,use_carrier=0
This doesn't affect keyfile, which sorts the hash keys themself (and
doesn't treat the "mode" special).
This however does affect ifcfg-rh writer how it writes the BONDING_OPTS
variable. I think this change is fine and preferable.
nm_utils_is_valid_iface_name() is a public API of libnm-core, let's use
our internal API.
$ sed -i 's/\<nm_utils_is_valid_iface_name\>/nm_utils_ifname_valid_kernel/g' $(git grep -l nm_utils_is_valid_iface_name)
It is undefined behavior and can lead to crashes or memory corruption.
In practice, this only had an issue on Big Endian systems.
Fixes: fdbf4ae5e6 ('ifcfg-rh: add IPV4_DHCP_TIMEOUT key for ipv4.dhcp-timeout property')
If a function is only called once, it may not help to simplify the code
but make it more complicated. It would only simplify the code, if it
had a clear, distinct purpose. That isn't the case here. Also, the
IPv4 writer doesn't have such a function either. Drop and inline it.
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)
In all the cases, we don't want to perform locale dependent comparison.
$ sed -i 's/\<strcasecmp\>/g_ascii_\0/g' $(git grep -w -l strcasecmp -- ':(exclude)shared/systemd/' )
Most callers would pass FALSE to nm_utils_error_is_cancelled(). That's
not very useful. Split the two functions and have nm_utils_error_is_cancelled()
and nm_utils_error_is_cancelled_is_disposing().
Add missing 'extern' keyword to fix the following error caused by GCC
10 defaulting to -fno-common:
src/settings/plugins/ifcfg-rh/.libs/libnms-ifcfg-rh-core.a(libnms_ifcfg_rh_core_la-shvar.o):/root/NetworkManager/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-utils.h:36: multiple definition of `nms_ifcfg_well_known_keys';
src/settings/plugins/ifcfg-rh/.libs/libnm_settings_plugin_ifcfg_rh_la-nms-ifcfg-rh-plugin.o:/root/NetworkManager/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-utils.h:36: first defined here
We should use the same "is-valid" function everywhere.
Since nm_utils_ipaddr_valid() is part of libnm, it does not qualify.
Use nm_utils_ipaddr_is_valid() instead.
and _nm_utils_inet6_ntop() instead of nm_utils_inet6_ntop().
nm_utils_inet4_ntop()/nm_utils_inet6_ntop() are public API of libnm.
For one, that means they are only available in code that links with
libnm/libnm-core. But such basic helpers should be available everywhere.
Also, they accept NULL as destination buffers. We keep that behavior
for potential libnm users, but internally we never want to use the
static buffers. This patch needs to take care that there are no callers
of _nm_utils_inet[46]_ntop() that pass NULL buffers.
Also, _nm_utils_inet[46]_ntop() are inline functions and the compiler
can get rid of them.
We should consistently use the same variant of the helper. The only
downside is that the "good" name is already taken. The leading
underscore is rather ugly and inconsistent.
Also, with our internal variants we can use "static array indices in
function parameter declarations" next. Thereby the compiler helps
to ensure that the provided buffers are of the right size.
Just read the content once. It's wasteful to read the file twice
while parsing.
But what is worse, the file content can change any time we read it.
We make decisions based on what we read, and hence we should only
read the file once in the parsing process to get one consistent result.
Split the parsing of the file content to a separate function.
Next we will load IPv4 files only once, and thus only need to parse
the content without reading it.
Note that the function temporarily modifies the input buffer, but
restores the original content before returning.
We will need both variants, so that the caller can read the file only
once.
Note that also utils_has_route_file_new_syntax_content() will
restore the original contents and not modify the input (from point
of view of the caller). In practice it will momentarily modify the
content.
Keyfile support was initially added under GPL-2.0+ license as part of
core. It was moved to "libnm-core" in commit 59eb5312a5 ('keyfile: merge
branch 'th/libnm-keyfile-bgo744699'').
"libnm-core" is statically linked with by core and "libnm". In
the former case under terms of GPL-2.0+ (good) and in the latter case
under terms of LGPL-2.1+ (bad).
In fact, to this day, "libnm" doesn't actually use the code. The linker
will probably remove all the GPL-2.0+ symbols when compiled with
gc-sections or LTO. Still, linking them together in the first place
makes "libnm" only available under GPL code (despite the code
not actually being used).
Instead, move the GPL code to a separate static library
"shared/nm-keyfile/libnm-keyfile.la" and only link it to the part
that actually uses the code (and which is GPL licensed too).
This fixes the license violation.
Eventually, it would be very useful to be able to expose keyfile
handling via "libnm". However that is not straight forward due to the
licensing conflict.
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/merge_requests/381
We don't need a separate "GSList *chains" to track the NMAuthChain
requests for the agents. Every agent should only have one auth-chain in
fly at any time. We can attach that NMAuthChain to the secret-agent.
Also, fix a race where:
1) A secret agent registers. We would start an auth-chain check, but not
yet track the secret agent.
2) Then the secret agent unregisters. The unregistration request will fail,
because the secret agent is not yet in the list of fully registered agents.
The same happens if the secret agent disconnects at this point.
agent_disconnect_cb() would not find the secret agent to remove.
3) afterwards, authentication completes and we register the
secret-agent, although we should not.
There is also another race: if we get authority_changed_cb() we would
not restart the authentication for the secret-agent that is still
registering. Hence, we don't know whether the result once it completes
would already contain the latest state.
Don't access the singleton getter here. Pass the agent-manager argument
instead to maybe_remove_agent_on_error().
Also, don't lookup the agent by name. We already know, whether the agent
is still tracked or not. Look at agent->agent_lst.
nm_agent_manager_get_agent_by_user() would only return the first
matching secret agent for the user. This way, we might miss an agent
that has permissions.
Instead, add nm_agent_manager_has_agent_with_permission() and search
all agents.
There was literally only one place where we would make use of
O(1) lookup of secret-agents: during removal.
In all other cases (which are the common cases) we had to iterate the
known agents. CList is more efficient and more convenient to use when
the main mode of operation is iterating.
Also note that handling secret agents inevitably scales linear with
the number of agents. That is, because for every check we will have
to sort the list of agents and send requests to them. It would be
very complicated (and probably less efficient for reasonable numbers
of secret agents) to avoid O(n).
NMAgentManager and NMSecretAgent work closely together. In particular,
the NMAgentManager creates and tracks the NMSecretAgents and controls
it.
Move NMSecretAgent struct to the header, so that some fields may become
accessible to NMAgentManager. In particular, we will track secret agents
with a CList, and this CList element can be embedded in the
NMSecretAgent structure.
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.
We call svFindFirstNumberedKey() to check whether we have any NETMASK
set. Since commit 9085c5c3a9 ('ifcfg-rh: rename
svFindFirstKeyWithPrefix() to svFindFirstNumberedKey() for finding
NETMASK') that function would no longer find the "NETMASK" without
number.
Fix that, by letting nms_ifcfg_rh_utils_is_numbered_tag() return TRUE
for the tag itself. This also makes more sense, because it matches our
common understanding what numbered tags are.
Adjust the other callers that don't want this behavior to explicitly
check.
Fixes: 9085c5c3a9 ('ifcfg-rh: rename svFindFirstKeyWithPrefix() to svFindFirstNumberedKey() for finding NETMASK')
