Commonly, the prefix is a string constant. We don't need to call
g_str_has_prefix() for that, which first requires strlen() on
the prefix. All the information is readily available.
Add a macro for that.
The state handling in add_and_activate_cb() and connect_device_cb() is
redundant to connected_state_cb() and in fact executed only if the
activation is really really really quick (which it never is).
Also, the return_text those implementations provide is different from
what connected_state_cb(), potentially confusing the users and adding
extra work for translators.
Not to mention extra lines of code, reading whose wastes our precious
time on the planet we could spend doing heroin instead.
Try to locate an existing connection before creating a new one when
handling "nmcli device wifi connect". This allows WPA-Enterprise
networks to be activated this way, consistent with the comment that this
command is equivalent to clicking on an SSID in a GUI client.
It's not yet used, but it will be. We will need nm_sd_utils_unbase64mem()
to strictly validate WireGuard settings, which contain keys in base64 encoding.
Note that we also need a stub implementation for logging. This will do
nothing for all logging from "libnm-systemd-shared.a". This makes
sense because "libnm.so" as a library should not log directly. Also,
"libnm.so" will only use a small portion of "libnm-systemd-shared.a" which
doesn't log anything. Thus this code is unused and dropped by the linker
with "--gc-sections".
glib has an base64 implementation, but g_base64_decode() et al. gives
no way to detect invalid encodings. All invalid codes are silently
ignored. That is not suitable for strictly validating user input.
Instead of reimplementing of copy-pasting the code from somewhere,
reuse systemd's unbase64mem().
But don't use "hexdecoct.h" directly. Instead, add a single accessor
function to our "nm-sd-utils-shared.h" gateway. We want to be careful
about which bits from systemd we use, because otherwise re-importing
systemd code becomes fragile as you don't know which relevant parts
changed.
For better or worse, we already pull in large parts of systemd sources.
I need a base64 decode implementation (because glib's g_base64_decode()
cannot reject invalid encodings). Instead of coming up with my own or
copy-paste if from somewhere, reuse systemd's unbase64mem().
But for that, make systemd's basic bits an independent static library
first because I will need it in libnm-core.
This doesn't really change anything except making "libnm-systemd-core.la"
an indpendent static library that could be used from "libnm-core". We
shall still be mindful about which internal code of systemd we use, and only
access functionality that is exposed via "systemd/nm-sd-utils-shared.h".
C11 provides _Generic(). Until now we used it when the compiler supports
it (in extended --std=gnu99 mode). In practice, now that we require C11
it should always be present.
We will drop compatibility code in the future. For now, just add a comment
and keep it. The reason is, that "shared/nm-utils/nm-macros-internal.h"
may be used by VPN plugins or applet, which may or may not yet bump to
C11. Keeping it for now, allows for an easier update.
We already import systemd code which is C11. To get this even
to build, we need workaround like patching import of <uchar.h>.
Also, the libraries from c-util and nettools are C11. We cannot even
compile them in C99 mode (and didn't do that either).
It's time to bump the version. We need C11 from now on (or better: gcc's
dialect of it).
Also, note that since nettools/nacd is not optional, we could not even
build NetworkManager without a C11 compiler. So, just use it everywhere.
In core ("src/"), we use "nm-logging.h" for all logging. This dispatches
for logging to syslog, glog or systemd-journald.
If we want to log from a shared component under "shared/", we need to
use a common logging function. Add "nm-utils/nm-logging-fwd.h" for
forward declaring the used logging mechaism.
The shared library will still need to link with "src/nm-logging.c"
or an alternative implementation, depending on whether it is used
inside core or not.
- in nm_keyfile_read(), unify _read_setting() and
_read_setting_vpn_secret() in they way they are called
(that is, they no longer return any value and don't accept
any arguments aside @info).
- use cleanup attributes
- use nm_streq() instead of strcmp().
- wrap lines that have multiple statements or conditions.
Several callers access the length output argument, expecting
it to be zero also on failure. That is a bug, because glib does
not guarantee that.
Fix that by making a stronger promise from our wrappers: the output
argument should also be set on failure.
Also ensure that calls to g_keyfile_get_groups() and
g_keyfile_get_keys() don't rely on the length output to be valid,
when the function call fails.
Actually, these issues were not severe because:
- `g_key_file_get_*_list()`'s implementation always sets the output length
even on failure (undocumented).
- `g_key_file_get_groups()` cannot fail and always set the length.
- `g_key_file_get_keys()` is called under circumstances where it won't
fail.
Still, be explicit about it.
This allows us to add a file "TODO.txt" in the top level directory.
This file is not intended to be merged to master, but keep track of
stuff that is still to do before merging a branch.
Let checkpatch.pl warn about the presence of such a file.
We need more information what failed. Don't only return success/failure,
but an error number.
Note that we still don't actually return an error number. Only
the link_add() function is changed to return an nm-error integer.
Platform had it's own scheme for reporting errors: NMPlatformError.
Before, NMPlatformError indicated success via zero, negative integer
values are numbers from <errno.h>, and positive integer values are
platform specific codes. This changes now according to nm-error:
success is still zero. Negative values indicate a failure, where the
numeric value is either from <errno.h> or one of our error codes.
The meaning of positive values depends on the functions. Most functions
can only report an error reason (negative) and success (zero). For such
functions, positive values should never be returned (but the caller
should anticipate them).
For some functions, positive values could mean additional information
(but still success). That depends.
This is also what systemd does, except that systemd only returns
(negative) integers from <errno.h>, while we merge our own error codes
into the range of <errno.h>.
The advantage is to get rid of one way how to signal errors. The other
advantage is, that these error codes are compatible with all other
nm-errno values. For example, previously negative values indicated error
codes from <errno.h>, but it did not entail error codes from netlink.
This will be our extension on top of <errno.h>.
We want to use (integer) error numbers, that can both
contain native errors from <errno.h> and our own defines,
both merge in one domain. That is, we will reserve a small
range of integers for our own defines (that hopefully won't
clash with errors from <errno.h>).
We can use this at places where GError is too cumbersome to use.
The advantage is, that our error numbers extend <errno.h> and can
be mixed.
This is what "src/platform/nm-netlink.h" already does with nl_errno(). Next,
the netlink errors from there will be merged into "nm-errno.h".
Also, platform has NMPlatformError, which are a distinct set of error
numbers. But these work differently in the sense that negative values
represent codes from <errno.h> and positive numbers are our own platform
specific defines. NMPlatformError will also be merged into "nm-errno.h".
"nm-errno.h" will unify the error handling of platform and netlink,
making it more similar to what we are used to from systemd, and give
room to extend it for our own purpose.
We use sd_dhcp_client_set_client_id() and sd_dhcp6_client_set_duid()
with the aim to set arbitrary client identifiers and DUIDs. Adjust
systemd DHCP library to not reject certain values.
https://github.com/systemd/systemd/pull/112105848a9eb4d
There are various functions to set the DUID of a DHCPv6 client.
However, none of them allows to set arbitrary data. The closest is
sd_dhcp6_client_set_duid(), which would still do validation of the
DUID's content via dhcp_validate_duid_len().
Relax the validation and only log a debug message if the DUID
does not validate.
Note that dhcp_validate_duid_len() already is not very strict. For example
with DUID_TYPE_LLT it only ensures that the length is suitable to contain
hwtype and time. It does not further check that the length of hwaddr is non-zero
or suitable for hwtype. Also, non-well-known DUID types are accepted for
extensibility. Why reject certain DUIDs but allowing clearly wrong formats
otherwise?
The validation and failure should happen earlier, when accepting the
unsuitable DUID. At that point, there is more context of what is wrong,
and a better failure reason (or warning) can be reported to the user. Rejecting
the DUID when setting up the DHCPv6 client seems not optimal, in particular
because the DHCPv6 client does not care about actual content of the
DUID and treats it as opaque blob.
Also, NetworkManager (which uses this code) allows to configure the entire
binary DUID in binary. It intentionally does not validate the binary
content any further. Hence, it needs to be able to set _invalid_ DUIDs,
provided that some basic constraints are satisfied (like the maximum length).
sd_dhcp6_client_set_duid() has two callers: both set the DUID obtained
from link_get_duid(), which comes from configuration.
`man networkd.conf` says: "The configured DHCP DUID should conform to
the specification in RFC 3315, RFC 6355.". It does not not state that
it MUST conform.
Note that dhcp_validate_duid_len() has another caller: DHCPv4's
dhcp_client_set_iaid_duid_internal(). In this case, continue with
strict validation, as the callers are more controlled. Also, there is
already sd_dhcp_client_set_client_id() which can be used to bypass
this check and set arbitrary client identifiers.
ab4a88bc29
sd_dhcp_client_set_client_id() is the only API for setting a raw client-id.
All other setters are more restricted and only allow to set a type 255 DUID.
Also, dhcp4_set_client_identifier() is the only caller, which already
does:
r = sd_dhcp_client_set_client_id(link->dhcp_client,
ARPHRD_ETHER,
(const uint8_t *) &link->mac,
sizeof(link->mac));
and hence ensures that the data length is indeed ETH_ALEN.
Drop additional input validation from sd_dhcp_client_set_client_id(). The client-id
is an opaque blob, and if a caller wishes to set type 1 (ethernet) or type 32
(infiniband) with unexpected address length, it should be allowed. The actual
client-id is not relevant to the DHCP client, and it's the responsibility of the
caller to generate a suitable client-id.
For example, in NetworkManager you can configure all the bytes of the
client-id, including such _invalid_ settings. I think it makes sense,
to allow the user to fully configure the identifier. Even if such configuration
would be rejected, it would be the responsibility of the higher layers (including
a sensible error message to the user) and not fail later during
sd_dhcp_client_set_client_id().
Still log a debug message if the length is unexpected.
bfda0d0f09
Infiniband addresses are 20 bytes (INFINIBAND_ALEN), but only the last
8 bytes are suitable for putting into the client-id.
This bug had no effect for networkd, because sd_dhcp_client_set_client_id()
has only one caller which always uses ARPHRD_ETHER type.
I was unable to find good references for why this is correct ([1]). Fedora/RHEL
has patches for ISC dhclient that also only use the last 8 bytes ([2], [3]).
RFC 4390 (Dynamic Host Configuration Protocol (DHCP) over InfiniBand) [4] does
not discuss the content of the client-id either.
[1] https://bugzilla.redhat.com/show_bug.cgi?id=1658057#c29
[2] https://bugzilla.redhat.com/show_bug.cgi?id=660681
[3] 3ccf3c8d81/f/dhcp-lpf-ib.patch
[4] https://tools.ietf.org/html/rfc4390b9d8071458
When the link is up and goes down link_changed_cb() schedules
device_link_changed() to be run later. If the function is dispatched
when the link is already up again, it does not detect that the link
was down.
Fix this by storing in the device state that we saw the link down so
that device_link_changed() can properly restore the IP configuration.
https://bugzilla.redhat.com/show_bug.cgi?id=1636715https://github.com/NetworkManager/NetworkManager/pull/264
The variable containing the list of compiler arguments to be checked
can be removed without any harm to readibility.
The variable has been removed by appending directly to the list of
common compiler arguments, those that are supported.
Although some paths related to DBus and PolicyKit are present in
their pkg-config files, those paths might not be writable for the
user. To solve this issue, some build options are present that can
be used to choose a different location.
However, usually these paths are relative to some other variables
such as `prefix`, `datadir`, etc. Using the `define_variable`
option the relative path can be change to point to a directory
under prefix.
These paths are now using relative paths based on the installation
`prefix` and their related options have been removed as they are
unnecessary now. Only `dbus_conf_dir` option has been left because
it must be modified depending on the distribution[0].
[0] contrib/fedora/rpm/NetworkManager.spec
