Use the path instead. This drop an useless use of the "name" property,
which is, coincidentally also wrong. (We use "ibft" in the plugin path
whereas the property is set to "iBFT".)
It's actually annoying, useless and wraps over even on wide displays.
Let's make it consistent with the log line we use for device plugins.
Also, this drops the last use of the "info" property and one useless use
of the "name" property.
There is no need to perform a lookup by path. NMSettings is a singleton,
it has the connection exactly iff the connection is linked.
Also add an assertion to double-check that the results agree with
the previous implementation.
nm_settings_add_connection_dbus() has two callers. One of them is NMManager
during AddAndActivate. In this case, the NMActiveConnection already created
an auth-subject. Re-use it.
Note how creating an auth-subject involves reading procfs to determine
whether the process still exists. This is not about the additional
overhead of that, but about the race where the process could drop
of in the meantime. The calling process might be gone now, and we would
fail creating the auth-subject. There is no need for that, because we
already evaluated all information we need. Quite likely, in the case
of this race, PolicyKit will also determine that the process is gone
and fail authorization too. But that's PolicyKit's decision to make,
not nm_settings_add_connection_dbus()'s.
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.
We also don't emit the PropertiesChanged signal while connections are
not loaded. Maybe that is wrong, in any case, the property should agree
with the way how we emit notifications. So, for now, make the property
agree with not notifying about connections during startup.
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.
NMSettings already references NMSettingsConnection. Hence, it should not
at the same time reference itself. Arguably, during shutdown we do not properly
release all NMSettingsConnection. For example, there is no nm_settings_stop().
But that is a bug that needs fixing.
No need to keep the NMSettings instance alive here. If this is really
necessary, it needs fixing somewhere else. Besides, we know that we leak
a lot during shutdown, so this needs more work to do a clean shutdown.
No longer rely on nm_connection_get_path() being meaningful in server.
It also was wrong. During update, nm_settings_connection_update()
would call
nm_utils_log_connection_diff (replace_connection, NM_CONNECTION (self), ...
where replace_connection has no path set, and nothing was logged.
Fix it, by explicitly passing the D-Bus path. Also, because
nm-core-utils.c should be independent of nm-dbus-object.h.
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.
This wasn't a problem, because load_plugins() can only fails
if the settings plugins fail to load, which can only happen
if you have a broken installation.
Don't subscribe twice to the same signal. The more subscribers a
signal has, the more confusing it gets what is happening.
We can handle also the default-wired-connection in the regular
connection-removed signal.
Note how connection_removed() is registered with
g_signal_connect_after(), but that is fine. There are few subscribers
to this signal (that don't do anything that interferes here).
Especially, since all other subscribers subscribe with the same
priority (hence, are unordered). So, moving this task explicitly
to after, does not change any ordering guarantee -- in fact, it
ensures an ordering that was undefined previously. Anyway, it
doesn't matter.
connections_cached_list stays only valid until we remove/add connections
to NMSettings. Using the list without cloning requires to be aware of that.
When clearing the list, invalidate all pointers, in the hope that a following
use-after-free will blow up with an assertion.
We only do this in elevated assertion mode. It's not to prevent any bugs,
it's to better notice it.
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 next commit will completely rework NMBusManager and replace
NMExportedObject by a new type NMDBusObject.
Originally, NMDBusObject was added along NMExportedObject to ease
the rework and have compilable, intermediate stages of refactoring. Now,
I think the new name is better, because NMDBusObject is very strongly related
to the bus manager and the old name NMExportedObject didn't make that
clear.
I also slighly prefer the name NMDBusObject over NMBusObject, hence
for consistancy, also rename NMBusManager to NMDBusManager.
This commit only renames the file for a nicer diff in the next commit.
It does not actually update the type name in sources. That will be done
later.
Even Gentoo disables this plugin since before 0.9.8 release
of NetworkManager. Time to say goodbye.
If somebody happens to show up to maintain it, we may resurrect it
later.
If "$distro_plugins=ifnet" was set, configure.ac would use that
to autodetect --with-hostname-persist=gentoo. Replace that autodetect
part by checking for /etc/gentoo-release file.
If a volatile connection is deleted by user when it was already being
deleted internally because the device vanished, we may hit the
following failed assertion:
file src/settings/nm-settings-connection.c: line 2196
(nm_settings_connection_signal_remove): should not be reached
The @removed flag keeps track of whether we already signaled the
connection removal. Instead of throwing an assertion if we try to emit
the signal again, just return without action because this can happen
in the situation described above.
While at it, remove the @allow_reuse argument from
nm_settings_connection_signal_remove(): we should never emit the
signal twice. Instead, we should reset the @removed flag when the
connection is added.
Fixes: a9384452edhttps://bugzilla.redhat.com/show_bug.cgi?id=1506552
We already need to re-emit the notify::flags signal.
It's cumbersome to do this for boolean properties, so
re-use the flags to also track the visibility state.
Settings plugins now return the connection that was reread from file
when adding a connection, which means that any agent-owned secret is
lost. Ensure that we don't forget agent-owned secrets by caching them
and readding them to the new connection returned by plugins.
Fixes: 8a1d483ca8
Fixes: b4594af55ehttps://bugzilla.gnome.org/show_bug.cgi?id=789383
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.
A property preferably only emits a notify-changed signal when
the value actually changes and it caches the value (so that
between property-changed signals the value is guaranteed not to change).
NMSettings and NMManager both already cache the hostname, because
NMHostnameManager didn't guarantee this basic concept.
Implement it and rely on it from NMSettings and NMPolicy.
And remove the copy of the property from NMManager.
Move the call for nm_dispatcher_call_hostname() from NMHostnameManager
to NMManager. Note that NMPolicy also has a call to the dispatcher
when set-transient-hostname returns. This should be cleaned up later.
Hostname management is complicated. At least, how it is implemented currently.
For example, NMPolicy also sets the hostname (NMPolicy calls
nm_settings_set_transient_hostname() to have hostnamed set the hostname,
but then falls back to sethostname() in settings_set_hostname_cb()).
Also, NMManager tracks the hostname in NM_MANAGER_HOSTNAME too, and
NMPolicy listens to changes from there -- instead of changes from
NMSettings.
Eventually, NMHostnameManager should contain the hostname parts from NMSettings
and NMPolicy.
As we try to set the hostname through dbus, we should also try to
retrieve current hostname value from dbus first: otherwise we may end
retrieving the "old" hostname via gethostname while the dbus hostnamed
updated is pending.
