The next commit is going to introduce a new object in libnm to
represent a range of ovs-port VLANs. A "range of integers" object
seems something that can be used for other purposes in the future, so
instead of adding an object specific for this case
(e.g. NMOvsPortVlanRange), introduce a generic NMRange object that
generically represents a range of non-negative integers.
Support managing the loopback interface through NM as the users want to
set the proper mtu for loopback interface when forwarding the packets.
Additionally, the IP addresses, DNS, route and routing rules are also
allowed to configure for the loopback connection profiles.
https://bugzilla.redhat.com/show_bug.cgi?id=2060905
nm_setting_diff() ends up calling the compare_fcn() hook. Previously,
the hook for "dns" was _nm_setting_property_compare_fcn_default()
and the hook for "dns-data" was _nm_setting_property_compare_fcn_ignore().
That's wrong. _nm_setting_property_compare_fcn_default() converts
the property to D-Bus and compares the GVariant. However, "dns" has
to_dbus_only_in_manager_process set, so it wouldn't
Fixes: 63eaf168d1 ('libnm: add "dns-data" replacement for "ipv[46].dns" properties on D-Bus')
Bond option values are just strings, however, some of them get
validated to be numbers, etc.
We also have effectively boolean values, like "use-carrier". Internally,
this is not validates as a boolean (_nm_utils_ascii_str_to_bool()) but
instead is an integer of either "0" or "1".
Add a helper function_nm_setting_bond_opt_value_as_intbool() to access
and parse such values.
The bond setting does some minimal validation of the options.
At least for those number typed values, it validates that the
string can be interpreted as a number and is within a certain range.
Add nm_assert() checks to our opt_value_u$SIZE() functions, that the
requested option is validated to be in a range which is sufficiently
narrow to be converted to the requested type. If that were not the case,
we would need some special handling (or question whether the option should
be retrieved as this type).
These variants provide additional nm_assert() checks, and are thus
preferable.
Note that we cannot just blindly replace &g_array_index() with
&nm_g_array_index(), because the latter would not allow getting a
pointer at index [arr->len]. That might be a valid (though uncommon)
usecase. The correct replacement of &g_array_index() is thus
nm_g_array_index_p().
I checked the code manually and replaced uses of nm_g_array_index_p()
with &nm_g_array_index(), if that was a safe thing to do. The latter
seems preferable, because it is familar to &g_array_index().
- name things related to `in_addr_t`, `struct in6_addr`, `NMIPAddr` as
`nm_ip4_addr_*()`, `nm_ip6_addr_*()`, `nm_ip_addr_*()`, respectively.
- we have a wrapper `nm_inet_ntop()` for `inet_ntop()`. This name
of our wrapper is chosen to be familiar with the libc underlying
function. With this, also name functions that are about string
representations of addresses `nm_inet_*()`, `nm_inet4_*()`,
`nm_inet6_*()`. For example, `nm_inet_parse_str()`,
`nm_inet_is_normalized()`.
<<<<
R() {
git grep -l "$1" | xargs sed -i "s/\<$1\>/$2/g"
}
R NM_CMP_DIRECT_IN4ADDR_SAME_PREFIX NM_CMP_DIRECT_IP4_ADDR_SAME_PREFIX
R NM_CMP_DIRECT_IN6ADDR_SAME_PREFIX NM_CMP_DIRECT_IP6_ADDR_SAME_PREFIX
R NM_UTILS_INET_ADDRSTRLEN NM_INET_ADDRSTRLEN
R _nm_utils_inet4_ntop nm_inet4_ntop
R _nm_utils_inet6_ntop nm_inet6_ntop
R _nm_utils_ip4_get_default_prefix nm_ip4_addr_get_default_prefix
R _nm_utils_ip4_get_default_prefix0 nm_ip4_addr_get_default_prefix0
R _nm_utils_ip4_netmask_to_prefix nm_ip4_addr_netmask_to_prefix
R _nm_utils_ip4_prefix_to_netmask nm_ip4_addr_netmask_from_prefix
R nm_utils_inet4_ntop_dup nm_inet4_ntop_dup
R nm_utils_inet6_ntop_dup nm_inet6_ntop_dup
R nm_utils_inet_ntop nm_inet_ntop
R nm_utils_inet_ntop_dup nm_inet_ntop_dup
R nm_utils_ip4_address_clear_host_address nm_ip4_addr_clear_host_address
R nm_utils_ip4_address_is_link_local nm_ip4_addr_is_link_local
R nm_utils_ip4_address_is_loopback nm_ip4_addr_is_loopback
R nm_utils_ip4_address_is_zeronet nm_ip4_addr_is_zeronet
R nm_utils_ip4_address_same_prefix nm_ip4_addr_same_prefix
R nm_utils_ip4_address_same_prefix_cmp nm_ip4_addr_same_prefix_cmp
R nm_utils_ip6_address_clear_host_address nm_ip6_addr_clear_host_address
R nm_utils_ip6_address_same_prefix nm_ip6_addr_same_prefix
R nm_utils_ip6_address_same_prefix_cmp nm_ip6_addr_same_prefix_cmp
R nm_utils_ip6_is_ula nm_ip6_addr_is_ula
R nm_utils_ip_address_same_prefix nm_ip_addr_same_prefix
R nm_utils_ip_address_same_prefix_cmp nm_ip_addr_same_prefix_cmp
R nm_utils_ip_is_site_local nm_ip_addr_is_site_local
R nm_utils_ipaddr_is_normalized nm_inet_is_normalized
R nm_utils_ipaddr_is_valid nm_inet_is_valid
R nm_utils_ipx_address_clear_host_address nm_ip_addr_clear_host_address
R nm_utils_parse_inaddr nm_inet_parse_str
R nm_utils_parse_inaddr_bin nm_inet_parse_bin
R nm_utils_parse_inaddr_bin_full nm_inet_parse_bin_full
R nm_utils_parse_inaddr_prefix nm_inet_parse_with_prefix_str
R nm_utils_parse_inaddr_prefix_bin nm_inet_parse_with_prefix_bin
R test_nm_utils_ip6_address_same_prefix test_nm_ip_addr_same_prefix
./contrib/scripts/nm-code-format.sh -F
It can be useful to choose a different "ipv6.addr-gen-mode". And it can be
useful to override the default for a set of profiles.
For example, in cloud or in a data center, stable-privacy might not be
the best choice. Add a mechanism to override the default via global defaults
in NetworkManager.conf:
# /etc/NetworkManager/conf.d/90-ipv6-addr-gen-mode-override.conf
[connection-90-ipv6-addr-gen-mode-override]
match-device=type:ethernet
ipv6.addr-gen-mode=0
"ipv6.addr-gen-mode" is a special property, because its default depends on
the component that configures the profile.
- when read from disk (keyfile and ifcfg-rh), a missing addr-gen-mode
key means to default to "eui64".
- when configured via D-Bus, a missing addr-gen-mode property means to
default to "stable-privacy".
- libnm's ip6-config::addr-gen-mode property defaults to
"stable-privacy".
- when some tool creates a profile, they either can explicitly
set the mode, or they get the default of the underlying mechanisms
above.
- nm-initrd-generator explicitly sets "eui64" for profiles it creates.
- nmcli doesn' explicitly set it, but inherits the default form
libnm's ip6-config::addr-gen-mode.
- when NM creates a auto-default-connection for ethernet ("Wired connection 1"),
it inherits the default from libnm's ip6-config::addr-gen-mode.
Global connection defaults only take effect when the per-profile
value is set to a special default/unset value. To account for the
different cases above, we add two such special values: "default" and
"default-or-eui64". That's something we didn't do before, but it seams
useful and easy to understand.
Also, this neatly expresses the current behaviors we already have. E.g.
if you don't specify the "addr-gen-mode" in a keyfile, "default-or-eui64"
is a pretty clear thing.
Note that usually we cannot change default values, in particular not for
libnm's properties. That is because we don't serialize the default
values to D-Bus/keyfile, so if we change the default, we change
behavior. Here we change from "stable-privacy" to "default" and
from "eui64" to "default-or-eui64". That means, the user only experiences
a change in behavior, if they have a ".conf" file that overrides the default.
https://bugzilla.redhat.com/show_bug.cgi?id=1743161https://bugzilla.redhat.com/show_bug.cgi?id=2082682
See-also: https://github.com/coreos/fedora-coreos-tracker/issues/907https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/1213
gcc-12.0.1-0.8.fc36 is annoying with false positives.
It's related to g_error() and its `for(;;) ;`.
For example:
../src/libnm-glib-aux/nm-shared-utils.c: In function 'nm_utils_parse_inaddr_bin_full':
../src/libnm-glib-aux/nm-shared-utils.c:1145:26: error: dangling pointer to 'error' may be used [-Werror=dangling-pointer=]
1145 | error->message);
| ^~
/usr/include/glib-2.0/glib/gmessages.h:343:32: note: in definition of macro 'g_error'
343 | __VA_ARGS__); \
| ^~~~~~~~~~~
../src/libnm-glib-aux/nm-shared-utils.c:1133:31: note: 'error' declared here
1133 | gs_free_error GError *error = NULL;
| ^~~~~
/usr/include/glib-2.0/glib/gmessages.h:341:25: error: dangling pointer to 'addrbin' may be used [-Werror=dangling-pointer=]
341 | g_log (G_LOG_DOMAIN, \
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
342 | G_LOG_LEVEL_ERROR, \
| ~~~~~~~~~~~~~~~~~~~~~~~
343 | __VA_ARGS__); \
| ~~~~~~~~~~~~
../src/libnm-glib-aux/nm-shared-utils.c:1141:13: note: in expansion of macro 'g_error'
1141 | g_error("unexpected assertion failure: could parse \"%s\" as %s, but not accepted by "
| ^~~~~~~
../src/libnm-glib-aux/nm-shared-utils.c:1112:14: note: 'addrbin' declared here
1112 | NMIPAddr addrbin;
| ^~~~~~~
I think the warning could potentially be useful and prevent real bugs.
So don't disable it altogether, but go through the effort to suppress it
at the places where it currently happens.
Note that NM_PRAGMA_WARNING_DISABLE_DANGLING_POINTER macro only expands
to suppressing the warning with __GNUC__ equal to 12. The purpose is to
only suppress the warning where we know we want to. Hopefully other gcc
versions don't have this problem.
I guess, we could also write a NM_COMPILER_WARNING() check in
"m4/compiler_options.m4", to disable the warning if we detect it. But
that seems too cumbersome.
G_TYPE_STRV is the last property type in NMSetting that is implemented
by directly accessing the GObect property. Note that we have lots of
override, non-default implementations that still use GObject properties,
but I am talking here about properties that don't have a special
implementation and use a G_TYPE_STRV GObject property.
Add a "direct" implementation also for strv arrays.
The advantage is that we no longer call g_value_get() for various
operations, which requires a deep-copy of the strv array. The other
advantage is that we will get a unified approach for implementing strv
properties. In particular strv arrays need a lot of code to implement,
and most settings do it differently. By adding a general mechanism,
this code (and behavior) can be unified.
Showcase it on "match.interface-name".
In function '_nm_auto_g_free',
inlined from 'test_tc_config_tfilter_matchall_mirred' at src/libnm-core-impl/tests/test-setting.c:2955:24:
./src/libnm-glib-aux/nm-macros-internal.h:58:1: error: 'str' may be used uninitialized [-Werror=maybe-uninitialized]
58 | NM_AUTO_DEFINE_FCN_VOID0(void *, _nm_auto_g_free, g_free);
| ^
src/libnm-core-impl/tests/test-setting.c: In function 'test_tc_config_tfilter_matchall_mirred':
src/libnm-core-impl/tests/test-setting.c:2955:24: note: 'str' was declared here
2955 | gs_free char *str;
| ^
lto1: all warnings being treated as errors
lto-wrapper: fatal error: gcc returned 1 exit status
Several properties like "connection.type" are enum-like and only take a few
known values. We can use a NMRefString to share their instances.
Currently nm_setting_duplicate() does not yet explicitly handle direct properties.
But it should, because it can handle them more efficiently. If it would do that, it
would be very cheap to "copy" a NMRefString. But even with the current implementation
will the result be deduplicated.
We want that our properties have little special cases and follow a
few common behaviors. For example, we have string properties, and those
should mostly behave the same (e.g. by being "direct-string"
properties).
That is already not fully enough, because we have slightly different
behaviors. For example, we have string properties that should have their
whitespace stripped, that should be ascii case down converted, that
should be normalized IP or MAC addresses. So far, that was expressed via
simple fields in NMSettInfoProperty, like NMSettInfoProperty's
direct_set_string_ascii_strdown field.
But that is not enough. In particular, for "wireguard.private-key" we
perform a different kind of normalization (base64 parsing, and taking
care not to leak secret in memory). It seems to special to add a boolean
flag "direct_set_string_wireguard_private_key".
Instead, add a hook that can cover that.
We need a hook, because we want one setter implementation throughout. Commonly,
we have at least two setters: the GObject set_property() and from D-Bus.
Both should call into the same underlying implementation, to avoid code
duplication. For that, the tweaked behavior must be "down", that is at
the deepest point in the call stack where we set the string. That's why
we need the hook. The alternative would be two special implementation
for GObject and D-Bus setters (and in the future we might add setters
from keyfile).
This seems a questionable thing to do, and should be made clearer by
having a parameter (that makes you think about what is happening here).
Also, the normalization for vxlan.remote does not perform this mapping,
so the parameter is there so that the approach can handle both flavors.
Let's sprinkle some snake ointment.
This is questionable, because we copy secrets all over the place where
we their deallocation (and clearing) is not in our control. For example,
the GValue setter/getter copies the string (but does not clean the
secret). Also, when converting the property to a GVariant, we won't
clear it. So this does not catch a lot of cases.
Still, if we can with relative ease avoid leaking the string at some
places, do it.
We use clang-format for automatic formatting of our source files.
Since clang-format is actively maintained software, the actual
formatting depends on the used version of clang-format. That is
unfortunate and painful, but really unavoidable unless clang-format
would be strictly bug-compatible.
So the version that we must use is from the current Fedora release, which
is also tested by our gitlab-ci. Previously, we were using Fedora 34 with
clang-tools-extra-12.0.1-1.fc34.x86_64.
As Fedora 35 comes along, we need to update our formatting as Fedora 35
comes with version "13.0.0~rc1-1.fc35".
An alternative would be to freeze on version 12, but that has different
problems (like, it's cumbersome to rebuild clang 12 on Fedora 35 and it
would be cumbersome for our developers which are on Fedora 35 to use a
clang that they cannot easily install).
The (differently painful) solution is to reformat from time to time, as we
switch to a new Fedora (and thus clang) version.
Usually we would expect that such a reformatting brings minor changes.
But this time, the changes are huge. That is mentioned in the release
notes [1] as
Makes PointerAligment: Right working with AlignConsecutiveDeclarations. (Fixes https://llvm.org/PR27353)
[1] https://releases.llvm.org/13.0.0/tools/clang/docs/ReleaseNotes.html#clang-format
"direct" properties are the latest preferred way to implement GObject
base properties. That way, the property meta data tracks the
"direct_type" and the offset where to find the data in the struct.
That way, we can automatically
- initialize the default values
- free during finalize
- implement get_property()/set_property()
Also, the other settings operations (compare, to/from D-Bus) are
implemented more efficiently and don't need to go through
g_object_get_property()/GValue API.
"flags" are a g_param_spec_flags() and correspond to G_TYPE_FLAGS type.
They are internally stored as guint, and exported on D-Bus as "u" (32 bit
integer).
String properties in libnm's NMSetting really should have NULL as a
default value. The only property that didn't, was "dcb.app-fcoe-mode".
Change the default so that it is also NULL.
Changing a default value is an API change, but in this case probably no
issue. For one, DCB is little used. But also, it's not clear who would
care and notice the change. Also, because previously verify() would reject
a NULL value as invalid. That means, there are no existing, valid profiles
that have this value set to NULL. We just make NULL the default, and
define that it means the same as "fabric".
Note that when we convert integer properties to D-Bus/GVariant, we often
omit the default value. For string properties, they are serialized as
"s" variant type. As such, NULL cannot be expressed as "s" type, so we
represent NULL by omitting the property. That makes especially sense if
the default value is also NULL. Otherwise, it's rather odd. We change
that, and we will now always express non-NULL value on D-Bus and let
NULL be encoded by omitting the property.
The name prefix "nmtst_*" is reserved for test helpers and stub
function. Such functions should not be in the actual build artifacts,
like the NetworkManager binary.
Instead, nmtst_connection_assert_unchanging() is not a test helper. It
is a assertion function that is only enabled with NM_MORE_ASSERTS
builds. That's different.
Rename.
In other words,
$ nm src/core/NetworkManager src/libnm-client-impl/.libs/libnm.so | grep nmtst
should give no results.
Naming is important, because the name of a thing should give you a good
idea what it does. Also, to find a thing, it needs a good name in the
first place. But naming is also hard.
Historically, some strv helper API was named as nm_utils_strv_*(),
and some API had a leading underscore (as it is internal API).
This was all inconsistent. Do some renaming and try to unify things.
We get rid of the leading underscore if this is just a regular
(internal) helper. But not for example from _nm_strv_find_first(),
because that is the implementation of nm_strv_find_first().
- _nm_utils_strv_cleanup() -> nm_strv_cleanup()
- _nm_utils_strv_cleanup_const() -> nm_strv_cleanup_const()
- _nm_utils_strv_cmp_n() -> _nm_strv_cmp_n()
- _nm_utils_strv_dup() -> _nm_strv_dup()
- _nm_utils_strv_dup_packed() -> _nm_strv_dup_packed()
- _nm_utils_strv_find_first() -> _nm_strv_find_first()
- _nm_utils_strv_sort() -> _nm_strv_sort()
- _nm_utils_strv_to_ptrarray() -> nm_strv_to_ptrarray()
- _nm_utils_strv_to_slist() -> nm_strv_to_gslist()
- nm_utils_strv_cmp_n() -> nm_strv_cmp_n()
- nm_utils_strv_dup() -> nm_strv_dup()
- nm_utils_strv_dup_packed() -> nm_strv_dup_packed()
- nm_utils_strv_dup_shallow_maybe_a() -> nm_strv_dup_shallow_maybe_a()
- nm_utils_strv_equal() -> nm_strv_equal()
- nm_utils_strv_find_binary_search() -> nm_strv_find_binary_search()
- nm_utils_strv_find_first() -> nm_strv_find_first()
- nm_utils_strv_make_deep_copied() -> nm_strv_make_deep_copied()
- nm_utils_strv_make_deep_copied_n() -> nm_strv_make_deep_copied_n()
- nm_utils_strv_make_deep_copied_nonnull() -> nm_strv_make_deep_copied_nonnull()
- nm_utils_strv_sort() -> nm_strv_sort()
Note that no names are swapped and none of the new names existed
previously. That means, all the new names are really new, which
simplifies to find errors due to this larger refactoring. E.g. if
you backport a patch from after this change to an old branch, you'll
get a compiler error and notice that something is missing.
There is a quest to move away from the GObject/GValue based setters.
Add _nm_setting_property_from_dbus_fcn_direct(), which can parse
the GVariant and use the direct_type to set the property.
Note that for backward compatibility, we still need
_nm_property_variant_to_gvalue() to convert alternative GVariant
types to the destination value. This means, as before, on the D-Bus
API a property of a certain type can be represented as various D-Bus
types.
This is a normalization employed by NMSettingIPConfig.gateway.
Also rework NMSettingIPConfig.set_property() to no longer assert against
valid input. We want to pass there untrusted strings from D-Bus,
asserting is a horrible idea. Instead, either normalize the string or
keep the invalid text that will be rejected by verify().
A MAC address is a relatively common "type". The GObject property is of type string,
but the D-Bus type is a bytestring ("ay"). We will need a special NMSettInfoPropertType.
Note that like most implementations, the from-dbus implementation still is based
on GObject setters. This will change in the future.
Also note that the previous compare function was
_nm_setting_property_compare_fcn_default(). That is, it used to convert
the property to GVariant and compare those. The conversion to GVariant
in that case normalizes the string (e.g. it is case insensitive). Also,
only properties could be compared which were also convertible to D-Bus
(which is probably fine, because there is no guarantee the profiles that
don't verify can be compared).
The code now uses the direct comparison of the strings. That mostly
preserves the case-insensitivity of the previous comparison, because
the property setters for mac addresses all use
_nm_utils_hwaddr_canonical_or_invalid() to normalize the strings.
This is subtle, but still correct. Note that this will improve later,
by ensuring that the property setters for mac addresses automatically
perform the right normalization.
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.
