Run:
./contrib/scripts/nm-code-format.sh -i
./contrib/scripts/nm-code-format.sh -i
Yes, it needs to run twice because the first run doesn't yet produce the
final result.
Signed-off-by: Antonio Cardace <acardace@redhat.com>
This code will change, but in essence we will still need such a function
to convert a list of addresses/routes to D-Bus. Extract the code, so it
can be better reused and adjusted.
and rename to nm_utils_ip_route_attribute_to_platform(). The function is independent
from NMIP4Config. We also will use it outside of NMIP4Config. Also, "NetworkManagerUtils.c"
already has similar functions that parse libnm structures to internal structures.
The previous to wait-types (NM_SHUTDOWN_WAIT_TYPE_OBJECT and
NM_SHUTDOWN_WAIT_TYPE_CANCELLABLE) both required a GObject/GCancellable,
and the shutdown was automatically unblocked when the object got
destroyed.
Add another wait type NM_SHUTDOWN_WAIT_TYPE_HANDLE, which does not take
an object to wait. Instead, shutdown is indefinitely blocked, until the
user unregisters the handle again. While other wait-types allow to
ignore the handle, this wait-type only makes sense if the user keeps
track of the handle.
Now nm_shutdown_wait_obj_*() supports two styles:
- NM_SHUTDOWN_WAIT_TYPE_OBJECT: this just registers a weak pointer
on a source GObject. As long as the object is not destroyed
(and the object is not unregistered), the shutdown gets blocked.
- now new is NM_SHUTDOWN_WAIT_TYPE_CANCELLABLE: this source object
is a GCancellable, and during shutdown, the system will cancel
the instances to notify about the shutdown. That aside, the GCancellable
is tracked exactly like a regular NM_SHUTDOWN_WAIT_TYPE_OBJECT (meaning:
a weak pointer is registered and shutdown gets delayed as long as the instance
lives).
As the rest of the shutdown, it's not yet implemented on the shutdown-side.
What is now possible is to register such cancellables, so that users can make
use of this API before we fix shutdown. We cannot fix it all at the same time,
so first users must be ready for this approach.
WireGuard's wq-quick configures such rules to avoid routing loops.
While we currently don't have an automatic solution for this, at least
we should support it via explicit user configuration.
One problem is that suppress_prefixlength is relatively new and kernel
might not support this attribute. That can lead to odd results, because
the NetworkManager is valid but it cannot be configured on the current
kernel. But this is a general problem, and we would require a general
solution. The solution cannot be to only support rule attributes that
are supported by the oldest possible kernel. It's not clear how much of
a problem there really is, or which general solution is required (if
any).
In fact, nm_shutdown_wait_obj_register*() API is still not implemented
and registering an object has no effect currently. That is, blocking
shutdown and waiting for instances to be destroyed during shutdown
is not yet implemented. Still, we already implement the API so that
components can register themself to block the shutdown. The point is
of course, that the callers already use this API, although it's not yet
implemented.
Anyway, sometimes the message string is not static. Add an option to
pass an allocated string and let the string be destroyed when no longer
needed.
We no longer add these. If you use Emacs, configure it yourself.
Also, due to our "smart-tab" usage the editor anyway does a subpar
job handling our tabs. However, on the upside every user can choose
whatever tab-width he/she prefers. If "smart-tabs" are used properly
(like we do), every tab-width will work.
No manual changes, just ran commands:
F=($(git grep -l -e '-\*-'))
sed '1 { /\/\* *-\*- *[mM]ode.*\*\/$/d }' -i "${F[@]}"
sed '1,4 { /^\(#\|--\|dnl\) *-\*- [mM]ode/d }' -i "${F[@]}"
Check remaining lines with:
git grep -e '-\*-'
The ultimate purpose of this is to cleanup our files and eventually use
SPDX license identifiers. For that, first get rid of the boilerplate lines.
"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.
(cherry picked from commit af07ed01c0)
From the files under "shared/nm-utils" we build an internal library
that provides glib-based helper utilities.
Move the files of that basic library to a new subdirectory
"shared/nm-glib-aux" and rename the helper library "libnm-core-base.la"
to "libnm-glib-aux.la".
Reasons:
- the name "utils" is overused in our code-base. Everything's an
"utils". Give this thing a more distinct name.
- there were additional files under "shared/nm-utils", which are not
part of this internal library "libnm-utils-base.la". All the files
that are part of this library should be together in the same
directory, but files that are not, should not be there.
- the new name should better convey what this library is and what is isn't:
it's a set of utilities and helper functions that extend glib with
funcitonality that we commonly need.
There are still some files left under "shared/nm-utils". They have less
a unifying propose to be in their own directory, so I leave them there
for now. But at least they are separate from "shared/nm-glib-aux",
which has a very clear purpose.
(cherry picked from commit 80db06f768)
nm_ip_route_get_prefix() and plen are guint type, hence the following
is not correct:
plen = nm_ip_route_get_prefix (route1);
r = plen - nm_ip_route_get_prefix (route2);
if (r)
return r > 0 ? 1 : -1;
Use the macro, it gets subtle cases like this right.
Fixes: b32bb36c61
NMIPAddr contains an unnamed union. We have to either explicitly
initialize one field, or omit it.
../shared/nm-utils/nm-shared-utils.c:38:36: error: suggest braces around initialization of subobject [-Werror,-Wmissing-braces]
const NMIPAddr nm_ip_addr_zero = { 0 };
^
{}
For better or worse, we already pull in large parts of systemd sources.
I need a base64 decode implementation (because glib's g_base64_decode()
cannot reject invalid encodings). Instead of coming up with my own or
copy-paste if from somewhere, reuse systemd's unbase64mem().
But for that, make systemd's basic bits an independent static library
first because I will need it in libnm-core.
This doesn't really change anything except making "libnm-systemd-core.la"
an indpendent static library that could be used from "libnm-core". We
shall still be mindful about which internal code of systemd we use, and only
access functionality that is exposed via "systemd/nm-sd-utils-shared.h".
Refactor some code to use nm_streq() and NM_IN_STRSET() instead of
strcmp().
Note that nm_utils_get_ip_config_method() never returns %NULL (not even
with g_return*() assertion failures). nm_streq() is sufficent.
Recently, more and more code was refactored to use an addr_family
integer to distinguish between IPv4 and IPv6.
Refactor nm_utils_get_ip_config_method() and nm_device_get_effective_ip_config_method()
to do that too. If we use different identifiers, we need to translate from one to
another and its inconsistent. Also, accessing a GType is an unnecessary function call,
instead of a plain constant.
The need for this is the following:
"ipv4.dhcp-client-id" can be specified via global connection defaults.
In absence of any configuration in NetworkManager, the default depends
on the DHCP client plugin. In case of "dhclient", the default further
depends on /etc/dhcp.
For "internal" plugin, we may very well want to change the default
client-id to "mac" by universally installing a configuration
snippet
[connection-use-mac-client-id]
ipv4.dhcp-client-id=mac
However, if we the user happens to enable "dhclient" plugin, this also
forces the client-id and overrules configuration from /etc/dhcp. The real
problem is, that dhclient can be configured via means outside of NetworkManager,
so our defaults shall not overwrite defaults from /etc/dhcp.
With the new device spec, we can avoid this issue:
[connection-dhcp-client-id]
match-device=except:dhcp-plugin:dhclient
ipv4.dhcp-client-id=mac
This will be part of the solution for rh#1640494. Note that merely
dropping a configuration snippet is not yet enough. More fixes for
DHCP will follow. Also, bug rh#1640494 may have alternative solutions
as well. The nice part of this new feature is that it is generally
useful for configuring connection defaults and not specifically for
the client-id issue.
Note that this match spec is per-device, although the plugin is selected
globally. That makes some sense, because in the future we may or may not
configure the DHCP plugin per-device or per address family.
https://bugzilla.redhat.com/show_bug.cgi?id=1640494
Add a helper function for the common check whether a file is
inside a path. Also, this function handles special cases like
repeated file separators. However, as it is still entirely text
based, it also cannot recognize if two (literally) different
paths reference the same inode/file.
NMConnection is an interface, which is implemented by the types
NMSimpleConnection (libnm-core), NMSettingsConnection (src) and
NMRemoteConnection (libnm).
NMSettingsConnection does a lot of things already:
1) it "is-a" NMDBusObject and exports the API of a connection profile
on D-Bus
2) it interacts with NMSettings and contains functionality
for tracking the profiles.
3) it is the base-class of types like NMSKeyfileConnection and
NMIfcfgConnection. These handle how the profile is persisted
on disk.
4) it implements NMConnection interface, to itself track the
settings of the profile.
3) and 4) would be better implemented via delegation than inheritance.
Address 4) and don't let NMSettingsConnection implemente the NMConnection
interface. Instead, a settings-connection references now a NMSimpleConnection
instance, to which it delegates for keeping the actual profiles.
Advantages:
- by delegating, there is a clearer separation of what
NMSettingsConnection does. For example, in C we often required
casts from NMSettingsConnection to NMConnection. NMConnection
is a very trivial object with very little logic. When we have
a NMConnection instance at hand, it's good to know that it is
*only* that simple instead of also being an entire
NMSettingsConnection instance.
The main purpose of this patch is to simplify the code by separating
the NMConnection from the NMSettingsConnection. We should generally
be aware whether we handle a NMSettingsConnection or a trivial
NMConnection instance. Now, because NMSettingsConnection no longer
"is-a" NMConnection, this distinction is apparent.
- NMConnection is implemented as an interface and we create
NMSimpleConnection instances whenever we need a real instance.
In GLib, interfaces have a performance overhead, that we needlessly
pay all the time. With this change, we no longer require
NMConnection to be an interface. Thus, in the future we could compile
a version of libnm-core for the daemon, where NMConnection is not an
interface but a GObject implementation akin to NMSimpleConnection.
- In the previous implementation, we cannot treat NMConnection immutable
and copy-on-write.
For example, when NMDevice needs a snapshot of the activated
profile as applied-connection, all it can do is clone the entire
NMSettingsConnection as a NMSimpleConnection.
Likewise, when we get a NMConnection instance and want to keep
a reference to it, we cannot do that, because we never know
who also references and modifies the instance.
By separating NMSettingsConnection we could in the future have
NMConnection immutable and copy-on-write, to avoid all unnecessary
clones.
Eventually we should do a coordinated shutdown when NetworkManager exits.
That is a large work, ensuring that all asynchronous actions are cancellable
(in time), and that we wait for all pending operations to complete.
Add a function nm_shutdown_register_watchdog(), so that objects can register
themselves, to block shutdown until they are destroyed. It's not yet used,
because actually iterating the mainloop during shutdown can only be done,
once the code is prepared to be ready for that. But we already need the
function, so that we can refactor individual parts of the code, in preparation
of using it in the future.
Use the same form everywhere: "TRANSLATORS" instead of "Translators".
The manual also seems to prefer the upper-case form [1].
$ sed 's/\<Translators\>: /TRANSLATORS: /g' $(git grep -l Translators) -i
[1] https://www.gnu.org/software/gettext/manual/gettext.html
NMSettings exposes a cached list of all connection. We don't need
to clone it. Note that this is not save against concurrent modification,
meaning, add/remove of connections in NMSettings will invalidate the
list.
However, it wasn't save against that previously either, because
altough we cloned the container (GSList), we didn't take an additional
reference to the elements.
This is purely a performance optimization, we don't need to clone the
list. Also, since the original list is of type "NMConnection *const*",
use that type insistently, instead of dependent API requiring GSList.
IMO, GSList is anyway not a very nice API for many use cases because
it requires an additional slice allocation for each element. It's
slower, and often less convenient to use.
Previously, we used the generated GDBusInterfaceSkeleton types and glued
them via the NMExportedObject base class to our NM types. We also used
GDBusObjectManagerServer.
Don't do that anymore. The resulting code was more complicated despite (or
because?) using generated classes. It was hard to understand, complex, had
ordering-issues, and had a runtime and memory overhead.
This patch refactors this entirely and uses the lower layer API GDBusConnection
directly. It replaces the generated code, GDBusInterfaceSkeleton, and
GDBusObjectManagerServer. All this is now done by NMDbusObject and NMDBusManager
and static descriptor instances of type GDBusInterfaceInfo.
This adds a net plus of more then 1300 lines of hand written code. I claim
that this implementation is easier to understand. Note that previously we
also required extensive and complex glue code to bind our objects to the
generated skeleton objects. Instead, now glue our objects directly to
GDBusConnection. The result is more immediate and gets rid of layers of
code in between.
Now that the D-Bus glue us more under our control, we can address issus and
bottlenecks better, instead of adding code to bend the generated skeletons
to our needs.
Note that the current implementation now only supports one D-Bus connection.
That was effectively the case already, although there were places (and still are)
where the code pretends it could also support connections from a private socket.
We dropped private socket support mainly because it was unused, untested and
buggy, but also because GDBusObjectManagerServer could not export the same
objects on multiple connections. Now, it would be rather straight forward to
fix that and re-introduce ObjectManager on each private connection. But this
commit doesn't do that yet, and the new code intentionally supports only one
D-Bus connection.
Also, the D-Bus startup was simplified. There is no retry, either nm_dbus_manager_start()
succeeds, or it detects the initrd case. In the initrd case, bus manager never tries to
connect to D-Bus. Since the initrd scenario is not yet used/tested, this is good enough
for the moment. It could be easily extended later, for example with polling whether the
system bus appears (like was done previously). Also, restart of D-Bus daemon isn't
supported either -- just like before.
Note how NMDBusManager now implements the ObjectManager D-Bus interface
directly.
Also, this fixes race issues in the server, by no longer delaying
PropertiesChanged signals. NMExportedObject would collect changed
properties and send the signal out in idle_emit_properties_changed()
on idle. This messes up the ordering of change events w.r.t. other
signals and events on the bus. Note that not only NMExportedObject
messed up the ordering. Also the generated code would hook into
notify() and process change events in and idle handle, exhibiting the
same ordering issue too.
No longer do that. PropertiesChanged signals will be sent right away
by hooking into dispatch_properties_changed(). This means, changing
a property in quick succession will no longer be combined and is
guaranteed to emit signals for each individual state. Quite possibly
we emit now more PropertiesChanged signals then before.
However, we are now able to group a set of changes by using standard
g_object_freeze_notify()/g_object_thaw_notify(). We probably should
make more use of that.
Also, now that our signals are all handled in the right order, we
might find places where we still emit them in the wrong order. But that
is then due to the order in which our GObjects emit signals, not due
to an ill behavior of the D-Bus glue. Possibly we need to identify
such ordering issues and fix them.
Numbers (for contrib/rpm --without debug on x86_64):
- the patch changes the code size of NetworkManager by
- 2809360 bytes
+ 2537528 bytes (-9.7%)
- Runtime measurements are harder because there is a large variance
during testing. In other words, the numbers are not reproducible.
Currently, the implementation performs no caching of GVariants at all,
but it would be rather simple to add it, if that turns out to be
useful.
Anyway, without strong claim, it seems that the new form tends to
perform slightly better. That would be no surprise.
$ time (for i in {1..1000}; do nmcli >/dev/null || break; echo -n .; done)
- real 1m39.355s
+ real 1m37.432s
$ time (for i in {1..2000}; do busctl call org.freedesktop.NetworkManager /org/freedesktop org.freedesktop.DBus.ObjectManager GetManagedObjects > /dev/null || break; echo -n .; done)
- real 0m26.843s
+ real 0m25.281s
- Regarding RSS size, just looking at the processes in similar
conditions, doesn't give a large difference. On my system they
consume about 19MB RSS. It seems that the new version has a
slightly smaller RSS size.
- 19356 RSS
+ 18660 RSS
nm_match_spec_device_by_pllink() does not support matching on all parameters,
unlike nm_match_spec_device(). The reason is that certain parameters are
only available when having a NMDevice instance.
Add an argument "match_device_type", so that the caller can inject the
device type to be used. Note that for NMDevice, the device-type is
nm_device_get_type_description(), which usually depends on the device
class only. The only caller of nm_match_spec_device_by_pllink() is the
wifi factory, and it already knows that it wants to create a device of
type NMDeviceWifi. Hence, it knows and can specify "wifi" as
match_device_type.
Add a variant of nm_device_spec_match_list() that looks up the match
paramters from a platform link instance.
Usually, we have a NMDevice instance that we use for matching.
However, at some places (like inside the device factory's
create_device() method), we might not have a NMDevice instance
to get the match paramters.
Add an alternative form, that gets the match paramters from a platform
link instance.
The code is placed inside src/NetworkManagerUtils.c, because
src/nm-core-utils.c is supposed to be independent of platform.
The matching works fuzzy and is not reliable. That is why we store
which connection should be assumed after restart in the state file
of NetworkManager.
In that case, we don't need to do a full check (with the possibility
of a false-reject). Just check for the minimum required properties:
the type and slave-type.
Yes, if the user modifies the connection while restarting NM, then
we might wrongly assume a connection that no longer would match.
But NM should not read minds, it should do as indicated.
Replace the usage of g_str_hash() with our own nm_str_hash().
GLib's g_str_hash() uses djb2 hashing function, just like we
do at the moment. The only difference is, that we use a diffrent
seed value.
Note, that we initialize the hash seed with random data (by calling
getrandom() or reading /dev/urandom). That is a change compared to
before.
This change of the hashing function and accessing the random pool
might be undesired for libnm/libnm-core. Hence, the change is not
done there as it possibly changes behavior for public API. Maybe
we should do that later though.
At this point, there isn't much of a change. This patch becomes
interesting, if we decide to use a different hashing algorithm.
When we generate the connection in nm_device_generate_connection(), we
add all routes that have rt_source (roundtrip(NM_IP_CONFIG_SOURCE_USER)).
Especially since commit e470e13922, this
includes automatically added host-routes to the gateway, added by
ip4_config_merge_and_apply().
Later, during nm_utils_match_connection() this route most not prevent
matching. Either nm_device_generate_connection() should not add it, or
nm_utils_match_connection() should ignore it.
I think adjusting the matching is better, because ip-configs are used
for several things, including exposing routes on D-Bus. We don't want
to hide this route on D-Bus.
Fixes: e470e13922https://bugzilla.redhat.com/show_bug.cgi?id=1487384
