The idea of NM_G_MUTEX_LOCKED() macro is not only to register a mutex
for unlocking (via nm_auto_unlock_g_mutex) but also to lock it at
the same time.
That is a useful helper macro. If you have to lock the mutex yourself,
it makes usage less convenient. At which point you don't need the macro
anymore and you should instead take full control and lock/unlock yourself.
Fix the macro and change behavior. The macro was not used so far, so
it's not a problem.
Fixes: dd33b3a14e ('shared: add nm_auto_unlock_g_mutex and NM_G_MUTEX_LOCKED() helper macros')
(cherry picked from commit 098ac7dbc0)
Under restricted permissions (like inside a podman container) opening
"/proc/self/ns/net" fails with Permission denied. Consequently we cannot
create our bottom NMPNetns instance. That is mostly fine, however we
would log an error message with severity <error>.
Note that test "src/core/platform/tests/test-platform-general" asserts
that no <warn> and <error> messages get logged. Hence, the test will
fail.
That is undesirable. Downgrade the message to <debug> so that the test
passes. Also, it's not clear that this error message is useful here.
Being unable to open a netns fd is fine and not necessarily an error
condition.
(cherry picked from commit 0213300dce)
We should always register the GArray stack with pthread
for cleanup the thread local storage. Do that first, before
creating the NMPNetns instance at the bottom of the stack.
(cherry picked from commit f9636080ac)
We call `tc` from iproute2, which commonly is at "/sbin/tc".
That might not be in the "$PATH" of a regular user, and consequently
we fail to run the test.
Work around that by always adding "/bin" and "/sbin" to the $PATH.
(cherry picked from commit f591aa41c6)
NMStrBuf's API is all about convenience. When you reset the buffer,
is it convenient to immediately append a new string?
It seems not. Make nm_str_buf_reset() simpler by doing only one thing.
We redefine G_VARIANT_TYPE() because we don't want to pay a run time
check for what should be a simple cast.
But this is strictly tied to glib, and it must only be done after glib.h
was included. Move it to the right place.
"nm-version-macros.h" is used directly by libnm-core and indirectly by
libnm and core.
Let's not have it randomly under shared/. Move it closer to where it's
used.
"shared/nm-meta-setting.[hc]" contains meta data about settings.
As such it is similarly used by libnm-core (as internal API) and
by clients (as extension of public API of libnm). However, it must
be compiled twice, because while it defines in both cases a
NMMetaSettingInfo type, these types are different between internal and
public API.
Hence, the files must also be compiled twice (and differently), once
against libnm-core and once against the client helper library.
Previously, the file was under "shared/", but there it's a bit odd
it doesn't clearly belong anywhere.
There are two goals here:
- copy the file to the two places where it is used. We also have
a "check-tree" unit test that ensures those files don't diverge in
the future.
- we no longer require CFLAGS set during built. Instead, the sources
should control the build. For that we have new (simple) headers
"nm-meta-setting-base.h" that define the right behavior for the
impl files.
There is still an ugliness (among several): the files must be named the
same for libnm-core and clients/common. Preferably, all our sources have
unique names, but that is not possible with this scheme (without
introducing other ugliness). To mitigate that, include the files only at
one exact place.
"nm-ethtool-utils.h" is part of public API in libnm(-core). It is also
used by shared/nm-base, which must have no dependency on libnm-core.
This was previously solved by symlinking the file. I find that error
prone, because the user might edit one file, without realizing that the
other file also changes.
Instead, copy the file and have it twice. Note that we have a unit test
which checks that both files are (and state) the same.
Copy+paste is a valid way of reusing code. By checking that the copy
does not diverge from the original, the downsides of copy+paste are
mitigated.
autotools projects commonly should include "config.h" as first header.
Also, commonly we need more headers, like glib.h or our nm_auto macros.
Hence, almost all our sources should as first include "nm-default.h".
However, as we build different parts, "nm-default.h" gets controlled
by the NETWORKMANAGER_COMPILATION define which autotools/meson needs
to specify in the build options.
That is confusing.
One advantage of that was, that theoretically the same sources can
be built twice, with different behavior. However, we should avoid doing
that altogether and build static libraries (once) that we link multiple
times.
Another advantage was that if NETWORKMANAGER_COMPILATION is for example
set to build a DAEMON source, there is a check that we don't include
private headers from libnm-core. However, that should be better solved
by not having public, internal and private headers in the same
directory.
Instead, introduce different "nm-default-*.h" headers that don't require
special defines and behave in a consistent way. This way, we require
fewer CFLAGS and it's immediately clear by looking at the source alone
which headers are included. Also, you will be easier see when a wrong
nm-default-*.h header gets included.
Introduce the first replacement. The others will follow.
Remove the check for NETWORKMANAGER_COMPILATION and NETWORKMANAGER_COMPILATION_TEST
define. We will no longer define that, and the check will be wrong.
NMUdevClient does not actually implement ref-counting, because it's not
used. Still, the destroy function was named nm_udev_client_unref(),
because theoretically then we could later, as the need arises, make
the type ref-counted. Then unref function already had the right name.
However, NMUdevClient also has a callback function that emits monitor
events. Again for simplicity, this callback function cannot be reset, it
can only be set once (in the constructor) and can also not be unset nor
disabled.
When the user of NMUdevClient is done with the instance and calls
"unref", then it must be sure that the callback is no longer invoked
afterwards. In practice that is already the case, but "unref" makes it
sound as if somebody else could also still hold a reference -- in which
case the user would have to first unset/disable the callback.
Rename the function to "destroy()", so that it's clear that the instance
is gone afterwards and that the callback will not be invoked anymore.
Drop some "helper" variables that are only used once. These variables
spread out what is defined, and only make the meson file more complicated
to follow.
We have g_idle_add() and g_timeout_add(). But these return
those odd guint source ids. That is totally pointless. The
only potential benefit is that a guint is only 4 bytes while
a pointer is 8 bytes (on 64 bit systems). Otherwise, it seems
always preferable to have an actual GSource instance instead
of an integer. It also saves the overhead in g_source_remove()
which first needs to do a hash lookup to find the GSource.
A GSource instance would theoretically work with multiple
GMainContext instances, while g_source_remove() only works
wit g_main_context_default().
On the other hand we have helper API like nm_g_idle_source_new()
and nm_g_timeout_source_new(), which is fully flexible and sensible
because it returns a reference to the GSource instance. However, it
is a bit verbose to use in the common case.
Add helper functions that simplify the use and are conceptionally
similar to g_{idle,timeout}_add() (hence the name).
The prepare() step of the GSource called frequently when the outer
GMainContext is preparing.
In the common case we have
- few file descriptors in the inner context to track
- the file descriptors don't change
We also need to consider whether the file descriptors change, because
some book-keeping is necessary when they do. But usually they don't
change.
Hence, let's optimize the prepare step to avoid a heap allocation
in the common case.
Also, because we use nm_utils_g_main_context_create_integrate_source()
with the internal GMainContext of NMClient. That context always has
a small number of file descriptors to track and it doesn't see much
change. In the vast majority of cases, the heap allocation can be
avoided.
With LTO enabled, the compiler might think that "len" in n_dhcp4_socket_packet_send()
might be uninitialized. That is even a correct assumption, as the compiler does not
understand the API of sendmsg() and that sendmsg() is supposed to set a positive errno.
Work around by using c_errno() in packet_sendto_udp().
shared/n-dhcp4/src/n-dhcp4-c-connection.c: In function n_dhcp4_c_connection_send_request:
shared/n-dhcp4/src/n-dhcp4-socket.c:368:19: error: len may be used uninitialized in this function [-Werror=maybe-uninitialized]
} else if (len != n_buf) {
^
shared/n-dhcp4/src/n-dhcp4-socket.c:351:23: note: len was declared here
size_t n_buf, len;
^
NMTernary is part of libnm's public API. It thus cannot be used by code
without libnm/libnm-core dependency.
Add another enum with the same purpose.
The name "NMTernary" is already taken, and we should not use some macro
trickery to use (effectively) different types under the same name.
Another possible name would be "NMTern", but for no strong reasons
we choose NMOptionBool. The naming reminds of rust's std::option::Option.
We want to use this by "shared/nm-platform", which should have
no dependency on "libnm-core".
Move "libnm-core/nm-ethtool-utils.h" to "libnm/nm-ethtool-utils.h" so
that it is only used by libnm. This file contains the defines for
the option names.
Also, symlink "libnm/nm-ethtool-utils.h" as "shared/nm-base/nm-ethtool-utils-base.h".
We want to use the same defines also internally. Since they are both
public API (must be in libnm) and should be in "shared/nm-base", this
is the way.
Currently src/platform depends on libnm-core. libnm-core is large
optimally we have a better separation between our code. That means
libnm-core does not depend on platform and vice versa.
However, nm-platform re-uses some enums from libnm-core for internal code.
To avoid that dependency, add _NMSettingWiredWakeOnLan as a duplicate to
nm-base/nm-base.h. nm-base can both be used by libnm-core, nm-platform
and src/platform.
The only problem is that NMSettingWiredWakeOnLan is also part of public
API of libnm. It means, we must duplicate the enum. But with several
static assertions in unit tests I think that is not a problem to do.
