Get rid of the NMAuthChain layer.
I think NMAuthChain only makes sense if we schedule multiple requests
together for the same topic. But NMSettingsConnection never does that:
each D-Bus request corresponds to only one polkit authorization request.
So, we can just call NMAuthManager directly.
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.
There are multiple tests with the same in different directories; add a
unique prefix to test names so that it is clear from the output which
one is running.
NM_CONTROLLED=no has the primary use of marking devices as unmanaged.
For that to work, the ifcfg file must contain either a MAC address,
an interface-name, or s390-subchannels that match a device.
In case the profile doesn't contain such specifiers, the profile
is ignored and a warning was logged:
<warn> [1522849679.7866] ifcfg-rh: loading "/etc/sysconfig/network-scripts/ifcfg-ens99" fails: NM_CONTROLLED was false but device was not uniquely identified; device will be managed
Downgrade this warning to a debug message. It's not unreasonable
that a user marks a ifcfg file with NM_CONTROLLED=no, to avoid
NetworkManager handling it. Yes, that way, the user did not explicitly
mark a device as unmanaged. But NetworkManager will ignore the profile,
as the user might resonably desire. No need to warn about that.
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.
The wireless mode property can be unset (NULL), in which case it
assumed to be equivalent to "infrastructure". If we convert an unset
mode to infrastructure, the connection will change on write,
triggering errors like:
settings-connection[...]: write: successfully updated (ifcfg-rh: persist (null)), connection was modified in the process
device (wlp4s0): Activation: (wifi) access point 'test1' has security, but secrets are required.
device (wlp4s0): state change: config -> need-auth (reason 'none', sys-iface-state: 'managed')
device (wlp4s0): The connection was modified since activation
device (wlp4s0): state change: need-auth -> failed (reason 'no-secrets', sys-iface-state: 'managed')
To fix this, remove the MODE key when the mode is unset so that the
property is read back exactly as it was. Note that initscripts need
the MODE set, but in most cases there are other issues that make Wi-Fi
connection written by NM not compatible with initscripts.
https://bugzilla.redhat.com/show_bug.cgi?id=1549972
The g_clear_pointer() macro already contains a cast to GDestroyNotify. No
need to do it ourself. In fact, with the cast, this only works with the
particular g_clear_pointer() implementation, that first assigns the
destroy function to a local variable.
See-also: https://bugzilla.gnome.org/show_bug.cgi?id=674634#c52
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.
The generated code is really just a thin wrapper around direct
GDBusProxy calls. GDBusProxy is reasonably convenient to use directly,
drop this wrapper.
We also don't use a generated wrapper for other cases where
NetworkManager acts as a D-Bus client. There is no reason to
do it in this case.
While the nmdbus_*() functions that we were using are small wrappers,
we also created a NMDBusSecretAgent instance, and hence several other
functions and symbols are used as well. It's unnecessary.
This saves 8552 bytes for NetworkManager binary (2817944 vs. 2809392
bytes for contrib/rpm on x86_64).
Always read and load ipv4 property values when method is shared also if
they will not be used: instead of dropping them at connection update,
keep their values in the ifcfg file.
Exceptions: ipv4.dns and ipv4.dns-search. They will be not read, otherwise
they may trigger a failure in nm-setting-ip4-config.c:verify() on load.
https://bugzilla.redhat.com/show_bug.cgi?id=1519299
After writing the connection to disk and rereading it, in addition to
restoring agent-owned secrets in the cache we must also restore
agent-owned secrets from the original connections since they are lost
during the write.
Reported-by: Märt Bakhoff <anon@sigil.red>
https://bugzilla.gnome.org/show_bug.cgi?id=793324
The ifcfg-rh plugin provides its own D-Bus service which initscripts
query to determine whether NetworkManager handles an ifcfg file.
Rework the D-Bus glue to hook GDBus with NetworkManager to use
GDBusConnection directly. Don't use generated code, don't use
GDBusInterfaceSkeleton.
We still keep "src/settings/plugins/ifcfg-rh/nm-ifcfg-rh.xml"
and still compile the static generated code. We don't actually need
them anymore, maybe the should be dropped later.
This is a proof of concept for reworking the D-Bus glue in
NetworkManager core to directly use GDBusConnection. Reworking core is
much more complicated, because there we also have properties, and a
class hierarchy.
Arguably, for the trivial ifcfg-rh service all this hardly matters, because
the entire D-Bus service only consists of one method, which is unlikely to
be extended in the future.
Now we get rid of layers of glue code, that were hard to comprehend.
Did you understand how nm_exported_object_skeleton_create() works
and uses the generated code and GDBusInterfaceSkeleton to hook into
GDBusConnection? Congratulations in that case. In my opinion, these
layers of code don't simplify but complicate the code.
The change also reduces the binary size of "libnm-settings-plugin-ifcfg-rh.so"
(build with contrib/rpm --without debug) by 8312 bytes (243672 vs. 235360).
In _dbus_setup(), call _dbus_clear(). It feels more correct to do that.
Although, technically, we never even call _dbus_setup() if there is
anything to clear.
Also, minor refactoring of config_changed_cb(). It's not entirely clear
whether we need that code, or how to handle D-Bus disconnecting, if at all.
We commonly only allow tabs at the beginning of a line, not
afterwards. The reason for this style is so that the code
looks formated right with tabstop=4 and tabstop=8.
XXX was used to either raise attention (NOTE) or to indicate
that this is ugly code that should be fixed (FIXME). The usage
was inconsistent.
Let's avoid XXX and use either NOTE or FIXME.
We also unconditionally use them with autotools.
Also, the detection for have_version_script does
not seem correct to me. At least, it didn't work
with clang.
Some targets are missing dependencies on some generated sources in
the meson port. These makes the build to fail due to missing source
files on a highly parallelized build.
These dependencies have been resolved by taking advantage of meson's
internal dependencies which can be used to pass source files,
include directories, libraries and compiler flags.
One of such internal dependencies called `core_dep` was already in
use. However, in order to avoid any confusion with another new
internal dependency called `nm_core_dep`, which is used to include
directories and source files from the `libnm-core` directory, the
`core_dep` dependency has been renamed to `nm_dep`.
These changes have allowed minimizing the build details which are
inherited by using those dependencies. The parallelized build has
also been improved.
