- move the main func declarations to nmcli.h and give them a common
prefix "nmc_command_func_" prefix.
- remove some of the header files that are now empty. In fact, these
headers did not really declare some well separated module. While we
probably should structure the code in nmcli better with better layering,
it was not and still is not. Having these dummy headers don't mean that
the code is well structured and they serve little purpose.
- move the static NMCommand lists variables into the function scope
where they are used.
Our own implementation of a string buffer like GString.
Advantages (in decreasing relevance):
- Since we are in control, we can easily let it nm_explicit_bzero()
the memory. The regular GString API cannot be used in such a case.
While nm_explicit_bzero() may or may not be of questionable benefit,
the problem is that if the underlying API counteracts the aim of
clearing memory, it gets impossible. As API like NMStrBuf supports
it, clearing memory is a easy as enable the right flag.
This would for example be useful for example when we read passwords
from a file or file descriptor (e.g. try_spawn_vpn_auth_helper()).
- We have API like
nmp_object_to_string (const NMPObject *obj,
NMPObjectToStringMode to_string_mode,
char *buf,
gsize buf_size);
which accept a fixed size output buffer. This has the problem of
how choosing the right sized buffer. With NMStrBuf such API could
be instead
nmp_object_to_string (const NMPObject *obj,
NMPObjectToStringMode to_string_mode,
NMStrBuf *buf);
which can automatically grow (using heap allocation). It would be
easy to extend NMStrBuf to use a fixed buffer or limiting the
maximum string length. The point is, that the to-string API wouldn't
have to change. Depending on the NMStrBuf passed in, you can fill
an unbounded heap allocated string, a heap allocated string up to
a fixed length, or a static string of fixed length. NMStrBuf currently
only implements the unbounded heap allocate string case, but it would
be simple to extend.
Note that we already have API like nm_utils_strbuf_*() to fill a buffer
of fixed size. GString is not useable for that (efficiently), hence
this API exists. NMStrBuf could be easily extended to replace this API
without usability or performance penalty. So, while this adds one new
API, it could replace other APIs.
- GString always requires a heap allocation for the container. In by far
most of the cases where we use GString, we use it to simply construct
a string dynamically. There is zero use for this overhead. If one
really needs a heap allocated buffer, NMStrBuf can easily embedded
in a malloc'ed memory and boxed that way.
- GString API supports inserting and removing range. We almost never
make use of that. We only require append-only, which is simple to
implement.
- GString needs to NUL terminate the buffer on every append. It
has unnecessary overhead for allowing a usage of where intermediate
buffer contents are valid strings too. That is not the case with
NMStrBuf: the API requires the user to call nm_str_buf_get_str() or
nm_str_buf_finalize(). In most cases, you would only access the string
once at the end, and not while constructing it.
- GString always grows the buffer size by doubling it. I don't think
that is optimal. I don't think there is one optimal approach for how
to grow the buffer, it depends on the usage patterns. However, trying
to make an optimal choice here makes a difference. QT also thinks so,
and I adopted their approach in nm_utils_get_next_realloc_size().
Our glib based code should also include our static utility library
libnm-glib-aux. This is our basic utility library that we want to
have around everywhere. Since we link statically, the linker will weed
out the unused stuff at compile time. So, there is no overhead, except
for the things that we actually use.
The advantage is that the API is now the same for IPv4 and IPv6: it's
all nm_dhcp_config_*() and we can (easier) treat the address family
generically.
We still need two distinct GObject types, mainly because of the
glue code for exposing the object on D-Bus as NMDBusObject. Of course,
that could be solved differently, but as it is, it's quite nice.
With `./configure --enable-more-asserts`, we add extra -W flags to
AM_CFLAGS. This variable is only used, if the per-library override
libnm_core_libnm_core_la_CFLAGS is unspecified ([1]).
Usually we avoid this problem be never specifying library_CFLAGS, but
placing all our per-library flags to library_CPPFLAGS. While that is a
bit of a hack and misuse of CPPFLAGS, it works well (enough).
This was broken recently. The effect was, that libnm-core was not
build with AM_CFLAGS flags. Fix it.
[1] https://www.gnu.org/software/automake/manual/html_node/Flag-Variables-Ordering.html
Fixes: d2d6a68697 ('build: use -fcommon when building libnm-core')
Fix the following warning:
Makefile.am:3671: warning: $(src_devices_wifi_libnm_device_plugin_wifi_la_OBJECTS) was already defined in condition TRUE, which includes condition WITH_WIFI ...
Makefile.am:1075: ... '$(src_devices_wifi_libnm_device_plugin_wifi_la_OBJECTS)' previously defined here
Building with GCC 10 gives the following error:
multiple definition of_nm_jansson_json_object_iter_key';
libnm/.libs/liblibnm.a(libnm_core_la-nm-json.o):/builddir/build/BUILD/NetworkManager-1.23.1/libnm-core/nm-json.c:24: first defined here /usr/bin/ld:
libnm/.libs/liblibnm.a(libnm_core_la-nm-team-utils.o):/usr/include/jansson.h:202: multiple definition of _nm_jansson_json_object_iter';
This happens because GCC 10 defaults to -fno-common and so multiple
definitions of the same global variable are not merged together.
_nm_jansson_json_* symbols are defined in nm-json.c as void pointers
and, due to the following macros in nm-json.h:
#define json_object_iter_next (*_nm_jansson_json_object_iter_next)
...
the function declaration in jansson.h:
void *json_object_iter_next(json_t *object, void *iter);
becomes a global variable as well:
void *(*_nm_jansson_json_object_iter_next)(json_t *object, void *iter);
So, the symbol is present in nm-json.o and all other object files that
include nm-json.h, and -fcommon is required. Without it, it would be
necessary to define the symbols only in one place (for example,
nm-json.c), but then static inline functions from the jannson.h header
would still refer to the original (missing) jansson functions.
For the moment, just use -fcommon.
On ppc64le, the linking fails due to unresolved symbols.
Fixes: 7d8da6c9c1 ('build: build intermediate library with core wifi for device-plugin and tests')
Add VRF support to the daemon. When the device we are activating is a
VRF or a VRF's slave, put routes in the table specified by the VRF
connection.
Also, introduce a VRF device type in libnm.
Don't build the same sources multiple times. The test code should
statically link against the tested code, just like the device plugin
that uses the code in production.
Keyfile support was initially added under GPL-2.0+ license as part of
core. It was moved to "libnm-core" in commit 59eb5312a5 ('keyfile: merge
branch 'th/libnm-keyfile-bgo744699'').
"libnm-core" is statically linked with by core and "libnm". In
the former case under terms of GPL-2.0+ (good) and in the latter case
under terms of LGPL-2.1+ (bad).
In fact, to this day, "libnm" doesn't actually use the code. The linker
will probably remove all the GPL-2.0+ symbols when compiled with
gc-sections or LTO. Still, linking them together in the first place
makes "libnm" only available under GPL code (despite the code
not actually being used).
Instead, move the GPL code to a separate static library
"shared/nm-keyfile/libnm-keyfile.la" and only link it to the part
that actually uses the code (and which is GPL licensed too).
This fixes the license violation.
Eventually, it would be very useful to be able to expose keyfile
handling via "libnm". However that is not straight forward due to the
licensing conflict.
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/merge_requests/381
Move it to shared as it's useful for clients as well.
Move and rename nm_dbus_manager_new_auth_subject_from_context() and
nm_dbus_manager_new_auth_subject_from_message() in nm-dbus-manager.c
as they're needed there.
Using sync init (nm_client_new()) has an overhead as it requires an internal
GMainContext to ensure preserving the order of D-Bus messages. Let's avoid
that by using the async init. Note that the difference here is that we will
iterate the caller's GMainContext while creating the instance. But that
is no problem for nmtui at that point.
We have "shared/nm-libnm-core-aux", which is shared code that can be used
by anybody (including libnm-core, src, libnm and clients).
We have "clients/common", which are helper function for clients. But
that implies that the code is inside "clients". I think it would be
useful to have auxiliary code that extends libnm, but is not only
usable by code in "clients". In other words, "shared/nm-libnm-aux"
is a better place than "clients/common", and I think most of the
functionality form "clients/common" should move there.
"shared/nm-utils" got long renamed and split into separate parts. The remaining
tests are really to test nm-std-aux and nm-glib-aux (no libnm dependencies). Move
the tests to the appropriate place.
We have "nm-logging-fwd.h", which (as the name implies) is header-only.
Add instead a "nm-logging-base.c", which also contains implementation for
logging functions that are not only useful under "src/nm-logging.c"
No longer use GDBusObjectMangaerClient and gdbus-codegen generated classes
for the NMClient cache. Instead, use GDBusConnection directly and a
custom implementation (NMLDBusObject) for caching D-Bus' ObjectManager
data.
CHANGES
-------
- This is a complete rework. I think the previous implementation was
difficult to understand. There were unfixed bugs and nobody understood
the code well enough to fix them. Maybe somebody out there understood the
code, but I certainly did not. At least nobody provided patches to fix those
issues. I do believe that this implementation is more straightforward and
easier to understand. It removes a lot of layers of code. Whether this claim
of simplicity is true, each reader must decide for himself/herself. Note
that it is still fairly complex.
- There was a lingering performance issue with large number of D-Bus
objects. The patch tries hard that the implementation scales well. Of
course, when we cache N objects that have N-to-M references to other,
we still are fundamentally O(N*M) for runtime and memory consumption (with
M being the number of references between objects). But each part should behave
efficiently and well.
- Play well with GMainContext. libnm code (NMClient) is generally not
thread safe. However, it should work to use multiple instances in
parallel, as long as each access to a NMClient is through the caller's
GMainContext. This follows glib's style and effectively allows to use NMClient
in a multi threaded scenario. This implies to stick to a main context
upon construction and ensure that callbacks are only invoked when
iterating that context. Also, NMClient itself shall never iterate the
caller's context. This also means, libnm must never use g_idle_add() or
g_timeout_add(), as those enqueue sources in the g_main_context_default()
context.
- Get ordering of messages right. All events are consistently enqueued
in a GMainContext and processed strictly in order. For example,
previously "nm-object.c" tried to combine signals and emit them on an
idle handler. That is wrong, signals must be emitted in the right order
and when they happen. Note that when using GInitable's synchronous initialization
to initialize the NMClient instance, NMClient internally still operates fully
asynchronously. In that case NMClient has an internal main context.
- NMClient takes over most of the functionality. When using D-Bus'
ObjectManager interface, one needs to handle basically the entire state
of the D-Bus interface. That cannot be separated well into distinct
parts, and even if you try, you just end up having closely related code
in different source files. Spreading related code does not make it
easier to understand, on the contrary. That means, NMClient is
inherently complex as it contains most of the logic. I think that is
not avoidable, but it's not as bad as it sounds.
- NMClient processes D-Bus messages and state changes in separate steps.
First NMClient unpacks the message (e.g. _dbus_handle_properties_changed()) and
keeps track of the changed data. Then we update the GObject instances
(_dbus_handle_obj_changed_dbus()) without emitting any signals yet. Finally,
we emit all signals and notifications that were collected
(_dbus_handle_changes_commit()). Note that for example during the initial
GetManagedObjects() reply, NMClient receive a large amount of state at once.
But we first apply all the changes to our GObject instances before
emitting any signals. The result is that signals are always emitted in a moment
when the cache is consistent. The unavoidable downside is that when you receive
a property changed signal, possibly many other properties changed
already and more signals are about to be emitted.
- NMDeviceWifi no longer modifies the content of the cache from client side
during poke_wireless_devices_with_rf_status(). The content of the cache
should be determined by D-Bus alone and follow what NetworkManager
service exposes. Local modifications should be avoided.
- This aims to bring no API/ABI change, though it does of course bring
various subtle changes in behavior. Those should be all for the better, but the
goal is not to break any existing clients. This does change internal
(albeit externally visible) API, like dropping NM_OBJECT_DBUS_OBJECT_MANAGER
property and NMObject no longer implementing GInitableIface and GAsyncInitableIface.
- Some uses of gdbus-codegen classes remain in NMVpnPluginOld, NMVpnServicePlugin
and NMSecretAgentOld. These are independent of NMClient/NMObject and
should be reworked separately.
- While we no longer use generated classes from gdbus-codegen, we don't
need more glue code than before. Also before we constructed NMPropertiesInfo and
a had large amount of code to propagate properties from NMDBus* to NMObject.
That got completely reworked, but did not fundamentally change. You still need
about the same effort to create the NMLDBusMetaIface. Not using
generated bindings did not make anything worse (which tells about the
usefulness of generated code, at least in the way it was used).
- NMLDBusMetaIface and other meta data is static and immutable. This
avoids copying them around. Also, macros like NML_DBUS_META_PROPERTY_INIT_U()
have compile time checks to ensure the property types matches. It's pretty hard
to misuse them because it won't compile.
- The meta data now explicitly encodes the expected D-Bus types and
makes sure never to accept wrong data. That would only matter when the
server (accidentally or intentionally) exposes unexpected types on
D-Bus. I don't think that was previously ensured in all cases.
For example, demarshal_generic() only cared about the GObject property
type, it didn't know the expected D-Bus type.
- Previously GDBusObjectManager would sometimes emit warnings (g_log()). Those
probably indicated real bugs. In any case, it prevented us from running CI
with G_DEBUG=fatal-warnings, because there would be just too many
unrelated crashes. Now we log debug messages that can be enabled with
"LIBNM_CLIENT_DEBUG=trace". Some of these messages can also be turned
into g_warning()/g_critical() by setting LIBNM_CLIENT_DEBUG=warning,error.
Together with G_DEBUG=fatal-warnings, this turns them into assertions.
Note that such "assertion failures" might also happen because of a server
bug (or change). Thus these are not common assertions that indicate a bug
in libnm and are thus not armed unless explicitly requested. In our CI we
should now always run with LIBNM_CLIENT_DEBUG=warning,error and
G_DEBUG=fatal-warnings and to catch bugs. Note that currently
NetworkManager has bugs in this regard, so enabling this will result in
assertion failures. That should be fixed first.
- Note that this changes the order in which we emit "notify:devices" and
"device-added" signals. I think it makes the most sense to emit first
"device-removed", then "notify:devices", and finally "device-added"
signals.
This changes behavior for commit 52ae28f6e5 ('libnm: queue
added/removed signals and suppress uninitialized notifications'),
but I don't think that users should actually rely on the order. Still,
the new order makes the most sense to me.
- In NetworkManager, profiles can be invisible to the user by setting
"connection.permissions". Such profiles would be hidden by NMClient's
nm_client_get_connections() and their "connection-added"/"connection-removed"
signals.
Note that NMActiveConnection's nm_active_connection_get_connection()
and NMDevice's nm_device_get_available_connections() still exposes such
hidden NMRemoteConnection instances. This behavior was preserved.
NUMBERS
-------
I compared 3 versions of libnm.
[1] 962297f908, current tip of nm-1-20 branch
[2] 4fad8c7c64, current master, immediate parent of this patch
[3] this patch
All tests were done on Fedora 31, x86_64, gcc 9.2.1-1.fc31.
The libraries were build with
$ ./contrib/fedora/rpm/build_clean.sh -g -w test -W debug
Note that RPM build already stripped the library.
---
N1) File size of libnm.so.0.1.0 in bytes. There currently seems to be a issue
on Fedora 31 generating wrong ELF notes. Usually, libnm is smaller but
in these tests it had large (and bogus) ELF notes. Anyway, the point
is to show the relative sizes, so it doesn't matter).
[1] 4075552 (102.7%)
[2] 3969624 (100.0%)
[3] 3705208 ( 93.3%)
---
N2) `size /usr/lib64/libnm.so.0.1.0`:
text data bss dec hex filename
[1] 1314569 (102.0%) 69980 ( 94.8%) 10632 ( 80.4%) 1395181 (101.4%) 1549ed /usr/lib64/libnm.so.0.1.0
[2] 1288410 (100.0%) 73796 (100.0%) 13224 (100.0%) 1375430 (100.0%) 14fcc6 /usr/lib64/libnm.so.0.1.0
[3] 1229066 ( 95.4%) 65248 ( 88.4%) 13400 (101.3%) 1307714 ( 95.1%) 13f442 /usr/lib64/libnm.so.0.1.0
---
N3) Performance test with test-client.py. With checkout of [2], run
```
prepare_checkout() {
rm -rf /tmp/nm-test && \
git checkout -B test 4fad8c7c64 && \
git clean -fdx && \
./autogen.sh --prefix=/tmp/nm-test && \
make -j 5 install && \
make -j 5 check-local-clients-tests-test-client
}
prepare_test() {
NM_TEST_REGENERATE=1 NM_TEST_CLIENT_BUILDDIR="/data/src/NetworkManager" NM_TEST_CLIENT_NMCLI_PATH=/usr/bin/nmcli python3 ./clients/tests/test-client.py -v
}
do_test() {
for i in {1..10}; do
NM_TEST_CLIENT_BUILDDIR="/data/src/NetworkManager" NM_TEST_CLIENT_NMCLI_PATH=/usr/bin/nmcli python3 ./clients/tests/test-client.py -v || return -1
done
echo "done!"
}
prepare_checkout
prepare_test
time do_test
```
[1] real 2m14.497s (101.3%) user 5m26.651s (100.3%) sys 1m40.453s (101.4%)
[2] real 2m12.800s (100.0%) user 5m25.619s (100.0%) sys 1m39.065s (100.0%)
[3] real 1m54.915s ( 86.5%) user 4m18.585s ( 79.4%) sys 1m32.066s ( 92.9%)
---
N4) Performance. Run NetworkManager from build [2] and setup a large number
of profiles (551 profiles and 515 devices, mostly unrealized). This
setup is already at the edge of what NetworkManager currently can
handle. Of course, that is a different issue. Here we just check how
long plain `nmcli` takes on the system.
```
do_cleanup() {
for UUID in $(nmcli -g NAME,UUID connection show | sed -n 's/^xx-c-.*:\([^:]\+\)$/\1/p'); do
nmcli connection delete uuid "$UUID"
done
for DEVICE in $(nmcli -g DEVICE device status | grep '^xx-i-'); do
nmcli device delete "$DEVICE"
done
}
do_setup() {
do_cleanup
for i in {1..30}; do
nmcli connection add type bond autoconnect no con-name xx-c-bond-$i ifname xx-i-bond-$i ipv4.method disabled ipv6.method ignore
for j in $(seq $i 30); do
nmcli connection add type vlan autoconnect no con-name xx-c-vlan-$i-$j vlan.id $j ifname xx-i-vlan-$i-$j vlan.parent xx-i-bond-$i ipv4.method disabled ipv6.method ignore
done
done
systemctl restart NetworkManager.service
sleep 5
}
do_test() {
perf stat -r 50 -B nmcli 1>/dev/null
}
do_test
```
[1]
Performance counter stats for 'nmcli' (50 runs):
456.33 msec task-clock:u # 1.093 CPUs utilized ( +- 0.44% )
0 context-switches:u # 0.000 K/sec
0 cpu-migrations:u # 0.000 K/sec
5,900 page-faults:u # 0.013 M/sec ( +- 0.02% )
1,408,675,453 cycles:u # 3.087 GHz ( +- 0.48% )
1,594,741,060 instructions:u # 1.13 insn per cycle ( +- 0.02% )
368,744,018 branches:u # 808.061 M/sec ( +- 0.02% )
4,566,058 branch-misses:u # 1.24% of all branches ( +- 0.76% )
0.41761 +- 0.00282 seconds time elapsed ( +- 0.68% )
[2]
Performance counter stats for 'nmcli' (50 runs):
477.99 msec task-clock:u # 1.088 CPUs utilized ( +- 0.36% )
0 context-switches:u # 0.000 K/sec
0 cpu-migrations:u # 0.000 K/sec
5,948 page-faults:u # 0.012 M/sec ( +- 0.03% )
1,471,133,482 cycles:u # 3.078 GHz ( +- 0.36% )
1,655,275,369 instructions:u # 1.13 insn per cycle ( +- 0.02% )
382,595,152 branches:u # 800.433 M/sec ( +- 0.02% )
4,746,070 branch-misses:u # 1.24% of all branches ( +- 0.49% )
0.43923 +- 0.00242 seconds time elapsed ( +- 0.55% )
[3]
Performance counter stats for 'nmcli' (50 runs):
352.36 msec task-clock:u # 1.027 CPUs utilized ( +- 0.32% )
0 context-switches:u # 0.000 K/sec
0 cpu-migrations:u # 0.000 K/sec
4,790 page-faults:u # 0.014 M/sec ( +- 0.26% )
1,092,341,186 cycles:u # 3.100 GHz ( +- 0.26% )
1,209,045,283 instructions:u # 1.11 insn per cycle ( +- 0.02% )
281,708,462 branches:u # 799.499 M/sec ( +- 0.01% )
3,101,031 branch-misses:u # 1.10% of all branches ( +- 0.61% )
0.34296 +- 0.00120 seconds time elapsed ( +- 0.35% )
---
N5) same setup as N4), but run `PAGER= /bin/time -v nmcli`:
[1]
Command being timed: "nmcli"
User time (seconds): 0.42
System time (seconds): 0.04
Percent of CPU this job got: 107%
Elapsed (wall clock) time (h:mm:ss or m:ss): 0:00.43
Average shared text size (kbytes): 0
Average unshared data size (kbytes): 0
Average stack size (kbytes): 0
Average total size (kbytes): 0
Maximum resident set size (kbytes): 34456
Average resident set size (kbytes): 0
Major (requiring I/O) page faults: 0
Minor (reclaiming a frame) page faults: 6128
Voluntary context switches: 1298
Involuntary context switches: 1106
Swaps: 0
File system inputs: 0
File system outputs: 0
Socket messages sent: 0
Socket messages received: 0
Signals delivered: 0
Page size (bytes): 4096
Exit status: 0
[2]
Command being timed: "nmcli"
User time (seconds): 0.44
System time (seconds): 0.04
Percent of CPU this job got: 108%
Elapsed (wall clock) time (h:mm:ss or m:ss): 0:00.44
Average shared text size (kbytes): 0
Average unshared data size (kbytes): 0
Average stack size (kbytes): 0
Average total size (kbytes): 0
Maximum resident set size (kbytes): 34452
Average resident set size (kbytes): 0
Major (requiring I/O) page faults: 0
Minor (reclaiming a frame) page faults: 6169
Voluntary context switches: 1849
Involuntary context switches: 142
Swaps: 0
File system inputs: 0
File system outputs: 0
Socket messages sent: 0
Socket messages received: 0
Signals delivered: 0
Page size (bytes): 4096
Exit status: 0
[3]
Command being timed: "nmcli"
User time (seconds): 0.32
System time (seconds): 0.02
Percent of CPU this job got: 102%
Elapsed (wall clock) time (h:mm:ss or m:ss): 0:00.34
Average shared text size (kbytes): 0
Average unshared data size (kbytes): 0
Average stack size (kbytes): 0
Average total size (kbytes): 0
Maximum resident set size (kbytes): 29196
Average resident set size (kbytes): 0
Major (requiring I/O) page faults: 0
Minor (reclaiming a frame) page faults: 5059
Voluntary context switches: 919
Involuntary context switches: 685
Swaps: 0
File system inputs: 0
File system outputs: 0
Socket messages sent: 0
Socket messages received: 0
Signals delivered: 0
Page size (bytes): 4096
Exit status: 0
---
N6) same setup as N4), but run `nmcli monitor` and look at `ps aux` for
the RSS size.
USER PID %CPU %MEM VSZ RSS TTY STAT START TIME COMMAND
[1] me 1492900 21.0 0.2 461348 33248 pts/10 Sl+ 15:02 0:00 nmcli monitor
[2] me 1490721 5.0 0.2 461496 33548 pts/10 Sl+ 15:00 0:00 nmcli monitor
[3] me 1495801 16.5 0.1 459476 28692 pts/10 Sl+ 15:04 0:00 nmcli monitor
The dependencies of make are exectured in the order as they appear.
We probably should start by creating the directories, before invoking
other install hooks. Currently there is no difference, because none of
the other hooks depend on the base directories. Still split it to
a special target.
$(data_edit) will be used later at an earlier place in the
makefile (to edit "clients/cloud-setup/nm-cloud-setup.service",
which will be handled earlier). Move it.
Also minor cleanups, like allowing to incrementally build
systemdsystemunit_DATA variable.
A quick overview of the currently connected Wi-Fi network, including
credentials. Comes handy if someone wants to connect more devices to
their Hotspot or the same network as they are connected to.
The devices tests' meson build files include only the build of a
single executable file and its execution as a test unit.
This has been moved to the devices' main meson build files so this
files can be removed.
In 878d4963e a new `nm-bt-test` helper program was added. However,
although `autotools` build steps were included, meson build steps
were not.
This add meson's build steps.
Makefile.am:3462: warning: $(src_devices_bluetooth_libnm_device_plugin_bluetooth_la_OBJECTS) was already defined in condition TRUE, which includes condition WITH_MODEM_MANAGER_1 ...
Makefile.am:960: ... '$(src_devices_bluetooth_libnm_device_plugin_bluetooth_la_OBJECTS)' previously defined here
Fixes: 878d4963ed ('bluetooth/tests: add "nm-bt-test helper" program for manual testing of bluetooth code')
This is a complete refactoring of the bluetooth code.
Now that BlueZ 4 support was dropped, the separation of NMBluezManager
and NMBluez5Manager makes no sense. They should be merged.
At that point, notice that BlueZ 5's D-Bus API is fully centered around
D-Bus's ObjectManager interface. Using that interface, we basically only
call GetManagedObjects() once and register to InterfacesAdded,
InterfacesRemoved and PropertiesChanged signals. There is no need to
fetch individual properties ever.
Note how NMBluezDevice used to query the D-Bus properties itself by
creating a GDBusProxy. This is redundant, because when using the ObjectManager
interfaces, we have all information already.
Instead, let NMBluezManager basically become the client-side cache of
all of BlueZ's ObjectManager interface. NMBluezDevice was mostly concerned
about caching the D-Bus interface's state, tracking suitable profiles
(pan_connection), and moderate between bluez and NMDeviceBt.
These tasks don't get simpler by moving them to a seprate file. Let them
also be handled by NMBluezManager.
I mean, just look how it was previously: NMBluez5Manager registers to
ObjectManager interface and sees a device appearing. It creates a
NMBluezDevice object and registers to its "initialized" and
"notify:usable" signal. In the meantime, NMBluezDevice fetches the
relevant information from D-Bus (although it was already present in the
data provided by the ObjectManager) and eventually emits these usable
and initialized signals.
Then, NMBlue5Manager emits a "bdaddr-added" signal, for which NMBluezManager
creates the NMDeviceBt instance. NMBluezManager, NMBluez5Manager and
NMBluezDevice are strongly cooperating to the point that it is simpler
to merge them.
This is not mere refactoring. This patch aims to make everything
asynchronously and always cancellable. Also, it aims to fix races
and inconsistencies of the state.
- Registering to a NAP server now waits for the response and delays
activation of the NMDeviceBridge accordingly.
- For NAP connections we now watch the bnep0 interface in platform, and tear
down the device when it goes away. Bluez doesn't send us a notification
on D-Bus in that case.
- Rework establishing a DUN connection. It no longer uses blocking
connect() and does not block until rfcomm device appears. It's
all async now. It also watches the rfcomm file descriptor for
POLLERR/POLLHUP to notice disconnect.
- drop nm_device_factory_emit_component_added() and instead let
NMDeviceBt directly register to the WWan factory's "added" signal.
I'd like to refactor libnm's caching. Note that cached D-Bus objects
have repeated strings all over the place. For example every object will
have a set of D-Bus interfaces (strings) and properties (strings) and an
object path (which is referenced by other objects). We can save a lot of
redundant strings by deduplicating/interning them. Also, by interning
them, we can compare them using pointer equality.
Add a NMRefString implementation for this.
Maybe an alternative name would be NMInternedString or NMDedupString, because
this string gets always interned. There is no way to create a NMRefString
that is not interned. Still, NMRefString name sounds better. It is ref-counted
after all.
Notes:
- glib has GQuark and g_intern_string(). However, such strings cannot
be unrefered and are leaked indefinitely. It is thus unsuited for
anything but a fixed set of well-known strings.
- glib 2.58 adds GRefString, but we cannot use that because we
currently still use glib 2.40.
There are some differences:
- GRefString is just a typedef to char. That means, the glib API
exposes GRefString like regular character strings.
NMRefString intentionally does that not. This makes it slightly
less convenient to pass it to API that expects "const char *".
But it makes it clear to the reader, that an instance is in fact
a NMRefString, which means it indicates that the string is
interned and can be referenced without additional copy.
- GRefString can be optionally interned. That means you can
only use pointer equality for comparing values if you know
that the GRefString was created with g_ref_string_new_intern().
So, GRefString looks like a "const char *" pointer and even if
you know it's a GRefString, you might not know whether it is
interned. NMRefString is always interned, and you can always
compare it using pointer equality.
- In the past I already proposed a different implementation for a
ref-string. That made different choices. For example NMRefString
then was a typedef to "const char *", it did not support interning
but deduplication (without a global cache), ref/unref was not
thread safe (but then there was no global cache so that two threads
could still use the API independently).
The point is, there are various choices to make. GRefString, the
previous NMRefString implementation and the one here, all have pros and
cons. I think for the purpose where I intend NMRefString (dedup and
efficient comparison), it is a preferable implementation.
Ah, and of course NMRefString is an immutable string, which is a nice
property.
Fixes build:
In file included from ../src/devices/wwan/nm-service-providers.c:10:
In file included from ../shared/nm-default.h:279:
../libnm-core/nm-core-types.h:14:10: fatal error: 'nm-core-enum-types.h' file not found
#include "nm-core-enum-types.h"
^
1 error generated.
make[2]: *** [src/devices/wwan/src_devices_wwan_tests_test_service_providers-nm-service-providers.o] Error 1
This adds capability to hand over the network configuration from
OpenFirmware (and potentially other boot loaders with openfirmware
support such as U-Boot) to NetworkManager.
It's done analogously to ACPI/iBFT. In fact, the same ip=ibft command
line option is used, adding a more general ip=fw alias. This probably
deserves some documentation, but I'm not adding any at this time.
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/merge_requests/257
gtkdoc-scan supports regular expressions in the --ignore-decorators
command-line option. Since it is easier to use a regexp than grepping
macros from a source file, revert the ugly solution from commit
2d941dc95a ('build: fix errors when building with gtk-doc 1.32').
This file causes a crash [1], add it to the tests.
Note that the test only check parsing the file and the
crash happens in the "upper" layers. So, it's not really
a test for the crash. But at least have such a file in
our repository.
[1] https://gitlab.freedesktop.org/NetworkManager/NetworkManager/issues/235
BlueZ 5.0 was released in December 2012 and broke API with
BlueZ 4. NetworkManager supports Bluez 5 for years already.
Of course, version 4 is long gone by now, so remove it.
Completely rework how settings plugin handle connections and how
NMSettings tracks the list of connections.
Previously, settings plugins would return objects of (a subtype of) type
NMSettingsConnection. The NMSettingsConnection was tightly coupled with
the settings plugin. That has a lot of downsides.
Change that. When changing this basic relation how settings connections
are tracked, everything falls appart. That's why this is a huge change.
Also, since I have to largely rewrite the settings plugins, I also
added support for multiple keyfile directories, handle in-memory
connections only by keyfile plugin and (partly) use copy-on-write NMConnection
instances. I don't want to spend effort rewriting large parts while
preserving the old way, that anyway should change. E.g. while rewriting ifcfg-rh,
I don't want to let it handle in-memory connections because that's not right
long-term.
--
If the settings plugins themself create subtypes of NMSettingsConnection
instances, then a lot of knowledge about tracking connections moves
to the plugins.
Just try to follow the code what happend during nm_settings_add_connection().
Note how the logic is spread out:
- nm_settings_add_connection() calls plugin's add_connection()
- add_connection() creates a NMSettingsConnection subtype
- the plugin has to know that it's called during add-connection and
not emit NM_SETTINGS_PLUGIN_CONNECTION_ADDED signal
- NMSettings calls claim_connection() which hocks up the new
NMSettingsConnection instance and configures the instance
(like calling nm_settings_connection_added()).
This summary does not sound like a lot, but try to follow that code. The logic
is all over the place.
Instead, settings plugins should have a very simple API for adding, modifying,
deleting, loading and reloading connections. All the plugin does is to return a
NMSettingsStorage handle. The storage instance is a handle to identify a profile
in storage (e.g. a particular file). The settings plugin is free to subtype
NMSettingsStorage, but it's not necessary.
There are no more events raised, and the settings plugin implements the small
API in a straightforward manner.
NMSettings now drives all of this. Even NMSettingsConnection has now
very little concern about how it's tracked and delegates only to NMSettings.
This should make settings plugins simpler. Currently settings plugins
are so cumbersome to implement, that we avoid having them. It should not be
like that and it should be easy, beneficial and lightweight to create a new
settings plugin.
Note also how the settings plugins no longer care about duplicate UUIDs.
Duplicated UUIDs are a fact of life and NMSettings must handle them. No
need to overly concern settings plugins with that.
--
NMSettingsConnection is exposed directly on D-Bus (being a subtype of
NMDBusObject) but it was also a GObject type provided by the settings
plugin. Hence, it was not possible to migrate a profile from one plugin to
another.
However that would be useful when one profile does not support a
connection type (like ifcfg-rh not supporting VPN). Currently such
migration is not implemented except for migrating them to/from keyfile's
run directory. The problem is that migrating profiles in general is
complicated but in some cases it is important to do.
For example checkpoint rollback should recreate the profile in the right
settings plugin, not just add it to persistent storage. This is not yet
properly implemented.
--
Previously, both keyfile and ifcfg-rh plugin implemented in-memory (unsaved)
profiles, while ifupdown plugin cannot handle them. That meant duplication of code
and a ifupdown profile could not be modified or made unsaved.
This is now unified and only keyfile plugin handles in-memory profiles (bgo #744711).
Also, NMSettings is aware of such profiles and treats them specially.
In particular, NMSettings drives the migration between persistent and non-persistent
storage.
Note that a settings plugins may create truly generated, in-memory profiles.
The settings plugin is free to generate and persist the profiles in any way it
wishes. But the concept of "unsaved" profiles is now something explicitly handled
by keyfile plugin. Also, these "unsaved" keyfile profiles are persisted to file system
too, to the /run directory. This is great for two reasons: first of all, all
profiles from keyfile storage in fact have a backing file -- even the
unsaved ones. It also means you can create "unsaved" profiles in /run
and load them with `nmcli connection load`, meaning there is a file
based API for creating unsaved profiles.
The other advantage is that these profiles now survive restarting
NetworkManager. It's paramount that restarting the daemon is as
non-disruptive as possible. Persisting unsaved files to /run improves
here significantly.
--
In the past, NMSettingsConnection also implemented NMConnection interface.
That was already changed a while ago and instead users call now
nm_settings_connection_get_connection() to delegate to a
NMSimpleConnection. What however still happened was that the NMConnection
instance gets never swapped but instead the instance was modified with
nm_connection_replace_settings_from_connection(), clear-secrets, etc.
Change that and treat the NMConnection instance immutable. Instead of modifying
it, reference/clone a new instance. This changes that previously when somebody
wanted to keep a reference to an NMConnection, then the profile would be cloned.
Now, it is supposed to be safe to reference the instance directly and everybody
must ensure not to modify the instance. nmtst_connection_assert_unchanging()
should help with that.
The point is that the settings plugins may keep references to the
NMConnection instance, and so does the NMSettingsConnection. We want
to avoid cloning the instances as long as they are the same.
Likewise, the device's applied connection can now also be referenced
instead of cloning it. This is not yet done, and possibly there are
further improvements possible.
--
Also implement multiple keyfile directores /usr/lib, /etc, /run (rh #1674545,
bgo #772414).
It was always the case that multiple files could provide the same UUID
(both in case of keyfile and ifcfg-rh). For keyfile plugin, if a profile in
read-only storage in /usr/lib gets modified, then it gets actually stored in
/etc (or /run, if the profile is unsaved).
--
While at it, make /etc/network/interfaces profiles for ifupdown plugin reloadable.
--
https://bugzilla.gnome.org/show_bug.cgi?id=772414https://bugzilla.gnome.org/show_bug.cgi?id=744711https://bugzilla.redhat.com/show_bug.cgi?id=1674545
In file included from ./shared/systemd/sd-adapt-shared/nm-sd-adapt-shared.h:21,
from shared/systemd/src/shared/dns-domain.c:3:
./shared/nm-default.h:106:10: fatal error: config-extra.h: No such file or directory
#include "config-extra.h"
^~~~~~~~~~~~~~~~
compilation terminated.
make[1]: *** [Makefile:12933: shared/systemd/src/shared/libnm_systemd_shared_la-dns-domain.lo] Error 1
Fixes: 7d3098ff90 ('systemd: add dns-domain utils to systemd static library')
dns-domain.c contains useful functions for manipulating DNS names.
Add it to the systemd static library we build in shared/, similarly to
what we already do for other utility files that were originally in
src/systemd/src/basic/.
This is inspired by the existing systemd integration, with a few differences:
* This parses the WPAD option, which systemd requested, but did not use.
* We hook into the DAD handling, only making use of the configured address
once DAD has completed successfully, and declining the lease if it fails.
There are still many areas of possible improvement. In particular, we need
to ensure the parsing of all options are compliant, as n-dhcp4 treats all
options as opaque, unlike sd-dhcp4. We probably also need to look at how
to handle failures and retries (in particular if we decline a lease).
We need to query the current MTU at client startu, as well as the hardware
broadcast address. Both these are provided by the kernel over netlink, so
it should simply be a matter of hooking that up with NM's netlink layer.
Contribution under LGPL2.0+, in addition to stated licenses.
The functionality of the ibft settings plugin is now handled by
nm-initrd-generator. There is no need for it anymore, drop it.
Note that ibft called iscsiadm, which requires CAP_SYS_ADMIN to work
([1]). We really want to drop this capability, so the current solution
of a settings plugin (as it is implemented) is wrong. The solution
instead is nm-initrd-generator.
Also, on Fedora the ibft was disabled and probably on most other
distributions as well. This was only used on RHEL.
[1] https://bugzilla.redhat.com/show_bug.cgi?id=1371201#c7
When the code that generates "config-extra.h" changes, we want to regenerate
the file. Move that code to a separate makefile so we can add a
dependency.
Otherwise, we'd had to depend on "Makefile", which itself is generated by
Makefile.am.
Also, depend on "config.h" to regenerate it when ./configure runs and
touches that header. This may not cover all cases where ./configure's
configuration changes and a regeneration would be due. But such is life.
Also, most components depend on this header, so let various .dirstamp
files depend on it, so we are sure to build this first. That because,
autotools generates dependencies for header files automatically, but
that requires that the header file exist. Such automatic dependencies
don't work out-of-the-box for generated headers.
Co-authored-by: Thomas Haller <thaller@redhat.com>
"config-extra.h" is really just like "config.h", except it works around some
limitations of autoconf.
If we depend on "Makefile", any changes to "Makefile.am" will cause a full
rebuild. We want to avoid that.
Instead, depend on "config.h". That one only changes when configure runs
again. And that's the better dependancy, because "config-extra.h" is
generated based on informations generated by configure (despite being
generated by "Makefile").
Not touching "config-extra.h" means that the target is rebuild every
time (because the timestampt does not get updated). On the other hand,
touching it will cause a full rebuild (which we often want to avoid).
The right solution is instead to depend on "config.h", which will be
done next.
This reverts commit 14271d84a0.
This code is now unused.
Also, it does not seem state of the art to me
anymore.
Drop it, it could always be resurrected if need by, but maybe
GFileMonitor could be used instead.
Before commit e3ac45c026 the reader set the private key in the
setting using the libnm function, which also set the key as client
certificate if it was in PKCS #12 format.
After the commit, existing connections with a PKCS #12 private key but
without a client certificate became invalid. Restore the old behavior.
Fixes: e3ac45c026 ('ifcfg-rh: don't use 802-1x certifcate setter functions')
We don't need it anymore.
Still, for tests let gdbus-codegen run and generate the sources and
compile them. We want to keep "dispatcher/nm-dispatcher.xml" and ensure
that it is still valid.
Glib has GValue which used for boxing value.
Add NMValueType enum, which has a similar purpose, but it's much more
limited.
- contrary to GValue, the type must be tracked separately from the
user-data. That is, the "user-data" is only a pointer of appropriate
type, and the knowledge of the actual NMValueType is kept separately.
This will be used to have a static list of meta-data that knows the
value types, but keeping the values independent of this type
information. With GValue this would not be possible.
- the use case is much more limited. Just support basic integers,
boolean and strings. Nothing fancy.
Note that we already do something similar at muliple places. See for
example NMVariantAttributeSpec and nm_utils_team_link_watcher_to_string().
These could/should instead use NMValueType.
When we link static libraries together, there must be no duplicate
symbols.
Since we have a lot of static/intermediate libraries, getting this right
is complicated and sometimes leads to ugly solutions.
As a new rule: don't let static libraries link with other static
libraries. Only binaries and libnm/libnm.la should explicitly link
with all the static libraries that they require.
There are exceptions: "src/libNetworkManager.la" and "libnm/liblibnm.la".
These are static, internal libraries, but they are basically *everything*
that ends up in "src/NetworkManager" and "libnm/libnm.la", respecitively.
Hence, these static libraries also link against other static libraries.
Another exception to this rule is "src/libNetworkManagerTest.la", for
similar reasons.
We compile src/main.c as part of src/NetworkManager. Explicitly link with
glib, because that is required by the source code. Apparently, it also
works without this, but still do it for correctness.
We have "src/libNetworkManager.la" which is an intermediate static
library containing everything that ends up in "src/NetworkManager".
Likewise, add "libnm/liblibnm.la" to be the static library that contains
everything from "libnm/libnm.la".
The point of these libraries is to tie everything together that is used
by "src/NetworkManager" and "libnm/libnm.la" so that it also can be used
by unit-tests. Thereby, the unit tests will link statically against the
code of libnm. The problem is that the unit tests also want to access
internal functionality of libnm that is not accessible when dynamically
linking.
In part, this new library replaces "libnm/libnm-utils.la". The previous
name was confusing, because to us everything is an "utils", and it's
unclear what the purpose of that library was. Now the purpose should be
a bit clearer: liblibnm.la is a step before libnm.la, similar to what
libNetworkManager.la is to NetworkManager.
We already have "libnm-core/tests/test-keyfile.c" from which we build
"test-keyfile".
Our test binaries should be named the following:
- "*/tests/test-*"
- the test binary "*/tests/test-*" should be build from a source file
"*/tests/test-*.c". Meaning: the source's and executable's name should
correspond.
- test binaries should be named uniquely. Also, because older meson
versions don't like having the same binary name more than once.
Rename to avoid the duplicate name.
Older versions of meson don't like building multiple artifacts
with the same name (even if they are in different directories). We
have multiple tests called "test-general.c", and it would be natural
to compile a test binary of the same name.
Meson encountered an error in file src/tests/meson.build, line 14, column 2:
Tried to create target "test-general", but a target of that name already exists.
It's generally a bad idea to have in our source tree multiple files with the
same name. Rename the test.
Fixes: 16cd84d346 ('build/meson: rename platform tests to use same name as autotools'):
make[3]: Entering directory 'NetworkManager/_build/sub'
CC clients/cli/nmcli-common.o
cc1: error: ./clients/common: No such file or directory [-Werror=missing-include-dirs]
cc1: all warnings being treated as errors
The only generated header in $builddir/clients/common is settings-docs.h
and only libnmc.la needs it. Include the directory on the command line
only when we know it exists.
The dispatcher looks there for scripts now. This actually doesn't break
the RPM build, since it doesn't mind extra empty directories in
buildroot. Good.
These tests cannot (easily) be under "shared/nm-libnm-core-aux/tests"
because libnm/libnm-core requires code under shared while
"nm-libnm-core-aux" requires libnm/libnm-core. With autotools that is
not problem, but with meson we include sub directories in a particular
order and there is no way to foward declare stuff (AFAIK). To avoid
the circular dependency, add the tests to "clients/common/tests", which
is always built last.
"libnm-core" implements common functionality for "NetworkManager" and
"libnm".
Note that clients like "nmcli" cannot access the internal API provided
by "libnm-core". So, if nmcli wants to do something that is also done by
"libnm-core", , "libnm", or "NetworkManager", the code would have to be
duplicated.
Instead, such code can be in "libnm-libnm-core-{intern|aux}.la".
Note that:
0) "libnm-libnm-core-intern.la" is used by libnm-core itsself.
On the other hand, "libnm-libnm-core-aux.la" is not used by
libnm-core, but provides utilities on top of it.
1) they both extend "libnm-core" with utlities that are not public
API of libnm itself. Maybe part of the code should one day become
public API of libnm. On the other hand, this is code for which
we may not want to commit to a stable interface or which we
don't want to provide as part of the API.
2) "libnm-libnm-core-intern.la" is statically linked by "libnm-core"
and thus directly available to "libnm" and "NetworkManager".
On the other hand, "libnm-libnm-core-aux.la" may be used by "libnm"
and "NetworkManager".
Both libraries may be statically linked by libnm clients (like
nmcli).
3) it must only use glib, libnm-glib-aux.la, and the public API
of libnm-core.
This is important: it must not use "libnm-core/nm-core-internal.h"
nor "libnm-core/nm-utils-private.h" so the static library is usable
by nmcli which couldn't access these.
Note that "shared/nm-meta-setting.c" is an entirely different case,
because it behaves differently depending on whether linking against
"libnm-core" or the client programs. As such, this file must be compiled
twice.
