We don't do that for ip4_start() either. The duid/client-id
is stored inside the NMDhcpClient instance, and the function can
access it from there.
Maybe, it is often preferable to have stateless objects and not
relying on ip4_start() to obtain the client ID from the client's
state. However, the purpose of the NMDhcpClient object is to
hold state about DHCP. To simplify the complexity of objects that
inherrently have state, we should be careful about mutating the state.
It adds little additional complexity of only reading the state when
needed anyway. In fact, it adds complexity, because previously
it wasn't enough to check all callers of nm_dhcp_client_get_client_id()
to see where the client-id is used. Instead, one would also need to
follow the @duid argument several layers of the call stack.
There should be lower layers that are concerned with writing
and reading dhclient configuration files. It's wrong to
have a nm_dhcp_dhclient_save_duid() function which requires
the caller to pre-escape the string to write. The caller shouldn't
be concerned with the file format, that's why the function
is used in the first place.
We only used "client_id" for IPv4 and "duid" for IPv6. Merge them.
Another advantage is, that we can share the logging functionality
of _set_client_id().
Drop unused function.
Aside from that, dhclient configuration files support a very complex
syntax. The parser was very naive and insufficient in parsing such
files. It's good we can just drop it.
Why would we do this? The configuration file we are reading back was
written by NetworkManager in the first place.
Maybe when assuming a connection after restart, this information could
be interesting. It however is not actually relevant.
Note how nm_dhcp_client_get_client_id() has only very few callers.
- nm_device_spawn_iface_helper() in 'nm-device.c'. In this case,
we either should use the client-id which we used when starting
DHCP, or none at all.
- ip4_start() in 'nm-dhcp-dhclient.c', but this is before starting
DHCP client and before it was re-read from configuration file.
- in "src/dhcp/nm-dhcp-systemd.c", but this has no effect for
the dhclient plugin.
Previously, whenever we needed /etc/machine-id we would re-load it
from file. The are 3 downsides of that:
- the smallest downside is the runtime overhead of repeatedly
reading the file and parse it.
- as we read it multiple times, it may change anytime. Most
code in NetworkManager does not expect or handle a change of
the machine-id.
Generally, the admin should make sure that the machine-id is properly
initialized before NetworkManager starts, and not change it. As such,
a change of the machine-id should never happen in practice.
But if it would change, we would get odd behaviors. Note for example
how generate_duid_from_machine_id() already cached the generated DUID
and only read it once.
It's better to pick the machine-id once, and rely to use the same
one for the remainder of the program.
If the admin wants to change the machine-id, NetworkManager must be
restarted as well (in case the admin cares).
Also, as we now only load it once, it makes sense to log an error
(once) when we fail to read the machine-id.
- previously, loading the machine-id could fail each time. And we
have to somehow handle that error. It seems, the best thing what we
anyway can do, is to log an error once and continue with a fake
machine-id. Here we add a fake machine-id based on the secret-key
or the boot-id. Now obtaining a machine-id can no longer fail
and error handling is no longer necessary.
Also, ensure that a machine-id of all zeros is not valid.
Technically, a machine-id is not an RFC 4122 UUID. But it's
the same size, so we also use NMUuid data structure for it.
While at it, also refactor caching of the boot-id and the secret
key. In particular, fix the thread-safety of the double-checked
locking implementations.
In the past, the headers "linux/if.h" and "net/if.h" were incompatible.
That means, we can either include one or the other, but not both.
This is fixed in the meantime, however the issue still exists when
building against older kernel/glibc.
That means, including one of these headers from a header file
is problematic. In particular if it's a header like "nm-platform.h",
which itself is dragged in by many other headers.
Avoid that by not including these headers from "platform.h", but instead
from the source files where needed (or possibly from less popular header
files).
Currently there is no problem. However, this allows an unknowing user to
include <net/if.h> at the same time with "nm-platform.h", which is easy
to get wrong.
The need for this is the following:
"ipv4.dhcp-client-id" can be specified via global connection defaults.
In absence of any configuration in NetworkManager, the default depends
on the DHCP client plugin. In case of "dhclient", the default further
depends on /etc/dhcp.
For "internal" plugin, we may very well want to change the default
client-id to "mac" by universally installing a configuration
snippet
[connection-use-mac-client-id]
ipv4.dhcp-client-id=mac
However, if we the user happens to enable "dhclient" plugin, this also
forces the client-id and overrules configuration from /etc/dhcp. The real
problem is, that dhclient can be configured via means outside of NetworkManager,
so our defaults shall not overwrite defaults from /etc/dhcp.
With the new device spec, we can avoid this issue:
[connection-dhcp-client-id]
match-device=except:dhcp-plugin:dhclient
ipv4.dhcp-client-id=mac
This will be part of the solution for rh#1640494. Note that merely
dropping a configuration snippet is not yet enough. More fixes for
DHCP will follow. Also, bug rh#1640494 may have alternative solutions
as well. The nice part of this new feature is that it is generally
useful for configuring connection defaults and not specifically for
the client-id issue.
Note that this match spec is per-device, although the plugin is selected
globally. That makes some sense, because in the future we may or may not
configure the DHCP plugin per-device or per address family.
https://bugzilla.redhat.com/show_bug.cgi?id=1640494
When the client terminates, we really don't care if it exited cleanly,
with an error or killed by a signal. We expect the client to never
exit and so all these situations are equally bad for us. Introduce a
new TERMINATED state instead of reusing existing FAIL or DONE states,
which are set when receiving particular events from the client.
We already have nm_utils_bin2hexstr() and _nm_utils_bin2hexstr_full().
This is confusing.
- nm_utils_bin2hexstr() is public API of libnm. Also, it has
a last argument @final_len to truncate the string at that
length.
It uses no delimiter and lower-case characters.
- _nm_utils_bin2hexstr_full() does not do any truncation, but
it has options to specify a delimiter, the character case,
and to update a given buffer in-place. Also, like
nm_utils_bin2hexstr() and _nm_utils_bin2hexstr() it can
allocate a new buffer on demand.
- _nm_utils_bin2hexstr() would use ':' as delimiter and make
the case configurable. Also, it would always allocate the returned
buffer.
It's too much and confusing. Drop _nm_utils_bin2hexstr() which is internal
API and just a wrapper around _nm_utils_bin2hexstr_full().
We need a mode that:
* doesn't leave processes behind
* doesn't force an internal dhclient
* doesn't auto-generate default connections
* doesn't write out files into libdir, only /run
The original configure-and-quit mode doesn't really fit the initrd use. But
it's proobably not a good idea to just change its behavior.
On networked boot we need to somehow communicate this to the early boot
machinery. Sadly, no DBus there and we're running in configure-and-quit
mode.
Abusing the state file for this sounds almost reasonable and is
reasonably straightforward thing to do.
dhcpcd version 6, the first supporting IPv6, was released more than 5
years ago. Remove all checks on version number and IPv6 support.
(cherry picked from commit e0c49d7341)
Internal DHCPv4 client requires a valid MAC address for functioning.
Just always require a MAC address to start DHCP, both v4 and v6.
We have no MAC address for example on Layer3 devices like tun or wireguard.
Also, before "0a797bdc2a systemd/dhcp: fix assertion starting DHCP
client without MAC address", if we tired to start sd_dhcp_client without
setting a MAC address, an assertion was triggered.
(cherry picked from commit e8fa75ce06)
Using '#ifdef' is generally error prone. It's better to always define
a define and check for it explicitly. This way, the compiler can issue
a warning if the define does not exist.
Also, note how meson would always define NM_MORE_LOGGING, possibly to
"0". That means, for meson, we unintentionally always enabled more
logging because the define was always present.
Fix that.
Extend the lame-ass dhclient.conf parser to ignore the blocks we can't
do anything useful about: alias{}, pseudo{} and even lease{}.
Note that there's still a lot of cases we can't handle without a
full-fledged dhclient.conf parser -- notably the files that don't use
line breaks to separate the statements.
That is probably okay -- the whole thing is probably mostly useless and
we shall ever bother only about cases that actually cause trouble.
https://github.com/NetworkManager/NetworkManager/pull/153
1) the command line gets shorter. I frequently run `make V=1` to see
the command line arguments for the compiler, and there is a lot
of noise.
2) define each of these variables at one place. This makes it easy
to verify that for all compilation units, a particular
define has the same value. Previously that was not obvious or
even not the case (see commit e5d1a71396
and commit d63cf1ef2f).
The point is to avoid redundancy.
3) not all compilation units need all defines. In fact, most modules
would only need a few of these defines. We aimed to pass the necessary
minium of defines to each compilation unit, but that was non-obvious
to get right and often we set a define that wasn't used. See for example
"src_settings_plugins_ibft_cppflags" which needlessly had "-DSYSCONFDIR".
This question is now entirely avoided by just defining all variables in
a header. We don't care to find the minimum, because every component
gets anyway all defines from the header.
4) this also avoids the situation, where a module that previously did
not use a particular define gets modified to require it. Previously,
that would have required to identify the missing define, and add
it to the CFLAGS of the complation unit. Since every compilation
now includes "config-extra.h", all defines are available everywhere.
5) the fact that each define is now available in all compilation units
could be perceived as a downside. But it isn't, because these defines
should have a unique name and one specific value. Defining the same
name with different values, or refer to the same value by different
names is a bug, not a desirable feature. Since these defines should
be unique accross the entire tree, there is no problem in providing
them to every compilation unit.
6) the reason why we generate "config-extra.h" this way, instead of using
AC_DEFINE() in configure.ac, is due to the particular handling of
autoconf for directory variables. See [1].
With meson, it would be trivial to put them into "config.h.meson".
While that is not easy with autoconf, the "config-extra.h" workaround
seems still preferable to me.
[1] https://www.gnu.org/software/autoconf/manual/autoconf-2.63/html_node/Installation-Directory-Variables.html
We commonly don't use the glib typedefs for char/short/int/long,
but their C types directly.
$ git grep '\<g\(char\|short\|int\|long\|float\|double\)\>' | wc -l
587
$ git grep '\<\(char\|short\|int\|long\|float\|double\)\>' | wc -l
21114
One could argue that using the glib typedefs is preferable in
public API (of our glib based libnm library) or where it clearly
is related to glib, like during
g_object_set (obj, PROPERTY, (gint) value, NULL);
However, that argument does not seem strong, because in practice we don't
follow that argument today, and seldomly use the glib typedefs.
Also, the style guide for this would be hard to formalize, because
"using them where clearly related to a glib" is a very loose suggestion.
Also note that glib typedefs will always just be typedefs of the
underlying C types. There is no danger of glib changing the meaning
of these typedefs (because that would be a major API break of glib).
A simple style guide is instead: don't use these typedefs.
No manual actions, I only ran the bash script:
FILES=($(git ls-files '*.[hc]'))
sed -i \
-e 's/\<g\(char\|short\|int\|long\|float\|double\)\>\( [^ ]\)/\1\2/g' \
-e 's/\<g\(char\|short\|int\|long\|float\|double\)\> /\1 /g' \
-e 's/\<g\(char\|short\|int\|long\|float\|double\)\>/\1/g' \
"${FILES[@]}"
Option to check just in NM private dhcp client specific lease files has
been dropped: either get DUID from specific DHCP plugin or just use the
provided one.
This reverts commit f054c3fcaa.
(cherry picked from commit 08116409f3)
When the used client is dhclient we were used to search for DUID not
only in the specific lease files generated by NetworkManager, but also
in the global lease file generated outside NetworkManager.
Keep this capability but allow to just search in the NM lease files if
a value different from the default one is specified in dhcp-duid.
This commit centralizes the DUID generation in nm-device.c.
As a consequence, a DUID is always provided when starting a
DHCPv6 client. The DHCP client can override the passed DUID
with the value contained in the client-specific lease file.
allow to specify the DUID to be used int the DHCPv6 client identifier
option: the dhcp-duid property accepts either a hex string or the
special values "lease", "llt", "ll", "stable-llt", "stable-ll" and
"stable-uuid".
"lease": give priority to the DUID available in the lease file if any,
otherwise fallback to a global default dependant on the dhcp
client used. This is the default and reflects how the DUID
was managed previously.
"ll": enforce generation and use of LL type DUID based on the current
hardware address.
"llt": enforce generation and use of LLT type DUID based on the current
hardware address and a stable time field.
"stable-ll": enforce generation and use of LL type DUID based on a
link layer address derived from the stable id.
"stable-llt": enforce generation and use of LLT type DUID based on
a link layer address and a timestamp both derived from the
stable id.
"stable-uuid": enforce generation and use of a UUID type DUID based on a
uuid generated from the stable id.
We will soon introduce a property to set a custom DUID and we want
to enforce that the provided value is used.
Note that this commit does not cause any change in behavior in current
code.
The nm_dhcp_dhclient_save_duid() function will save a newly generated
DUID to a previously existing lease file. The function will only save
the DUID if not present in the lease file: in this case, should preserve
the other contents of the lease file.
A dhclient lease file for IPv6 generated by NetworkManager will always
add the DUID as a first item: so in practice finding a lease file
without DUID will never happen.
This has hidden a bug in the function: the loop that is meant to append
the non-duid lines in the lease file would strip all the newlines,
mangling the lease file.
Fix the function allowing to keep the original lines and add a test to
check this functionality is kept well functioning.
FIXME: the new test and the other duid ones already there store the file
in the current working-directory. Tests should not do that.
Use two common defines NM_BUILD_SRCDIR and NM_BUILD_BUILDDIR
for specifying the location of srcdir and builddir.
Note that this is only relevant for tests, as they expect
a certain layout of the directories, to find files that concern
them.
Coccinelle:
@@
expression a, b;
@@
-a ? a : b
+a ?: b
Applied with:
spatch --sp-file ternary.cocci --in-place --smpl-spacing --dir .
With some manual adjustments on spots that Cocci didn't catch for
reasons unknown.
Thanks to the marvelous effort of the GNU compiler developer we can now
spare a couple of bits that could be used for more important things,
like this commit message. Standards commitees yet have to catch up.
Requesting broadcast replies from the DHCP server can be problematic in
filtered environments like some wireless networks. Don't override the
default of using unicast. This matches the behaviour of the external DHCP
clients.
https://github.com/NetworkManager/NetworkManager/pull/93
There are multiple tests with the same in different directories; add a
unique prefix to test names so that it is clear from the output which
one is running.
