Previously, nm_active_connection_authorize() accepts two user-data
pointers for convenience.
nm_active_connection_authorize() has three callers. One only requires
one user-data, one passes two user-data pointers, and one requires
three pointer.
Also, the way how the third passes the user data (via
g_object_set_qdata_full()) is not great.
Let's only use one user-data pointer. We commonly do that, and it's easy
enough to allocate a buffer to pack multiple pointers together.
Specify a reason when creating active connections. The reason will be
used in the next commit to tell whether slaves must be reconnected or
not if a master has autoconnect-slaves=yes.
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
Using CList, we embed the list element in NMActiveConnection struct
itself. That means for example, that you couldn't track a
NMActiveConnection more then once. But we anyway never want that.
The advantage is, that removing an active connection from the list
is O(1), and we safe additional GSlice allocations for each node
element.
It includes a reason code that makes it possible for the clients to be
more reasonable about error messages.
The reason code is essentially copied from the VPN, plus three more
reasons that were useful for non-VPN connections.
When deciding whether to touch a device we sometimes look at whether
the active connection is external/assumed. In many cases however,
there is no active connection around (e.g. while moving the device
from state unmanaged to disconnected before assuming).
So in most cases we instead look at the device-state-reason to decide
whether to touch the interface (see nm_device_state_reason_check()).
Often it's desirable to have no state and passing data as function
arguments. However, the state reason has to be passed along several hops
(e.g. a queued state change). Or a change to a master/slave can affect
the slave/master, where we pass on the state reason. Or an intermediate
event might invalidate a previous state reason. Passing the state
whether to touch a device or not as a state-reason is cumbersome
and limited.
Instead, the device should be aware of whats going on. Add a
sys-iface-state with:
- SYS_IFACE_STATE_EXTERNAL: meaning, NM should not touch it
- SYS_IFACE_STATE_ASSUME: meaning, NM is gracefully taking over
- SYS_IFACE_STATE_MANAGED: meaning, the device is managed by NM
- SYS_IFACE_STATE_REMOVED: the device no longer exists
This replaces most checks of nm_device_state_reason_check() and
nm_active_connection_get_activation_type() by instead looking at
the sys-iface-state of the device.
This patch probably has still issues, but the previous behavior was
not very clear either. We will need to identify those issues in future
tests and tweak the behavior. At least, now there is one flag that
describes how to behave.
nm_device_uses_assumed_connection() basically called
nm_active_connection_get_assumed() on the device.
Rename those functions to be closer to the activation-type
flags.
The concepts of "assume", "external", and "assume_or_external"
will make sense with the following commits.
This field will be later used by NMDevice's Reapply and
GetAppliedConnection methods. The usecase is to first fetch
the currently applied connection, adjust it and reapply it.
Using the version-id, a concurrent modification can be detected
and Reapply can reject the invocation.
Clone the connection upon activation. This makes it safe for the user
to modify the original connection while it is activated.
This involves several changes:
- NMActiveConnection gets @settings_connection and @applied_connection.
To support add-and-activate, we constructing a NMActiveConnection with
no connection set. Previously, we would set the "connection" field to
a temporary NMConnection. Now NMManager piggybacks this temporary
connection as object-data (TAG_ACTIVE_CONNETION_ADD_AND_ACTIVATE).
- get rid of the functions nm_active_connection_get_connection_type()
and nm_active_connection_get_connection_uuid(). From their names
it is unclear whether this returns the settings or applied connection.
The (few) callers should figure that out themselves.
- rename nm_active_connection_get_id() to
nm_active_connection_get_settings_connection_id(). This function
is only used internally for logging.
- dispatcher calls now get two connections as well. The
applied-connection is used for the connection data, while
the settings-connection is used for the connection path.
- needs special handling for properties that apply immediately
when changed (nm_device_reapply_settings_immediately()).
Co-Authored-By: Thomas Haller <thaller@redhat.com>
https://bugzilla.gnome.org/show_bug.cgi?id=724041
Move D-Bus export/unexport handling into NMExportedObject and remove
type-specific export/get_path methods (export paths are now specified
at the class level, and NMExportedObject handles the counters for all
exported types automatically).
Since all exportable objects now use the same get_path() method, we
can also add some helper methods to simplify get_property()
implementations for object-path and object-path-array properties.
Add NMExportedObject, make it the base class of all D-Bus-exported
types, and move the nm-properties-changed-signal logic into it. (Also,
make NMSettings use the same properties-changed code as everything
else, which it was not previously doing, presumably for historical
reasons).
(This is mostly just shuffling code around at this point, but
NMExportedObject will be more important in the gdbus port, since
gdbus-codegen doesn't do a very good job of supporting objects that
export multiple interfaces [as each NMDevice subclass does, for
example], so we will need more glue/helper code in NMExportedObject
then.)
Rather than randomly including one or more of <glib.h>,
<glib-object.h>, and <gio/gio.h> everywhere (and forgetting to include
"nm-glib-compat.h" most of the time), rename nm-glib-compat.h to
nm-glib.h, include <gio/gio.h> from there, and then change all .c
files in NM to include "nm-glib.h" rather than including the glib
headers directly.
(Public headers files still have to include the real glib headers,
since nm-glib.h isn't installed...)
Also, remove glib includes from header files that are already
including a base object header file (which must itself already include
the glib headers).
Previously, src/nm-ip4-config.h, libnm/nm-ip4-config.h, and
libnm-glib/nm-ip4-config.h all used "NM_IP4_CONFIG_H" as an include
guard, which meant that nm-test-utils.h could not tell which of them
was being included (and so, eg, if you tried to include
nm-ip4-config.h in a libnm test, it would fail to compile because
nm-test-utils.h was referring to symbols in src/nm-ip4-config.h).
Fix this by changing the include guards in the non-API-stable parts of
the tree:
- libnm-glib/nm-ip4-config.h remains NM_IP4_CONFIG_H
- libnm/nm-ip4-config.h now uses __NM_IP4_CONFIG_H__
- src/nm-ip4-config.h now uses __NETWORKMANAGER_IP4_CONFIG_H__
And likewise for all other headers.
The two non-"nm"-prefixed headers, libnm/NetworkManager.h and
src/NetworkManagerUtils.h are now __NETWORKMANAGER_H__ and
__NETWORKMANAGER_UTILS_H__ respectively, which, while not entirely
consistent with the general scheme, do still mostly make sense in
isolation.
Clean up some of the cross-includes between headers (which made it so
that, eg, if you included NetworkManagerUtils.h in a test program, you
would need to build the test with -I$(top_srcdir)/src/platform, and if
you included nm-device.h you'd need $(POLKIT_CFLAGS)) by moving all
GObject struct definitions for src/ and src/settings/ into nm-types.h
(which already existed to solve the NMDevice/NMActRequest circular
references).
Update various .c files to explicitly include the headers they used to
get implicitly, and remove some now-unnecessary -I options from
Makefiles.
Add IP and DHCP config properties to the D-Bus ActiveConnection
objects.
For device connections, this is redundant with the properties already
on the Device object, but for VPN connections, this information was
not previously available.
Rather than explicitly passing around a UID and a flag saying whether
or not it's relevant.
(This also fixes a bug where the wrong UID was being recorded in
nm-settings-connection.c::auth_start(), which caused problems such as
agent-owned secrets not getting saved because of a perceived UID
mismatch.)
Various code during the activation paths will want to know whether
the connection is assumed or not, so that it doesn't do stuff that
touches the device.
The device may not be created yet (in the case of software devices)
when the ActiveConnection is created; in that case we still want to
proceed with authorization for the connection, but we'll create the
device when authorization is complete.
Both NMActRequest and NMVPNConnection need to track their device's state,
so instead of both subclasses having to do so, consolidate that code into
the superclass.
Add a 'master-ready' property to NMActiveConnection that NMDevice can
watch for to indicate that the master connection/device is ready to accept
slaves. Since the slave device's ActiveConnection is already tracking
its master connection, and since ActiveConnections don't enter the
ACTIVATING state until their device is ready for slaves, it's pretty
trivial to implement this property.
We need to track the master active connection, since it may require authentication
or other operations to complete before the device actually starts activating.
We want to create the object earlier now, and figuring out the master is
a lot of code that we don't want to run before creating the object. The
master still must be set before exporting the object over D-Bus though,
as before.
Whether an active connection is assumed or connected from scratch is
only important during nm_device_activate(). When the activation process
is set up, there's no difference from any other active connection.
Acked-by: Dan Winship <danw@gnome.org>
Acked-by: Thomas Haller <thaller@redhat.com>
Change the way that nm-properties-changed-signal works, and parse the
dbus-binding-tool-generated info to get the exact list of properties
that it's expected to export.
This makes NM_PROPERTY_PARAM_NO_EXPORT unnecessary, and also fixes the
problem of properties like NMDevice:hw-address being exported on
classes where it shouldn't be.
We'll want to use NMActiveConnection more in the manager and also fold
the PendingActivation functionality into it. All this functionality
applies to the VPN connections too, so it makes sense to have it all
in the base class instead of both NMActRequest and NMVPNConnection.
Track a master active connection and emit wait/ready/fail when
it changes state. This signal is intended for devices to
delay their activation until a master device is ready.
That was always the goal, but never got there. This time we need it
for real to abstract handling of dependent connections so bite the
bullet and make it happen.
Adds a new "master" property to NMActiveConnection containing the path
of the master NMDevice if the connection has a master.
Signed-off-by: Thomas Graf <tgraf@redhat.com>
A convenience so that clients which might key certain operations off
which connections are active (checking work mail only when on VPN for
example) can more easily get which connections are active. This would
allow those apps to store the UUID (which they would already be doing)
and not have to create a Connection proxy and then get the connection
properties just to retrieve the UUID of the connection. Instead they
can now get it from GetAll of the ActiveConnection object, which they
would already be doing.
Due to limitations in dbus-glib, where one GObject cannot have more
than one introspection XML object attached to it, we used to include
more than one <interface> in the VPNConnection object introspection
XML. This was suboptimal for two reasons:
1) it duplicated the Connection.Active introspection XML which
made it harder for clients to use the introspection data in a
dynamic fashion, besides looking ugly in the docs
2) not many other programs use this feature of dbus-glib, which
means it didn't get a lot of testing, and broke, which sucks
for NM.
To fix this issue, create a base class for NMVpnConnection that
handles the Connection.Active API, and make NMVpnConnection itself
handle just the VPN pieces that it layers on top. This makes
dbus-glib happy because we aren't using two <interface> blocks
in the same introspection XML, and it makes the NM code more
robust because we can re-use the existing Connection.Active
introspection XML in the NMVpnConnectionBase class.
