Some trivial changes:
- move nm_keep_alive_new() after nm_keep_alive_init(), so that the
functions that initialize the instance are beside each other.
- prefer nm_streq*() over strcmp().
- wrap some lines.
- remove some empty lines.
This allows binding the lifetime of the created connection to the
existance of the requesting dbus client. This feature is useful if one
has a service specific connection (e.g. P2P wireless) which will not be
useful without the specific service.
This is simply a mechanism to ensure proper connection cleanup if the
requesting service has a failure.
For P2P connections it makes sense to bind the connection to the status
of the operation that is being done. One example is that a wifi display
(miracast) P2P connection should be shut down when streaming fails for
some reason.
This new helper class allows binding a connection to the presence of a
DBus path meaning that it will be torn down if the process disappears.
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.
glib 2.56's g_steal_pointer() won't tolerate a function pointer in place
of a gpointer.
CC src/src_libNetworkManager_la-nm-active-connection.lo
src/nm-active-connection.c:1017:17: error: pointer type mismatch
('NMActiveConnectionAuthResultFunc' (aka 'void (*)(struct _NMActiveConnection *,
int, const char *, void *)') and 'gpointer' (aka 'void *'))
[-Werror,-Wpointer-type-mismatch]
result_func = g_steal_pointer (&priv->auth.result_func);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/include/glib-2.0/glib/gmem.h:200:6: note: expanded from macro 'g_steal_pointer'
(0 ? (*(pp)) : (g_steal_pointer) (pp))
^ ~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~
1 error generated.
There's just a single spot we use it that way, so it's perhaps better to
work around the warning instead of disabling it.
Until now, we only really cared about whether a connection was activated
with reason NM_ACTIVATION_REASON_AUTOCONNECT_SLAVES or not.
Now however, we will care about whether a connection was activated via
(genuine) autoconnect by NMPolicy, or external/assume by NMManager.
Add a new reason to distinguish between them.
The async authorization request also carries user-data and its result
must always be handled. For example, it might carry a GDBusMethodInvocation
context, which must be returned and freed.
Hence, when cancelling the request, we must always invoke the callback.
Also, when the NMActiveConnection progresses to state disconnected,
automatically abort the authorization request.
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.
NMDBusObject already gets this right, by calling nm_dbus_utils_get_property(),
which calls g_dbus_gvalue_to_gvariant(), which correctly converts NULL
object paths to "/".
We already rely on that elsewhere. No need for this workaround.
For one, these flags are "internal" flags. Soon, we will gain
a new NMSettingsConnectionFlags type that is exported on D-Bus
and partly overlaps with these internal flags. However, then we
will need the "flags" properties to expose the public bits.
This property only exists because other parts are interested in
notification signals. Note that we encourage NMDbusObject types
to freeze/thaw property-changed notifications. As freezing the
notifications also delays the signals, this is not desired for
the purpose where internal users subscribe to the signal.
"NMSettingsConnectionFlags" was an internal enum. Soon, we will add such
a type in libnm. Avoid the naming conflict by renaming. The "Int" stands
for "internal".
More of a proof of concept, how convenient (or not) it is to drop
NMAuthChain and use NMAuthManager's API directly. I think it's
reasonably nice.
As before, when asking for both general network-control permissions
and wifi-shared-permissions, we will not fail with
wifi-shared-permissions, as long as network-control check is still
pending. The effect is, that the error response preferably complains
about no permissions to network-control (in case both permissions
are missing).
One change in behavior is, if network-control authorization check
fails before wifi-shared-permissions, we declare the result and
cancel the pending wifi-shared-permissions. Previously, we would
have waited for both results. The change in behavior is not merely
that we declare the result earlier, but also that NMAuthManager will
actively send a "CancelCheckAuthorization" D-Bus call to cancel the
still pending wifi-shared-permissions check.
NMAuthChain is not really ref-counted. True, we have an internal ref-counter
to ensure that the instance stays alive while the callback is invoked. However,
the user cannot take additional references as there is no nm_auth_chain_ref().
When the user wants to get rid of the auth-chain, with the current API it
is important that the callback won't be called after that point. From the
name nm_auth_chain_unref(), it sounds like that there could be multiple references
to the auth-chain, and merely unreferencing the object might not guarantee that
the callback is canceled. However, that is luckily not the case, because
there is no real ref-counting involved here.
Just rename the destroy function to make this clearer.
When a settings-connection gets deleted, we need to bring down the
NMActiveConnection that contains it. However, we shouldn't just unexport
the active connection from D-Bus. Instead, clear the settings path.
We need to drop the path, because the connection is going away. It's a
bit ugly, that an active-connection might reference no
settings-connection. However, this only happens during shut-down.
The alternative, would be to keep the settings-connection object
in a zombie state, exported on D-Bus. However, that seems even more
confusing to me.
Essentially, nm_connection_get_path() mirros nm_dbus_object_get_path().
However, when cloning a simple-connection, the path also gets cloned.
I think this field doesn't belong to NMConnection in the first place,
because NMConnection is not a D-Bus object. NMSettingsConnection (in
core) and NMRemoteConnection (in libnm) is.
Don't use the misleading alias, but use nm_dbus_object_get_path()
directly.
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.
I dislike the static hash table to cache the integer counter for
numbered paths. Let's instead cache the counter at the class instance
itself -- since the class contains the information how the export
path should be exported.
However, we cannot use a plain integer field inside the class structure,
because the class is copied between derived classes. For example,
NMDeviceEthernet and NMDeviceBridge both get a copy of the NMDeviceClass
instance. Hence, the class doesn't contain the counter directly, but
a pointer to one counter that can be shared between sibling classes.
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
The accessor functions just look whether a certain flag is set. As these
functions have a different name then the flags, this is more confusing
then helpful. For example, if you want to know where the NM_GENERATED
flag matters, you had to know to grep for nm_settings_connection_get_nm_generated()
in addition to NM_SETTINGS_CONNECTION_FLAGS_NM_GENERATED.
The accessor function hid that the property was implemented as
a connection flag. For example, it was not immediately obvious
that nm_settings_connection_get_nm_generated() is the same
as having the NM_SETTINGS_CONNECTION_FLAGS_NM_GENERATED flag
set.
Drop them.
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.
For externally managed interfaces, we create an in-memory connection
and keep the device with sys-iface-state=external.
When the user actively modifies the connection, we persist it to
storage. But we also must take over managing the device.
One problem is that nm_device_reapply() errors out if the device
is still activating. It's unclear how to reapply the connection
while the device is in the process of activation. So, if the user
modifies the created connection very quickly, reapplying the settings
will fail.
https://bugzilla.redhat.com/show_bug.cgi?id=1462223
We react on changes to NMSettingsConnection.flags, so that we can update
from an external activation to a managed one.
However, previously we would only register the _settings_connection_notify_flags
callback during _set_settings_connection(). So, if via constructor properties
we first set PROP_SETTINGS_CONNECTION and later PROP_ACTIVATION_TYPE, we wouldn't
register the callback.
Note that the reason tracking starts as soon as the object exists (which
is immediately after GDBusObject is created), not when the asynchronous
NMObject initialization finishes. That is so that we the reason changes
in between are not lost.
The vpn-connection should probably be doing the same.
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.
An EXTERNAL activation type comes with a nm-generated, volatile,
in-memory connection. When the user actively modifies that connection, we
shall mark the active connection as fully managed.
We need a distinction between external activations and assuming
connections. The former shall have the meaning of devices that are
*not* managed by NetworkManager, the latter are configurations that
are gracefully taken over after restart (but fully managed).
Express that in the activation-type of the active connection.
Also, no longer use the settings NM_SETTINGS_CONNECTION_FLAGS_VOLATILE
flag to determine whether an assumed connection is "external". These
concepts are entirely orthogonal (although in pratice, external
activations are in-memory and flagged as volatile, but the inverse
is not necessarily true).
Also change match_connection_filter() to consider all connections.
Later, we only call nm_utils_match_connection() for the connection
we want to assume -- which will be a regular settings connection,
not a generated one.
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.
