Introduce a new ifcfg-rh variable ACD_TIMEOUT that stores the exact
value of ipv4.dad-timeout without rounding. We still write the
initscripts-compatible ARPING_WAIT variable, and read it when
ACD_TIMEOUT is missing.
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.
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.
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
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
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
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.
The connection.mdns setting is a per-connection setting,
so one might expect that one activated device can only have
one MDNS setting at a time.
However, with certain VPN plugins (those that don't have their
own IP interface, like libreswan), the VPN configuration is merged
into the configuration of the device. So, in this case, there
might be multiple settings for one device that must be merged.
We already have a mechanism for that. It's NMIP4Config. Let NMIP4Config
track this piece of information. Although, stricitly speaking this
is not tied to IPv4, the alternative would be to introduce a new
object to track such data, which would be a tremendous effort
and more complicated then this.
Luckily, NMDnsManager and NMDnsPlugin are already equipped to
handle multiple NMIPConfig instances per device (IPv4 vs. IPv6,
and Device vs. VPN).
Also make "connection.mdns" configurable via global defaults in
NetworkManager.conf.
"UNKNOWN" is not a good name. If you don't set the property
in the connection explicitly, it should be "DEFAULT".
Also, make "DEFAULT" -1. For one, that ensures that the enum's
underlying integer type is signed. Otherwise, it's cumbersome
to test "if (mdns >= DEFAULT)" because in case of unsigned types,
the compiler will warn about the check always being true.
Also, it allows for "NO" to be zero. These are no strong reasons,
but I tend to think this is better.
Also, don't make the property of NMSettingConnection a CONSTRUCT property.
Initialize the default manually in the init function.
Also, order the numeric values so that DEFAULT < NO < RESOLVE < YES with
YES being largest because it enables *the most*.
Tests are commonly created via copy&paste. Hence, it's
better to express a certain concept explicitly via a function
or macro. This way, the implementation of the concept can be
adjusted at one place, without requiring to change all the callers.
Also, the macro is shorter, and brevity is better for tests
so it's easier to understand what the test does. Without being
bothered by noise from the redundant information.
Also, the macro knows better which message to expect. For example,
messages inside "src" are prepended by nm-logging.c with a level
and a timestamp. The expect macro is aware of that and tests for it
#define NMTST_EXPECT_NM_ERROR(msg) NMTST_EXPECT_NM (G_LOG_LEVEL_MESSAGE, "*<error> [*] "msg)
This again allows the caller to ignore this prefix, but still assert
more strictly.
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.
When a new settings-connection is populated with the actual settings
read from disk by the plugin, calling nm_settings_connection_update()
with KEEP mode also marks it as unsaved, which should not happen on a
new connection just written to (or read from) disk.
Introduce a new KEEP_SAVED persist mode that is similar to KEEP but
clears the UNSAVED flag.
Fixes: 023ce50d21https://bugzilla.redhat.com/show_bug.cgi?id=1525078
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
src/dns/nm-dns-manager.c: In function ‘write_to_netconfig’:
src/dns/nm-dns-manager.c:387:8: error: ignoring return value of ‘write’, declared with attribute warn_unused_result [-Werror=unused-result]
write (fd, str, strlen (str));
^
src/settings/plugins/ifnet/nms-ifnet-connection-parser.c: In function ‘ifnet_update_parsers_by_connection’:
src/settings/plugins/ifnet/nms-ifnet-connection-parser.c:2600:26: error: variable ‘pppoe’ set but not used [-Werror=unused-but-set-variable]
gboolean wired = FALSE, pppoe = TRUE;
^~~~~
While at it, don't log line breaks in ifnet_update_parsers_by_connection().
Fixes: e912b36d95
There are a few cases where we don't want to clear a potential
nm-generated/volatile flag, but only mark the connection as
unsaved.
Otherwise, we wrongly end up clearing these flags and the connection
is wrongly not NM_DEVICE_SYS_IFACE_STATE_EXTERNAL.
Fixes: 35dc6421de
