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.
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".
If the kernel command-line doesn't contain an explict ip=$method,
currently the generator creates connections with both IPv4 and IPv6
set to 'auto', and both allowed to fail.
Since NM is run in configure-and-quit mode in the initrd, NM can get
an IPv4 address or an IPv6 one (or both) depending on which address
family is quicker to complete. This unpredictable behavior is not
present in the legacy module, which always does IPv4 only by default.
Set a required-timeout of 20 seconds for IPv4, so that NM will
preferably get an IPv4, or will fall back to IPv6.
See also: https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/issues/729
This reverts commit 389575a6b1.
When the command line contains BOOTIF and there is another ip=
argument specifying an interface name, we can follow 2 approaches:
a) BOOTIF creates a new distinct connection with DHCP
(the behaviour before the commit)
b) the connection generated for ip= will be also be bound to the
BOOTIF MAC (the behavior introduced by the commit)
Restore a) because we can't be sure that the MAC address refers to the
same interface. In that case it's preferable to generate a different
connection.
https://bugzilla.redhat.com/show_bug.cgi?id=1915493#c35
Ignore a rd.znet argument without subchannels. When using net.ifnames
(the default), subchannels are used to build the interface name, which
is required to match the right connection.
With net.ifnames=0 the interface name is build using a prefix and a
global counter and therefore in theory it is possible to omit
subchannels. However, without subchannels there won't be a udev rule
that renames the interface and so it can't work.
https://bugzilla.redhat.com/show_bug.cgi?id=1931284https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/783
