When looking at a property, it should always be clear how it is handled.
Also the "default" action should be an explicit hook.
Add _nm_setting_property_from_dbus_fcn_gprop() and set that as
from_dbus_fcn() callback to handle the "default" case which us
build around g_object_set_property().
While this adds lines of code, I think it makes the code easier to
understand. Basically, to convert a GVariant to a property, now all
properties call their from_dbus_fcn() handler, there is no special casing.
And the gprop-hook is only called for properties that are using
_nm_setting_property_from_dbus_fcn_gprop(). So, you can reason about
these two functions at separate layers.
NM_SETTING_NAME is also a GObject property, but it's
not supposed to be serialized to/from D-Bus. It also
is irrelevant for comparison.
Hence, it's operations are all NOPs. Make an explicit property type for
that case instead of checking the GParamSpec flags.
The "to_dbus_data" existed for namespacing the properties inside it.
However, such a struct adds overhead due to the alignment that it
enforces. We can share the memory needed for the bitfield by having
them beside each other.
The goal is to get rid of gprop_to_dbus_fcn() uses.
Note that there is a change in behavior. The "dns" GPtrArray in
NMSettingIPConfig is never NULL (the default of the boxed property),
thus the previous code always serialized the property, even the
empty list.
Now, empty dns properties are omitted from D-Bus.
Also, there is another change in behavior: nm_utils_ip4_dns_to_variant()
will now skip over strings that are not valid IPv4 addresses.
Previously, it would have added 0.0.0.0 (or some undefined address).
The goal is to get rid of gprop_to_dbus_fcn() uses.
Note that there is a change in behavior. The "dns" GPtrArray in
NMSettingIPConfig is never NULL (the default of the boxed property),
thus the previous code always serialized the property, even the
empty list.
Now, empty dns properties are omitted from D-Bus.
All settings have a "name" property. Their compare_fcn() is not interesting
and was already previously ignored. But we should not special handle it via
_nm_setting_property_compare_fcn_default().
So far, we only have NMSettingClass.compare_property() hook.
The ugliness is that this hook is per-setting, when basically
all implementations only compare one property.
It feels cleaner to have a per-property hook and call that consistently.
In step one, we give all properties (the same) compare_fcn() implementation,
which delegates to the existing NMSettingClass.compare_property().
In a second step, this will be untangled.
There is one problem with this approach: NMSettInfoPropertType grows by
one pointer size, and we have potentially many such types. That should
be addressed by unifying types in the future.
Various NMSetting API would accept a property_idx parameter. Together
with the NMSettInfoSetting instance, this was useful to find the actual
NMSettInfoProperty instance.
The idea was, to provide the most of the functionality. That is, if you
might need the property_idx too, you had it -- after all, the
property_info you could lookup yourself.
However,
- literally zero users care about the property_idx. The care about
the property_info.
- if the user really, really required the property_idx, then it
is a given that it can be easily computed by
(property_info - sett_info->property_infos)
Problems of this patch:
- the code does not differentiate between an ifcfg file and an alias
file. Different shell variables are honored however depending on the
context and the warning should reflect that.
- there are no warnings about /etc/sysconfig/network. The main problem
is that we read this file for every ifcfg file we parse, and we would
need to ratelimit the number of warnings. Another problem is that
the file likely contains keys that we intentionally don't support.
We would need a new way to omit warnings about those lines.
Example:
TYPE=Ethernet
PROXY_METHOD=none
BROWSER_ONLY=no
BOOTPROTO=dhcp
DEFROUTE=yes
STABLE_ID=$'xxx\xF4yy'
IPV4_FAILURE_FATAL=no
IPV6INIT=yes
XX=foo
XX1=foo'
'
IPV6_AUTOCONF=yes xxxx
IPV6_DEFROUTE=yesx
IPV6_DEFROUTE=yes
IPV6_FAILURE_FATAL=no
IPV6_ADDR_GEN_MODE=stable-privacy
NAME=xxx
UUID=9d8ed7ff-3cdd-4336-9e26-3e978dc87102
ONBOOT=no
<warn> [...] ifcfg-rh: ifcfg,/etc/sysconfig/network-scripts/ifcfg-xxx:6: key STABLE_ID does not contain valid UTF-8 and is treated as ""
<debug> [...] ifcfg-rh: ifcfg,/etc/sysconfig/network-scripts/ifcfg-xxx:9: key XX is unknown and ignored
<warn> [...] ifcfg-rh: ifcfg,/etc/sysconfig/network-scripts/ifcfg-xxx:10: key XX1 is badly quoted and is treated as ""
<warn> [...] ifcfg-rh: ifcfg,/etc/sysconfig/network-scripts/ifcfg-xxx:11: invalid line ignored
<warn> [...] ifcfg-rh: ifcfg,/etc/sysconfig/network-scripts/ifcfg-xxx:12: key IPV6_AUTOCONF is badly quoted and is treated as ""
<warn> [...] ifcfg-rh: ifcfg,/etc/sysconfig/network-scripts/ifcfg-xxx:13: key IPV6_DEFROUTE is duplicated and the early occurrence ignored
https://bugzilla.redhat.com/show_bug.cgi?id=1959656
ifcfg files are a text format. It makes no sense to ever accept
non-UTF-8 blobs. If binary data is to be encoded in a ifcfg file, then
the upper layers must escape/encode it in valid UTF-8.
Let svUnescape() silently reject any binary "text". This will lead to treat such
strings as empty strings "". This is no different than some invalid
quoting: the string is not parsable as (UTF-8) text and will be treated
as such.
This is potentially a breaking change. But the benefit is that all the
upper layers can rely on only getting valid UTF-8 strings. For example,
a non-UTF-8 string cannot be converted to a "s" GVariant (of course not,
it's not a string). But our nm_connection_verify() commonly does not
check that all strings are in fact valid UTF-8. So a user who edits
an ifcfg file could inject non-valid strings, and cause assertion
failures later on.
It's actually easy to provoke a crash (or at least an assertion failure)
by writing an ifcfg file with certain keys as binary.
Note that you can either reproduce the binary files by writing non-UTF-8
"strings" dirctly, or by using \x, \u, or \U escape sequences.
Note that also '\0' gets rejected and renders the string as invalid
(i.e. as empty). Before the returned string would have been simply
truncated and the rest ignored. Such NUL bytes can only be produced
using the escape sequences, because the ifcfg reader already (silently)
truncates the file on the first binary NUL.
Note that previously the check
if (s[slen] < ' ') {
...
return (*to_free = _escape_ansic(s));
}
would be TRUE for all UTF-8 characters if `char` is signed. That means,
depending on the compiler, we would always ANSI escape all UTF-8
characters. With this patch, we no longer do that!
Instead, valid unicode gets now preserved (albeit quoted).
On the other hand, always ANSIC escape invalid UTF-8 (regardless of the
compiler). ifcfg-rh is really a text based format. If a caller wants to store
binary data, they need to escape it first, for example with some own escaping
scheme, base64 or bin2hexstr.
A caller passing a non-text to svEscape() is likely a bug already and
they should have not done that.
Still, let svEscape() handle that by using ANSIC escaping. That works
as far as escaping is concerned, but likely later will be a problem
during unescaping, when the reader expects a valid UTF-8 string.
svEscape() is in no place to signal a sensible error, so proceed the
best it can, by escaping.
The libreadline starting from version 6 is licensed as GPLv3. For some
use cases it is not acceptable to use this license.
In the NetworkManager the libreadline is used by nmcli.
This change allows using libedit instead of libreadline.
Following adjustments were made:
1. The history_set_history_state() is not supported in the libedit.
Instead, the where_history() with remove_history() were used to remove
the history content if needed.
2. rl_complete_with_tilde_expansion - it is the binary flag used only
when one wants to have the expansion support. The libedit is not
supporting and hence exporting this flag.
When one wants to compile the nmcli with libedit (GPLv2 replacement of
libreadline) the rl_completion_display_matches_hook hook shall be left
untouched (as NULL) as it is not supported in libedit.
The rl_startup_hook function has different prototype in libreadline and
in the libedit.
To fix this issue, arguments of hook function has been wrapped to C
preprocessor macro and properly adjusted.
src/libnm-glib-aux/nm-random-utils.c:112:12: error: ignoring return value of 'getrandom' declared with attribute 'warn_unused_result' [-Werror=unused-result]
Fixes: 18597e33cb ('glib-aux: also use getrandom() for seeding pseudo random generator')
During nm_device_unrealize(), we first clear the device's ifindex. Then
we call _set_state_full(NM_DEVICE_STATE_UNMANAGED).
NMVpnConnection are subclasses of NMActiveConnection, it is that way
connected to NM_DEVICE_STATE_CHANGED signal. And this leads to a call
to _set_vpn_state(), which then calls nm_device_replace_vpn6_config()
to unregister the config. Thereby an assertion fails because the
ifindex no longer matches.
Fix that by relaxing the assertion. Also, don't apply the IP
configuration in unexpected device states.
https://bugzilla.redhat.com/show_bug.cgi?id=1912423https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/927
NMDeviceModem has priv->modem set from construction to dispose, and
the NM_MODEM_IP4_CONFIG_RESULT/NM_MODEM_IP6_CONFIG_RESULT signals
connected all the time.
On the other hand, NMModem may hook up to NMPPPManager's
NM_PPP_MANAGER_SIGNAL_IP{4,6}_CONFIG signals, which emit the
config-results signals. And PPP manager emits the config signals
from impl_ppp_manager_set_ip{4,6}_config().
That means, at any moment can be a D-Bus calls, which leads to emitting
those signals and calling into modem_ip4_config_result() and
modem_ip6_config_result().
At least, it's not clear from review what would prevent that from
happening. If you cannot easily verify that certain conditions are
satisfied, then this is not the place to assert, but to handle the case
as something that can happen regularly.
Handle signals in the unexpected state by ignoring them.
https://bugzilla.redhat.com/show_bug.cgi?id=1916192https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/926
- the return value of getrandom() is ssize_t.
- handle EAGAIN to indicate low entropy.
- treat a return value of zero the same as any other
low "n", by falling back to bad random bytes.
We make an effort to get a better fallback case with
_bad_random_bytes().
Also make an effort to get good randomness in the first place. Even if
we compile against libc headers that don't provide getrandom(). Also,
this isn't really ugly, because for a long time glibc was reluctant to
add getrandom() wrapper and using syscall() was the way to go.
nm_utils_random_bytes() tries to get good randomness. If it fails, we still
try our own approach, but also signal that the returned numbers are bad.
In practice, none of the callers cares about the return value, because they
wouldn't know what to do in case of bad randomness (abort() is not an
option and retry is not expected to help and sending an email to the
admin isn't gonna help either). So the fallback case really should try
its best.
The fallback case depends on a good random seed and a good pseudorandom
number generator.
Getting a good seed is in reality impossible, after kernel let us down.
That is part of the problem, but we try our best.
The other part is to use a cryptographic pseudorandom number generator.
GRand uses a Mersenne Twister, so that is not good enough. In this low
level code we also cannot call gnutls/nss, because usually we don't have
that dependency. Maybe we could copy&paste the chacha20 implementation,
it's simple enough and a compatible license. That might be good, but
instead cock our own by adding some sha256 into the mix. This is
fallback code after all, and we want to try hard, but not *that* hard to
add chacha20 to NetworkManager.
So, what we do is to use a well seeded GRand instance, and XOR that
output with a sha256 digest of the state. It's probably slow, but
performance is not the issue in this code path.
If a prefix delegation is needed, currently NM restarts DHCPv6 on the
device with default route, but only if DHCPv6 was already running.
Allow the device to start DHCPv6 for a PD even if it was running
without DHCPv6.
See also: https://github.com/coreos/fedora-coreos-tracker/issues/888
Previously we sent announcements immediately for non-controllers, or
after the first port was attached for controllers.
This has two problems:
- announcements can be sent when there is no carrier and they would
be lost;
- if a controller has a port, the port could be itself a controller;
in that case we start sending ARPs with the fake address of the
port. Later, when a leaf port is added to the second-level
controller, the correct port MAC will be propagated by kernel up to
both controllers.
To solve both problems, send ARP announcements only when the interface
has carrier. This also solves the second issue because controllers
created by NM have carrier only when there is a port with carrier.
Fixes: de1022285a ('device: do ARP announcements only after masters have a slave')
https://bugzilla.redhat.com/show_bug.cgi?id=1956793
We encode the default value "direct" properties in the GParamSpec.
But we also avoid CONSTRUCT properties, because they have an overhead
and they are generally odd for the settings.
So up to now, it was cumbersome to explicitly set the default value,
but it was also error prone.
Avoid that by always initializing the default value for our "direct"
properties.
And as example, implement NMSettingVrf.table this way. This also
makes all properties of NMSettingVrf implemened as "direct" properties,
and we can drop the explicit getter/setters.
If all settings would be strictly be implemented as "direct" properties,
we could call this from NMSetting.finalize() and be done with it.
As it is, for now we cannot, so it's still cumbersome.
