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.
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.
- use GDBusConnection instead of GDBusProxy.
- rename "call-id" to "conf-id". It's really not a "call" but
configuration that gets added and NMPacrunnerManager ensures that
the configuration is send to pacrunner.
- let "conf-id" keep a reference to NMPacrunnerManager. For one,
when we remove configurations we need to call DestroyProxyConfiguration
to remove it again. We cannot just abort the requests but must linger
around until our configuration is properly cleaned up. Hence, we
anyway cannot destroy the NMPacrunnerManager earlier.
With respect to fixing shutdown not to leak anything, this merely
means that we must wait (and iterate the main loop) as long as
NMPacrunnerManager singleton still exits (that is anyway the plan
how to fix shutdown).
With these considerations it's also clear that our D-Bus calls must
have a stricter timeout: NM_SHUTDOWN_TIMEOUT_MS.
This is also nice because nm_pacrunner_manager_remove() no longer
needs a manager parameter, it can just rely on having a reference
to the manager.
- for logging the configuration IDs, don't log pointer values.
Logging pointer values should be avoided as it defeats ASLR.
Instead, give them a "log_id" number.
- pacrunner is a D-Bus activatable service. D-Bus activatable services
needs special care. We don't want to start it over and over again.
Instead, we only try to "StartServiceByName" if
- we have any configuration to add
- if pacrunner is currently confirmed not to be running (by watching
name owner changes)
- we didn't try to start it already. That means, only start it
at the beginning and afterwards set a flag to block it. When
we see pacrunner appear on D-Bus we always clear that flag,
that means if pacrunner drops of, we will try to restart it
(once).
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.
nm_ppp_manager_stop() wants to ensure that the pppd process is really
gone. For that it uses nm_utils_kill_child_async() to first send
SIGTERM, and sending SIGKILL after a timeout.
Later, we want to fix shutdown of NetworkManager to iterate the mainloop
during shutdown, so that such operations are still handled. However, we
can only delay shutdown for a certain time. After a timeout (NM_SHUTDOWN_TIMEOUT_MS
plus NM_SHUTDOWN_TIMEOUT_MS_GRACE) we really have to give up and
terminate.
That means, the right amount of time between sending SIGTERM and SIGKILL
is exactly NM_SHUTDOWN_TIMEOUT_MS. Hopefully that is of course
sufficient in the first place. If not, send SIGKILL afterwards, and give
a bit more time (NM_SHUTDOWN_TIMEOUT_MS_GRACE) to reap the child.
And if all this time is still not enough, something is really odd and we
abort waiting, with a warning in the logfile.
Since we don't properly handle shutdown yet, the description above is
not really true. But with this patch, we fix it from point of view of
NMPPPManager.
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.
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.
Now we only search for a candiate with matching UUID. No need to
first lookup all activatable connections, just find the candidate
by UUID and see if it is activatable.
"NetworkManagerUtils.h" contains a bunch of helper tools for core
daemon ("src/").
Unfortunately, it has dependencies to other parts of core,
such as "nm-device.h" and "nm-platform.h". Split out a part
of tools that are independent so that they can be used without
dragging in other dependencies.
"nm-core-utils.h" should only use libnm-core, "nm-logging.h"
and shared.
"NetworkManagerUtils.h" should provide all "nm-core-utils.h" and
possibly other utilities that have larger dependencies.
When NM tries to match a generated connection to a persistent one, it
considers also the metric of static routes. However, if the field is
set to -1 (use default value for the device) on the persistent
connection, the comparison will always fail because the generated
connection contains the actual value read from kernel.
To fix the issue, modify check_possible_match() to deal correctly with
-1 and translate it to the expected value for the current device when
performing the comparison.
This allows connections with static routes and default metric to
properly be re-assumed when NM is restarted.
https://bugzilla.redhat.com/show_bug.cgi?id=1302532
The compiler might be able to optimize the switch better.
But more importantly, it has the type information of the enum
and can give warnings about unmentioned enum values.
RFC7217 introduces an alternative mechanism for creating addresses during
stateless IPv6 address configuration. It's supposed to create addresses whose
host part stays stable in a particular network but changes when the hosts
enters another network to mitigate possibility of tracking the host movement.
It can be used alongside RFC 4941 privacy extensions (temporary addresses)
and replaces the use of RFC 4862 interface identifiers.
The address creation mode is controlld by ip6.addr_gen_mode property
(ADDR_GEN_MODE in ifcfg-rh), with values of "stable-privacy" and "eui-64",
defaulting to "eui-64" if unspecified.
The host part of an address is computed by hashing a system-specific secret
salted with various stable values that identify the connection with a secure
hash algorithm:
RID = F(Prefix, Net_Iface, Network_ID, DAD_Counter, secret_key)
For NetworkManager we use these parameters:
* F()
SHA256 hash function.
* Prefix
This is a network part of the /64 address
* Net_Iface
We use the interface name (e.g. "eth0"). This ensures the address won't
change with the change of interface hardware.
* Network_ID
We use the connection UUID here. This ensures the salt is different for
wireless networks with a different SSID as suggested by RFC7217.
* DAD_Counter
A per-address counter that increases with each DAD failure.
* secret_key
We store the secret key in /var/lib/NetworkManager/secret_key. If it's
shorter than 128 bits then it's rejected. If the file is not present we
initialize it by fetching 256 pseudo-random bits from /dev/urandom on
first use.
Duplicate address detection uses IDGEN_RETRIES = 3 and does not utilize the
IDGEN_DELAY delay (despite it SHOULD). This is for ease of implementation
and may change in future. Neither parameter is currently configurable.
Having a static string buffer for convenience is useful not only
for platform. Define the string buffer in NetworkManagerUtils.h,
so that all to-string functions can reuse *one* buffer.
Of course, this has the potential danger, that different
to-string method might reuse the same buffer. Hence, low-level
library functions are adviced to use their own buffer, because
an upper level might already use the global buffer for another
string.
Support a new configuration option
[.config]
enable=<ENABLED>
for configuration snippets.
This new [.config] section is only relevant within the snippet itself
and it is not merged into the combined configuration.
Currently only the "enable" key is supported. If the "enable" key is
missing, it obviously defaults to being enabled. It allows snippets
to be skipped from loading. The main configuration "NetworkManager.conf"
cannot be skipped.
<ENABLED> can be a boolean value (false), to skip a configuration
snippet from loading.
It can also be a string to match against the NetworkManager version,
like "enable=nm-version-min:1.1,nm-version-min:1.0.6"
There are several motivations for this:
- the user can disable an entire configuration snippet by toggeling
one entry.
This generalizes the functionality of the global-dns.enable
setting, but in a way that applies to configuration on a per-file
basis.
- for developing, we often switch between different versions of
NetworkManager. Thus, we might want to use different configuration.
E.g. before global-dns options, I want to use "dns=none" and manage
resolv.conf myself. Now, I can use global-dns setting to do that.
That can be achieved with something like the following (not exactly,
it's an example only):
[.config]
enable=nm-version-min:1.1
[main]
dns=default
[global-dns-domain-*]
nameserver=127.0.0.1
Arguably, this would be more awesome, if we would bump our micro devel
version (1.1.0) more often while developing 1.2.0 (*hint*).
- in principle, packages could drop configuration snippets and enable
them based on the NetworkManager version.
- with the "env:" spec, you can enable/disable snippets by configuring
an environment variable. Again, useful for testing and developing.
This is effectively the same as nm_config_parse_boolean(). The difference is,
that "nm-config.c" is not available to the interface-helper, thus any
code used by interface-helper (like "NetworkManager.c") cannot use this
function.
Still don't drop nm_config_parse_boolean() entirely, because it's better
to have the explicit notion of parsing a string in the config-context.
I ended up not using the function. But I'd still keep this patch.
