Since we already cached the result of getpagesize() in a static variable (at
two places), move the code to nm-shared-utils, so it is reusable.
Also, use sysconf() instead of getpagesize(), like suggested by `man
getpagesize`.
Using strncpy() in the macro directly can result in a compiler warning.
We don't want to replace this with memcpy(), because strncpy() aborts
on the first NUL and fills the rest with NUL. Since nm_strndup_a() is a
replacement for g_strndup(), we want to do that here as well.
In file included from ../shared/nm-default.h:294,
from ../libnm-core/nm-utils.c:22:
../libnm-core/nm-utils.c: In function nm_sock_addr_endpoint_new:
../shared/nm-utils/nm-shared-utils.h:281:4: error: strncpy output truncated before terminating nul copying as many bytes from a string as its length [-Werror=stringop-truncation]
strncpy (_s, _str, _len); \
^~~~~~~~~~~~~~~~~~~~~~~~
../libnm-core/nm-utils.c:154:26: note: in expansion of macro nm_strndup_a
host = _parse_endpoint (nm_strndup_a (200, endpoint, l_endpoint - 1, &host_clone),
^~~~~~~~~~~~
../libnm-core/nm-utils.c:152:15: note: length computed here
l_endpoint = strlen (endpoint) + 1;
^~~~~~~~~~~~~~~~~
Add helper wrappers around nm_g_object_set_property() that take a
native value, construct a GValue of the according type, and call
nm_g_object_set_property().
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.
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.
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.
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.
Preface: RFC 3442 (The Classless Static Route Option for Dynamic Host
Configuration Protocol (DHCP) version 4) states:
If the DHCP server returns both a Classless Static Routes option and
a Router option, the DHCP client MUST ignore the Router option.
Similarly, if the DHCP server returns both a Classless Static Routes
option and a Static Routes option, the DHCP client MUST ignore the
Static Routes option.
Changes:
- sd_dhcp_lease_get_routes() returns the combination of both option 33
(static routes) and 121 (classless static routes). If classless static
routes are provided, the state routes must be ignored.
- we collect the options hash that we expose on D-Bus. For that purpose,
we must not merge both option types as classless static routes. Instead,
we want to expose the values like we received them originally: as two
different options.
- we continue our deviation from RFC 3442, when receiving classless static
routes with option 3 (Router), we only ignore the router if we didn't
already receive a default route via classless static routes.
- in the past, NetworkManager treated the default route specially, and
one device could only have one default route. That limitation was
already (partly) lifted by commit 5c299454b4
(core: rework tracking of gateway/default-route in ip-config). However,
from DHCP we still would only accept one default route. Fix that for
internal client. Installing multiple default routes might make sense, as
kernel apparently can skip unreachable routers (as it notes via ICMP
messages) (rh#1634657).
https://bugzilla.redhat.com/show_bug.cgi?id=1634657
We have nm_utils_inet*_ntop(), however:
- that is partly private API libnm-core, and thus only available in
components that have access to that. Partly it's public API of
libnm, but still only available in components that use libnm.
- relying on the static buffers is discouraged for nm_utils_inet*_ntop().
For testing, that is fine as we are in a more controlled envionment.
So, add a test variant that explicitly relies on static buffers.
That way, it's more convenient to use from tests.
- these functions can assert more and are more convenient to use from
tests.
With the default 128KiB buffer size it is easy to lose events. For
example when 64 interfaces appear at the same time, we lose events for
the last 16. Increase the buffer size to 4MiB.
https://bugzilla.redhat.com/show_bug.cgi?id=1651578
CSiphash is a first class citizen, it's fine to use everwhere where we
need it.
NMHash wraps CSiphash and provides three things:
1) Convenience macros that make hashing nicer to use.
2) it uses a randomly generated, per-run hash seed, that can be combined
with a guint static seed.
3) it's a general API for hashing data. It nowhere promises that it
actually uses siphash24, although currently it does everywhere.
NMHash is not (officially) siphash24.
Add API nm_hash_siphash42_init() and nm_hash_siphash42() to "nm-hash-utils.h",
that exposes (2) for use with regular CSiphash. You of course no longer
get the convenice macros (1) but you get plain siphash24 (which
NMHash does not give (3)).
While at it, also add a nm_hash_complete_u64(). Usually, for hasing we
want guint types. But we don't need to hide the fact, that the
underlying value is first uint64. Expose it.
These are simple macros/functions that append a heap-allocated string to
a GPtrArray. It is intended for a GPtrArray which takes ownership of the
strings (meaning, it has a g_free() GDestroyNotify).
From [1]:
You may optionally specify attribute names with ‘__’ preceding and
following the name. This allows you to use them in header files
without being concerned about a possible macro of the same name. For
example, you may use the attribute name __noreturn__ instead of
noreturn.
[1] https://gcc.gnu.org/onlinedocs/gcc/Attribute-Syntax.html#Attribute-Syntax
Add a new CON_DEFAULT() macro that places a property name into a
special section used at runtime to check whether it is a supported
connection default.
Unfortunately, this mechanism doesn't work for plugins so we have to
enumerate the connection defaults from plugins in the daemon using
another CON_DEFAULT_NOP() macro.
Supporting a trailing comma in NM_MAKE_STRV() can be desirable, because it
allows to extend the code with less noise in the diff.
Now, there may or may not be a trailing comma at the end.
There is a downside to this: the following no longer work:
const char *const v1[] = NM_MAKE_STRV ("a", "b");
const char *const v2[3] = NM_MAKE_STRV ("a", "b");
but then, above can be written more simply already as:
const char *const v1[] = { "a", "b", NULL };
const char *const v2[3] = { "a", "b" };
so the fact that the macro won't work in that case may be preferable,
because it forces you to use the already existing better variant.
nm_utils_get_monotonic_timestamp*() inherrently use static data. Let's
initialize it in a thread safe manner.
nm_utils_get_monotonic_timestamp*() are a fundamental utility function
that should work correctly in all cases. Such a low level function should
be thread safe.
The GChecksum API is cumbersome to use.
For example, g_checksum_get_digest() requires a length input/output
argument. At the same time, GChecksum does not allow you to query its
checksum-type nor the desired digest-length. When you have a GChecksum
at hand, you must always know the digest-length you are going to use.
So, the length parameter is only good for asserting.
Add a macro to make that more convenient.
Benefits: it's less lines of code, and we always do all the asserts
that are due.
- fix thread-safety by adding a memory barrier (g_atomic_pointer_get())
to the double-checked locking pattern when initializing the hash key.
- generate the random data outside the lock. Calling nm_utils_random_bytes()
within the lock is ugly, because we don't want to assume that the function
has no side effects which are prone to dead-lock. There is no problem attempting
to generate the random data without lock, and only use it when the race is won.
Often, during tests we want to assert against the logged messages.
In fact, most tests enable assertions for all logging and enforce
them with g_test_assert_expected_messages(). So, this is common.
However, sometimes it can be cumbersome to understand which logging
lines will be produced. For example, the next commits will call
nm_dhcp_manager_get() during the tests, which initializes NMDhcpManager
and logs a message which plugin was selected (or an additional warning,
if the selected plugin was not found). The availability of the DHCP plugin
depends on searching the path for "/usr/bin/dhclient", so from testing code
it's hard to determine what will be logged.
Instead, add a way to temporarily disable logging during testing.
If passed a relative path, load the editor .so from the same directory
as the plugin .so. This is useful for development, as it allows running
the editor plugin from the build tree conveniently.
https://github.com/NetworkManager/NetworkManager/pull/242
Refactor the check so that integer overflow cannot happen. Realistically,
it anyway couldn't happen, because _name is nowhere near the size of
G_MAXSIZE. Still, avoid such code. Also, the operands involved here are
constants, so the extra check can anyway be resolved at compile-time.
"shared/nm-utils" is a loose collection of utility functions.
There is a certain aim that they can be used independently.
However, they also rely on each other.
Add a test that we can build a minimal shared library with
these tools, independent of libnm-core.
This is independent functionality that only depends on linux API
and glib.
Note how "nm-logging" uses this for getting the timestamps. This
makes "nm-logging.c" itself dependen on "src/nm-core-utils.c",
for little reason.
1. NetworkManager-1.14.0/shared/nm-utils/nm-shared-utils.c:1242: value_overwrite: Overwriting previous write to "ch" with value from "v".
2. NetworkManager-1.14.0/shared/nm-utils/nm-shared-utils.c:1239: assigned_value: Assigning value from "++str[0]" to "ch" here, but that stored value is overwritten before it can be used.
# 1237| if (ch >= '0' && ch <= '7') {
# 1238| v = v * 8 + (ch - '0');
# 1239|-> ch = (++str)[0];
# 1240| }
# 1241| }
Don't assign ch when it is going to be overwritten.
keyfile already supports omitting the "connection.id" and
"connection.uuid". In that case, the ID would be taken from the
keyfile's name, and the UUID was generated by md5 hashing the
full filename.
No longer do this during nm_keyfile_read(), instead let all
callers call nm_keyfile_read_ensure_*() to their liking. This is done
for two reasons:
- a minor reason is, that one day we want to expose keyfile API
as public API. That means, we also want to read keyfiles from
stdin, where there is no filename available. The implementation
which parses stdio needs to define their own way of auto-generating
ID and UUID. Note how nm_keyfile_read()'s API no longer takes a
filename as argument, which would be awkward for the stdin case.
- Currently, we only support one keyfile directory, which (configurably)
is "/etc/NetworkManager/system-connections".
In the future, we want to support multiple keyfile dirctories, like
"/var/run/NetworkManager/profiles" or "/usr/lib/NetworkManager/profiles".
Here we want that a file "foo" (which does not specify a UUID) gets the
same UUID regardless of the directory it is in. That seems better, because
then the UUID won't change as you move the file between directories.
Yes, that means, that the same UUID will be provided by multiple
files, but NetworkManager must already cope with that situation anyway.
Unfortunately, the UUID generation scheme hashes the full path. That
means, we must hash the path name of the file "foo" inside the
original "system-connections" directory.
Refactor the code so that it accounds for a difference between the
filename of the keyfile, and the profile_dir used for generating
the UUID.
In general, it's fine to pass %NULL to g_free().
However, consider:
char *
foo (void)
{
gs_free char *value = NULL;
value = g_strdup ("hi");
return g_steal_pointer (&value);
}
gs_free, gs_local_free(), and g_steal_pointer() are all inlinable.
Here the compiler can easily recognize that we always pass %NULL to
g_free(). But with the previous implementation, the compiler would
not omit the call to g_free().
Similar patterns happen all over the place:
gboolean
baz (void)
{
gs_free char *value = NULL;
if (!some_check ())
return FALSE;
value = get_value ();
if (!value)
return FALSE;
return TRUE;
}
in this example, g_free() is only required after setting @value to
non-NULL.
Note that this does increase the binary side a bit (4k for libnm, 8k
for NetworkManager, with "-O2").
