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.
By now, keyfile code got relicensed as LGPL-2.1+ and is just a regular part
of libnm-core (in particular, because it uses private API of libnm-core).
It should no longer be in a separate directory, but for now, at lead compile
it the same as libnm-core.
We got rid of all these redundant defines. All we need, is the base
source directory, which we already define in config.h as
NM_BUILD_SRCDIR. Use that.
If you configure with "--enable-tests=no", then the tests
are not build during `make all`, but only during `make check`.
That is convenient for development, so if you don't work on tests,
they usually don't get build and the (partial) compilation is faster.
However, there is no target to build the tests without running them.
Add `make check-progs` for that.
Currently "src/" mostly contains the source code of the daemon.
I say mostly, because that is not true, there are also the device,
settings, wwan, ppp plugins, the initrd generator, the pppd and dhcp
helper, and probably more.
Also we have source code under libnm-core/, libnm/, clients/, and
shared/ directories. That is all confusing.
We should have one "src" directory, that contains subdirectories. Those
subdirectories should contain individual parts (libraries or
applications), that possibly have dependencies on other subdirectories.
There should be a flat hierarchy of directories under src/, which
contains individual modules.
As the name "src/" is already taken, that prevents any sensible
restructuring of the code.
As a first step, move "src/" to "src/core/". This gives space to
reorganize the code better by moving individual components into "src/".
For inspiration, look at systemd's "src/" directory.
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/743
This is basically all of libnm as a static-library. The name
liblibnm isn't great. Arguably, we do have quite a lot of
libnmxyz, so finding a good name is hard. But libnm-static seems
a better name. Rename.
Fix the following build error:
In file included from shared/nm-base/nm-ethtool-base.c:6:
./shared/nm-default.h:87:10: fatal error: config-extra.h: No such file or directory
87 | #include "config-extra.h"
| ^~~~~~~~~~~~~~~~
Fixes: e5d2a05ad5 ('libnm: add "shared/nm-base/nm-base.h"')
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.
We want to use these defines for option names also in "shared/nm-base"
(and in turn in "shared/nm-platform), which cannot include "libnm-core".
However, they are also public API of libnm.
To get this done, in a first step, move these defines to a new header
"libnm-core/nm-ethtool-utils.h".
Since now the name "nm-ethtool-utils.h" is taken, also rename
nm-libnm-core-intern files.
Our dependencies are complicated.
Currently "src/platform" uses parts of libnm-core and is relatively
strongly entangled with core. It would be nice to have that part
clearly independent from "src" and from "libnm-core".
Also, "src/platform/nm-platform-utils.h" uses NMEthtoolID enum, which
previously was defined in "libnm-core/nm-libnm-core-intern/nm-ethtool-utils.h".
Move that to a new place "shared/nm-base/nm-base.h".
Note that we have "libnm-core/nm-libnm-core-intern", which is
libnm/core related code which uses and is used by libnm-core.
There is a need for a library which is used by libnm-core, but
does not depend on libnm-core itself. Here comes "shared/nm-base".
Yes, many libraries. But the goal is to entangle the dependencies
and have a clear hierarchy of includes. And to have "shared/nm-platform"
independent of libnm-core.
We want to move platform code to "shared/nm-platform". However, platform
code uses the logging infrastructure from the daemon, there is thus
an odd circular dependency.
Solve that by moving the "src/nm-logging.[hc]" to a new helper library
in "shared/nm-log-core".
NetworkManager core is huge. We should try to split out
parts that are independent.
Platform code is already mostly independent. But due to having it
under "src/", there is no strict separation/layering which determines
the parts that can work independently. So, while the code is mostly
independent (in practice), that is not obvious from looking at the
source tree. It thus still contributes to cognitive load.
Add a shared library "shared/nm-platform", which should have no
dependencies on libnm-core or NetworkManager core.
In a first step, move the netlink code there. More should follow.
Add a new key management option to support WPA3 Enteprise wifi
connection.
Only supported with wpa_supplicant for the time being.
Signed-off-by: Antonio Cardace <acardace@redhat.com>
In C, includes with <> are for system headers, while "" prefers the
current working directory (implementation defined).
For libnm headers that include other libnm headers, we tend to use
"" instead of <>. That makes sense to me. Be consistent about that.
NetworkManager is now able to configure veth interfaces throught the
NMSettingVeth. Veth interfaces only have "peer" property.
In order to support Veth interfaces in NetworkManager the design need
to pass the following requirements:
* Veth setting only has "peer" attribute.
* Ethernet profiles must be applicable to Veth interfaces.
* When creating a veth interface, the peer will be managed by
NetworkManager but will not have a profile.
* Veth connection can reapply only if the peer has not been modified.
* In order to modify the veth peer, NetworkManager must deactivate the
connection and create a new one with peer modified.
In general, it should support the basis of veth interfaces but without
breaking any existing feature or use case. The users that are using veth
interfaces as ethernet should not notice anything changed unless they
specified the veth peer setting.
Creating a Veth interface in NetworkManager is useful even without the
support for namespaces for some use cases, e.g "connecting one side of
the veth to an OVS bridge and the other side to a Linux bridge" this is
done when using OVN kubernetes [1][2]. In addition, it would provide
persistent configuration and rollback support for Veth interfaces.
[1] https://bugzilla.redhat.com/show_bug.cgi?id=1885605
[2] https://bugzilla.redhat.com/show_bug.cgi?id=1894139
Signed-off-by: Fernando Fernandez Mancera <ffmancera@riseup.net>
We can generate "generate-docs-nm-settings-nmcli.xml" by running "clients/cli/generate-docs-nm-settings-nmcli".
However, during cross compilation, that binary gets build in the target architecture,
it can thus not run to generate the XML.
Note that "--enable-gtk-doc" requires "--enable-introspection".
For generating "settings-docs.h" we (only) need introspection/pygobject.
We also have a pre-generated .in file that we can use if introspection
is not available. Since we have a pre-generated variant, it would be fine
to always use that one. However, we want to use generate the file if we
have the necessary dependencies, because thereby we can check whether
the pre-generated file is identical to what would be generated.
We have a similar problem with "generate-docs-nm-settings-nmcli.xml".
However there we don't need introspection, but merely being able to
execute a binary that we build. That does not work during cross
compilation, so we will honor "--enable-gtk-doc" flag to decide when
to generate the file.
For consistency, also adjust the condition for "settings-docs.h" to only
generate the file if we have "--enable-gtk-doc" (but not it we build
with "--enable-introspection" alone).
With glib2-2.67.0-1.fc34.x86_64.rpm, clang-11.0.0-2.fc34.x86_64.rpm, the
generated code emits a compiler warning:
libnm-core/tests/nm-core-tests-enum-types.c:17:7: error: passing 'typeof (*(&g_define_type_id__volatile)) *' (aka 'volatile unsigned long *') to parameter of type 'gsize *' (aka 'unsigned long *') discards qualifiers [-Werror,-Wincompatible-pointer-types-discards-qualifiers]
if (g_once_init_enter (&g_define_type_id__volatile))
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/include/glib-2.0/glib/gthread.h:260:7: note: expanded from macro 'g_once_init_enter'
(!g_atomic_pointer_get (location) && \
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/include/glib-2.0/glib/gatomic.h:112:38: note: expanded from macro 'g_atomic_pointer_get'
__atomic_load (gapg_temp_atomic, &gapg_temp_newval, __ATOMIC_SEQ_CST); \
^~~~~~~~~~~~~~~~~
libnm-core/tests/nm-core-tests-enum-types.c:40:7: error: passing 'typeof (*(&g_define_type_id__volatile)) *' (aka 'volatile unsigned long *') to parameter of type 'gsize *' (aka 'unsigned long *') discards qualifiers [-Werror,-Wincompatible-pointer-types-discards-qualifiers]
if (g_once_init_enter (&g_define_type_id__volatile))
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/include/glib-2.0/glib/gthread.h:260:7: note: expanded from macro 'g_once_init_enter'
(!g_atomic_pointer_get (location) && \
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/include/glib-2.0/glib/gatomic.h:112:38: note: expanded from macro 'g_atomic_pointer_get'
__atomic_load (gapg_temp_atomic, &gapg_temp_newval, __ATOMIC_SEQ_CST); \
^~~~~~~~~~~~~~~~~
libnm-core/tests/nm-core-tests-enum-types.c:63:7: error: passing 'typeof (*(&g_define_type_id__volatile)) *' (aka 'volatile unsigned long *') to parameter of type 'gsize *' (aka 'unsigned long *') discards qualifiers [-Werror,-Wincompatible-pointer-types-discards-qualifiers]
if (g_once_init_enter (&g_define_type_id__volatile))
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/include/glib-2.0/glib/gthread.h:260:7: note: expanded from macro 'g_once_init_enter'
(!g_atomic_pointer_get (location) && \
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/include/glib-2.0/glib/gatomic.h:112:38: note: expanded from macro 'g_atomic_pointer_get'
__atomic_load (gapg_temp_atomic, &gapg_temp_newval, __ATOMIC_SEQ_CST); \
^~~~~~~~~~~~~~~~~
We could pass "-Wincompatible-pointer-types-discards-qualifiers" as CFLAGS
when building this file. However, we have a workaround in our "nm-glib-aux/nm-glib.h",
so we can instead include "nm-default.h". At first glance, that might look like
the less preferable solution. However, this file is only there for unit tests,
and we also include "nm-default.h" for other sources that are generated with
"glib-mkenums". So, doing it also for our tests becomes the preferable solution.
NML3Cfg already handles IPv4 ACD. IPv4LL is just a small additional
layer on top of that, so it makes sense that it also is handled by
NML3Cfg.
Also, the overall goal is that multiple NMDevice and NMVpnConnection
instances can cooperate independently. So if multiple "users" enable
IPv4LL on an interface, then we should only run it once. This is
achieved by NML3IPv4LL's API where users register what they want,
and NML3IPv4LL figures out what that means as a whole.
Also, we thus will no longer need to use sd_ipv4ll/n-ipv4ll, because
we implement it ourself.
Currently, NMDevice does ACD. It intercepts certain NMIP4Config
instances, and tries to perform ACD on the addresses. I think this
functionality should be handled by NML3Cfg instead.
For one, NML3Cfg sees all configurations, and can perform ACD for all
(relevant) addresses. Also, it moves logic away from NMDevice and makes
the functionality available without an NMDevice. As such, it also will
allow that independent "controllers" contribute NML3ConfigData instances
and ACD will performed for all of them (as requested).
This will be our implementation for IPv4 ACD (https://tools.ietf.org/html/rfc5227)
based on nettools' n-acd library.
The code is not actually tested yes, because NMDevice did not yet switch
over to use NML3Cfg. Once that happens, surely issues with this patch
will be found that will need fixing.
nmcli is build with libtool, so "clients/cli/nmcli" is really a shell script
that invokes the real nmcli (at "clients/cli/.libs/nmcli").
When building with LTO for some reasons "clients/cli/nmcli" still
does some build steps during the first invocation.
That means, if we run `make check-local-clients-tests-test-client` it
would first do the final build step. This takes a while, and the test
times out (worse, we do that build step many times in parallel).
Avoid that by invoking "clients/cli/nmcli" first.
We use a linker version script "NetworkManager.ver", to hide
symbols from NetworkManager that are not used. That is important
due to our habit of using internal helper libraries that we link
statically everywhere, without handpicking the symbols we actually
need. We want the tooling to get rid of unnecessary symbols.
However, NetworkManager loads shared libraries for settings and device
plugins. These libraries require symbols from the NetworkManager binary,
but which one depends on build options. Hence, we also generate
"NetworkManager.ver" by the "tools/create-exports-NetworkManager.sh"
script.
For that the script uses "nm" to find symbols that are undefined in the
plugin libraries but defined in NetworkManager. With autotools the
script looked at "./src/.libs/libNetworkManager.a" to find the present
symbols. Note that for meson that already didn't work, and we build
instead an intermediate NetworkManager binary first (with all symbols
exposed). With LTO, "nm" doesn't find all symbols in
"./src/.libs/libNetworkManager.a", and consequently they are not
exported and dropped/hidden.
This also causes unit tests to fail with LTO, because our test script
"tools/check-exports.sh" catches such bugs.
Fix that by also with autotools generate a complete "NetworkManager-all-sym"
binary that is used to generate "NetworkManager.ver", before rebuilding
"NetworkManager" again.
NMIP[46]Config will become much simpler than it is today.
It's sole responsibility will be to expose current settings
on D-Bus, in it's function as a NMDBusObject subtype.
However, it still make sense to let them share a common base class.
Add it.
Currently NMIP4Config and NMIP6Config both track the data to be
configured, they expose properties on D-Bus, and they have logic for
capturing and applying settings to platform.
We will split that.
- NMIP4Config and NMIP6Config will expose data on D-Bus.
- NML3Cfg will have the logic for handling IP configuration.
- NML3ConfigData will track data to be configured.
NML3ConfigData mirrors NMIP4Config/NMIP6Config in many aspects. For now,
this duplicates a lot of code. More will be done later. Eventually,
NMIP4Config/NMIP6Config will drop the duplicated functionality.
Our "nm-json-aux.h" redefines various things from <jansson.h> header.
Add a unit test that checks that what we redefine exactly matches what
libjansson would provide, so that they are compatible.
They serve a similar purpose.
Previously, nm-json-aux.h contained the virtual function table for accessing
the dynamically loaded libjansson. But there is no reason why our own
helper functions from nm-json.h cannot be there too.
nm-json.[hc] uses libjansson, but only loads it at runtime with dlopen. There
is no more run compile time dependency. Move it to shared, so that it can be
(theoretically) used by other components.
Also, drop the conditional compilation. Granted, if you don't build with
libjansson enabled, then the JANSSON_SONAME define is unset and the code
will fail to load at runtime (which is fine). However, we can still build
against our JSON wrappers. The code savings of conditional build are minimal
so drop it.
This add a provider implementation for GCP that when detected fetches
the ip addresses of configured internal load balancers.
Once this information is fetched from the metadata server it instructs
NetworkManager to add local routes for each found forwarded-ip.
https://bugzilla.redhat.com/show_bug.cgi?id=1821787
If python black is install then it would check the
formating of all of the python files and test the for it.
Otherwise, it would just simply ignore the python black
if python black is not installed.
It would seem that the proper dependency is "man/.dirstamp". But that just
doesn't work. Use "man/common.ent" instead. If you figure out how to
convince autotools to make .dirstamp working, send a patch.
Especially for "nm-settings-docs-nmcli.xml", the first XML to merge is
"clients/cli/generate-docs-nm-settings-nmcli.xml". That file is
generated with the meta data from nmcli, and it contains all the
properties that are supported. Properties from other XML files,
that are passed as additional arguments should not be merged.
In most cases, there is no difference. It only matters for
"ipv6.dad-timeout" and "user.data". For example, "ipv6.dad-timeout"
is supported by GObject (part of "libnm/nm-settings-docs-gir.xml"),
but not by nmcli. Don't include it in the manual.
This also drops the now empty settings "dummy", "user", and "generic".
We have the correct meta-data of supported properties for nmcli. It is
in clients/common. Use that for generating the manual page instead of
the properties that are part of libnm (some properties may be in libnm
but not supported by nmcli, or some properties may not be GObject
properties, and not detected as by GObject introspection).
"nm-settings-docs-nmcli.xml" will be generated by a tool that depends on
"clients/common/". The file should thus not be in libnm directory, otherwise
there is a circular dependency.
Move the file to "man/" directory.
For consistency, also move "nm-settings-docs-dbus.xml". Note that we
cannot move "nm-settings-docs-gir.xml" to "man/", because that one is
needed for building clients.
Like the previous commit. Move code that depends on libnm out
of shared to avoid circular dependency.
Also add a readme file explaining the reason for existence of
the helper library.
Like the previous commit. Move code that depends on libnm-core out
of shared to avoid circular dependency.
Also add a readme file explaining the reason for existence of
the helper libraries nm-libnm-core-intern and nm-libnm-core-aux.
The "shared" directory is used by libnm-core, it should thus only depend on
code that is in the "shared" directory. Otherwise there is a circular
dependency, and meson's subdir() does not work nicely.
Also, libnm-core is really part of (and also an extension of) libnm-core,
so it belongs there.
I guess, the original idea was that this is also an extension for libnm,
so another project could take these utility functions (by copying them
into their source tree) and use them. That is still possible, it's
just that the sources are no longer under the shared directory.
Also add a readme to explain the non-obvious meaning of these files.
Originally, these files were part of libnm-core and linked together.
However, that is a licensing violation, because the code is GPL-2.0+
licensed, while libnm-core also gets linked with libnm (it must thus
be LGPL-2.1+). The original intent behind moving the code to "shared/"
was to avoid the licensing issue, but also to prepare when we would add
a separate, GPL licensed libnm-keyfile. However, currently we hope to
be able to relicense the code, so that it actually could be exposed as
part of libnm. This is work in progress at ([1]).
[1] https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/ ## 517
Anyway, the current directory layout is problematic. libnm-keyfile
depends on libnm-core, while libnm-core depends on code under shared.
That means, there is a circular dependency and meson's subdir() does
not work well.
Move the code.
A significant part of NetworkManager's API are the connection profiles, documented
in `man nm-settings*`. But there are different aspects about profiles, depending
on what you are interested. There is the D-Bus API, nmcli options, keyfile format,
and ifcfg-rh format. Additionally, there is also libnm API.
Add distinct manual pages for the four aspects. Currently the two new manual
pages "nm-settings-dbus" and "nm-settings-nmcli" are still identical to the
former "nm-settings.5" manual. In the future, they will diverge to
account for the differences.
There are the following aspects:
- "dbus"
- "keyfile"
- "ifcfg-rh"
- "nmcli"
For "libnm" we don't generate a separate "nm-settings-libnm" manual
page. That is instead documented via gtk-doc.
Currently the keyfile and ifcfg-rh manual pages only detail settings
which differ. But later I think also these manual pages should contain
all settings that apply.
"nm-settings-docs-dbus.xml" is "nm-settings-docs-gir.xml" merged with
"nm-property-infos-dbus.xml". The name should reflect that, also because
we will get more files with this naming scheme.
The name is bad. For one, we will have more files of the same format
("nm-settings-docs-nmcli.xml").
Also, "libnm/nm-settings-docs.xml" and "libnm/nm-property-docs.xml" had
basically the same file format. Their name should be similar.
Also the tool to generate the file should have a name that reminds to
the file that it creates.
The naming was inconsistent. Rename.
- all the property infos of this kind a now consistently called
"libnm/nm-property-infos-$TAG.xml".
- the script to generate files "libnm/nm-property-infos-$TAG.xml" is
now called "libnm/generate-docs-nm-property-infos.pl".
"man/nm-settings%.xml" really should depend on "common.ent".
The reason is that XSL files like "man/nm-settings.xsl" include
"common.ent".
The previous code already tried to express that, but for some
reasons this dependency was not honored. Fix that.
However, that uncovers another problem with gtk-doc.make. If we do
that without the workaround for "docs/api/Makefile.am", then
$ ./autogen.sh && make V=1 SHELL='sh -x' distcheck
breaks.
The reason is not clear to me. The new dependency leads to rebuild
"man/nm-settings-keyfile.xml". But that is worse, somehow the file
"$(top_srcdir)/man/nm-settings-keyfile.xml" ends up being read-only.
Afterwards, gtk-doc.make does
setup-build.stamp:
-$(GTK_DOC_V_SETUP)if test "$(abs_srcdir)" != "$(abs_builddir)" ;
then \
files=`echo $(SETUP_FILES) $(DOC_MODULE).types`; \
if test "x$$files" != "x" ; then \
for file in $$files ; do \
destdir=`dirname $(abs_builddir)/$$file`; \
test -d "$$destdir" || mkdir -p "$$destdir"; \
test -f $(abs_srcdir)/$$file && \
cp -pf $(abs_srcdir)/$$file $(abs_builddir)/$$file || true;
\
done; \
fi; \
fi
$(AM_V_at)touch setup-build.stamp
so that the files in build dir are also read-only. Then, make distcheck
goes ahead and builds the files once again, which fails.
You are welcome to understand why this workaround is necessary. Please
then create a better fix.
The script "libnm/generate-setting-docs.py" generates property info based
on GObject introspection data.
Optionally, when creating the manual for D-Bus documentation, it would accept
an argument "--override" to merge the generated information with the information
from an XML generated by "libnm/generate-plugin-docs.xml". Change this.
Instead, let "libnm/generate-setting-docs.py" just do one thing: generate
the XML based on GObject introspection data. Then, a second script
"libnm/generate-docs-nm-settings-docs-merge.py" can merge the XMLs.
Note that currently the manual for "nm-settings-keyfile" only contains
information about properties that are explicitly mentioned for keyfile.
It think that is not right. In NetworkManager there are multiple "aspects"
about connection profiles: D-Bus, libnm, nmcli, keyfile and ifcfg-rh.
When we generate a manual page for any of these aspects, we should always
detail all properties. At least for nmcli and D-Bus. That means, we will
do the merging multiple times. Hence, keep the steps for parsing GObject
introspection data and the merging separate.
Also, "generate-setting-docs.py" and "generate-plugin-docs.pl" should
generate the same XML scheme, so that merge doesn't need special hacks.
That is currently not the case, for example, the override XML contains a
"format" attribute, while the other one contains a "type". Merging these
is a special hack. This should be unified.
Install a NM-specific firewalld zone to be used for interfaces that
are used for connection sharing. The zone blocks all traffic to the
local machine except some protocols (DHCP, DNS and ICMP) and allows
all forwarded traffic.
- 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.
