nm_strv_find_first() is useful (and used) to find the first index (if
any). I can thus also used to check for membership.
However, we also have nm_strv_contains(), which seems better for
readability, when we check for membership. Use it.
nm_inet_parse_bin_full() supports a legacy mode for IPv4, which used
inet_aton(). This is only used by initrd reader, which parses the
kernel command line as defined by dracut. Since that dracut API is old
and not defined by us, we want to be more forgiving in case a user
specifies something that used to work in the past. In particular,
we want to parse "255.256.256.000" as netmask (which inet_pton() would
reject).
inet_aton() trips off some ABI checkers that we shouldn't use this ABI.
It was anyway only used as *additional* guard when we parsed certain
legacy formats for IPv4 addresses. We can drop that and just use our
parser.
Note that there is still an nm_assert() path, which loads inet_aton()
dynamically, just to ensure that our legacy parser implementation is in
agree with inet_aton().
https://bugzilla.redhat.com/show_bug.cgi?id=2049134
This is the version shipped in Fedora 37. As Fedora 37 is now out, the
core developers switch to it. Our gitlab-ci will also use that as base
image for the check-{patch.tree} tests and to generate the pages. There
is a need that everybody agrees on which clang-format version to use,
and that version should be the one of the currently used Fedora release.
Also update the used Fedora image in "contrib/scripts/nm-code-format-container.sh"
script.
The gitlab-ci still needs update in the following commit. The change
in isolation will break the "check-tree" test.
For a long time we have a function like nm_uuid_generate_from_strings().
This was recently reworked and renamed, but it preserved behavior. Preserving
behavior is important for this function, because for the existing users,
we need to keep generating the same UUIDs.
Originally, this function was a variadic function with NULL sentinel.
That means, when you write
nm_uuid_generate_from_strings(uuid_type, type_arg, v1, v2, v3, NULL);
and v2 happens to be NULL, then v3 is ignored. That is most likely not
what the user intended. Maybe they had a bug and v2 should not be NULL.
But nm_uuid_generate_from_strings() should not require that all
arguments are non-NULL and it should not ignore arguments after the
first NULL.
For example, one user works around this via
uuid = nm_uuid_generate_from_strings_old("ibft",
s_hwaddr,
s_vlanid ? "V" : "v",
s_vlanid ? s_vlanid : "",
s_ipaddr ? "A" : "DHCP",
s_ipaddr ? s_ipaddr : "");
which is cumbersome and ugly.
That will be fixed next, by adding a function that doesn't suffer
from this problem. But "this problem" is part of the API of the
function, we cannot just change it. Instead, rename it and all
users, so they can keep doing the same.
New users of course should no longer use the "old" function.
For new uses of nm_uuid_generate_from_strings() we should generate version5
UUIDs and we should use unique namespace UUID arguments.
The namespace UUID was so far replaced by always passing a special prefix
as first string. It seems nicer to use a namespace instead.
Version3 UUIDs should not be used for new applications.
Hence, nm_uuid_generate_from_strings_v3() is no longer a desirable way to
generate UUIDs, so drop the wrapper.
nm_uuid_generate_from_strings() uses variant3 UUIDs based on MD5.
We shouldn't use that in the future.
We will add a replacement, so rename this function so that the "good"
name is free again. Of course, code that uses this function currently
relies on that the behavior doesn't change. We cannot just drop it
entirely, but will replace it by something that gives the same result.
Rename.
... and profiles from firmware with autoconnect-priority -200.
In general, after switch root we remember the still activated profile in
/run, and NetworkManager would take over the device with the same
profile as before. In that case, autoconnect and autoconnect-priority
doesn't matter.
Autoconnect only matters when having a device in disconnected state and
not being blocked from autoconnect. For example, if you unplug and
replug the cable. In that case, it does make sense to me that
user-provided profiles from real-root are preferred.
To me the reasons for this change is not very strong (but neither are
the reasons against it). Read the discussion on rhbz #2089707.
https://bugzilla.redhat.com/show_bug.cgi?id=2089707
Co-authored-by: Lubomir Rintel <lkundrak@v3.sk>
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/1376
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().
- move the second g_file_test() inside the if-block. No need to check
twice, if the file exists.
- load_one_nic() can return NULL. Use nm_g_hash_table_lookup() to avoid
NULL pointer assertion.
- use cleanup attribute for "nic" variable, and explicitly pass
ownership on with g_steal_pointer().
- 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
In initrd, a too short carrier timeout means that the machine will
possibly fail to boot. On the other hand, increasing the value doesn't
have side effects, except for a bit longer delay on some machines.
Increase the value to 10 seconds. Note that the default value is not
propagated to the real root.
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/1239
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
We have two variants of the function: nm_utils_ip4_netmask_to_prefix()
and _nm_utils_ip4_netmask_to_prefix(). The former only exists because it
is public API in libnm. Internally, only use the latter.
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
"Impossible to set rd.ethtool options: invalid format" is not very
clear. Try to explain what is invalid about the format (the interface
name is missing).
"Invalid value for rd.ethtool.autoneg, rd.ethtool.autoneg was not set"
is also confusing. The message gets printed if the autoneg value was
specified on the command line, so "was not set" seems wrong. Maybe the
message meant that the profile value is left at the default (FALSE),
but that isn't very clear.
Reword.
The idea of positional arguments is that they might be extended in the
future. That means, there might be an option "rd.ethtool:eth0:::foo".
Also, if multiple "rd.ethtool:eth0" options are specified on the command
line, then the autoneg/speed settings should only be set if present.
That means
"rd.ethtool:eth0:on:100 rd.ethtool:eth0:::foo"
should work as expected and first set autoneg/speed options, but the
second argument only sets "foo" (without resetting autoneg/speed).
To NetworkManager, "autoneg=FALSE && speed=0" has the meaning to
not configure these options and leave whatever is configured previously.
That is also the default.
Explicitly configuring "rd.ethtool=eth0:off:0" is thus likely a misconfiguration,
because it tells NetworkManager to not configure the interface.
Note that the user can configure that, via "rd.ethtool=eth0::", that
is by omitting all parameters. That is a valid configuration and causes
no warning. The reason to support this silently, is so that we can
add in the future more positional arguments that the user can set
without changing autoneg/speed.
The point of positional arguments is that you can omit them, and that
should be treated as the parameter being set to the default.
So, don't treat "rd.ethtool=eth0" (or "rd.ethtool=eth0:") special.
Just continue the parsing and take all following positional arguments
as unset.
Don't return early from parsing "autoneg", if there are not additional
arguments.
The behavior should be exactly the same, whether a positional
argument is missing, empty, or set to the default.
That is,
- "rd.ethtool=eth0:on"
- "rd.ethtool=eth0🔛"
- "rd.ethtool=eth0🔛:"
- "rd.ethtool=eth0🔛0:"
should all evaluate the same thing.
That was already the case in practice, but that was hard to see.
So don't treat missing positional arguments special and don't return
early. Parse all parameters regardless.
The change is visible when parsing "rd.ethtool=eth0:off:100 rd.ethtool=eth0:on".
Autoneg and speed really belongs together, so when we parse the second
argument, we should reset the speed too -- even if it's not present.
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.
Dracut supports several options for the "ip=" method.
NetworkManager interprets and handles them in a certain way that aims to
give a similar behavior. But as such it maps different settings ("auth6"
and "dhcp6") to exactly the same behavior.
Add _parse_ip_method() function to normalize these keys, and map their
aliases to the keyword that nm-initrd-generator handles. The advantage
is that you see now in _parse_ip_method() which methods are mapped to
the same behavior, and the later (more complex) code only deals with the
normalized kinds.
Also, use the same validation code at all 3 places where IP methods
can appear, that is
ip=<method>
ip=<ifname>:<method>[:...]
ip=<client-ip>:...:<method>[:...]
Also, dracut supports specifying multiple methods and concatenate them
with comma. nm-initrd-generator only did partly, for example,
`ip=dhcp,dhcp6" would have worked, but only because the code failed
to recognize the string and fell back to the default behavior. It would
not have worked as `ip=<ifname>:dhcp,dhcp6[:...]`. Not all combinations
make sense, but some do. So let _parse_ip_method() detect and handle
them. Currently, they mostly map to "auto", but in the future it might
make sense that `ip=dhcp,local6` is a distinct kind.
Try to tighten up the parsing. It's fine to be forgiving and flexible
about what we parse, but bogus values should not silently be
accepted. However, in order to keep previous behavior, `ip=bogus`
and `ip=<client-ip>:...:<bogus-method>[:...]` explicitly map invalid
method to "auto".
Introduce a user tag key to indicate where the connection comes
from. It would also be possible to have this as a standard property
(as 'connection.origin'), but since this information can be considered
'meta-data' I think the user setting is more appropriate.
