NMSettings exposes a cached list of all connection. We don't need
to clone it. Note that this is not save against concurrent modification,
meaning, add/remove of connections in NMSettings will invalidate the
list.
However, it wasn't save against that previously either, because
altough we cloned the container (GSList), we didn't take an additional
reference to the elements.
This is purely a performance optimization, we don't need to clone the
list. Also, since the original list is of type "NMConnection *const*",
use that type insistently, instead of dependent API requiring GSList.
IMO, GSList is anyway not a very nice API for many use cases because
it requires an additional slice allocation for each element. It's
slower, and often less convenient to use.
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
Change the output of nm_platform_error_to_string() to print the numeric value.
Also, accept a string buffer instead of using an alloca() allocated buffer.
There is still a macro to provide the previous functionality, but it
was ill-suited to call from inside a loop.
The default value for miimon, when missing in the setting, is 0 if
arp_interval is != 0, and 100 otherwise. So, when generating a
connection, let's ignore miimon=0 (which means that miimon is
disabled) and accept any other value. Adding miimon=100 does not cause
any harm to the connection assumption.
While at it, slightly improve the code: ignore_if_zero() is not useful
for 'updelay','downdelay','arp_interval' because zero is their default
value, so introduce a new function that checks if the value is the
default (and specially handles 'miimon').
Reported-by: Taketo Kabe <rkabe@vega.pgw.jp>
https://bugzilla.redhat.com/show_bug.cgi?id=1463077
Previously, master device types like bridge, bond, and team
would overwrite is_available() and check_connection_available()
and always return TRUE.
The device already expresses via nm_device_is_master() that it
is of a master kind. Refactor the code, so, instead of having these
device types overwrite is_available() and check_connection_available(),
let the parents implementation react on nm_device_is_master().
There is no change in behavior at all. Instead, the knowledge how to
treat a master device moves from the device implementation to the
parent class.
Don't crash if the bond mode can't be read from sysfs - for example
when the interface disappears. The generated connection will be bogus,
but at that point it doesn't matter because the in-memory connection
will be destroyed.
Fixes: 056a973a4fhttps://bugzilla.redhat.com/show_bug.cgi?id=1459580
Reduce the use of NM_PLATFORM_GET / nm_platform_get() to get
the platform singleton instance.
For one, this is a step towards supporting namespaces, where we need
to use different NMNetns/NMPlatform instances depending on in which
namespace the device lives.
Also, we should reduce our use of singletons. They are difficult to
coordinate on shutdown. Instead there should be a clear order of
dependencies, expressed by owning a reference to those singelton
instances. We already own a reference to the platform singelton,
so use it and avoid NM_PLATFORM_GET.
(cherry picked from commit 94d9ee129d)
src/devices/nm-device-bond.c: In function 'check_changed_options':
src/devices/nm-device-bond.c:529:4: error: 'name' may be used uninitialized in this function [-Werror=maybe-uninitialized]
g_set_error (error,
^
src/devices/nm-device-bond.c:505:14: note: 'name' was declared here
const char *name, *value_a, *value_b;
^
src/devices/nm-device-bond.c:528:8: error: 'value_a' may be used uninitialized in this function [-Werror=maybe-uninitialized]
if (!nm_streq0 (value_a, value_b)) {
^
src/devices/nm-device-bond.c:505:21: note: 'value_a' was declared here
const char *name, *value_a, *value_b;
^
(cherry picked from commit f66de1dd0f)
The kernel doesn't accept an interface to be set as active_slave until
the interface is enslaved to the bond. Delay the initialization of the
property.
This argument is only relevant when the NMActStageReturn argument
indicates NM_ACT_STAGE_RETURN_FAILURE. In all other cases it is ignored.
Rename the argument to make the meaning clearer. The argument is passed
through several layers of code, it isn't obvious that this argument only
matters for the failure case. Also, the distinct name makes it easier
to distinguish from other uses of the "reason" name.
While at it, do some drive-by cleanup:
- use g_return_*() instead of g_assert() to have a more graceful
assertion.
- functions like dhcp4_start() don't need to return a failure reason.
Most callers don't care, and the caller who does can determine the
proper reason.
- allow omitting the out-argument via NM_SET_OUT().
Instead of overwriting ip4_config_pre_commit(), add a new function
get_mtu().
This also adds a default value in case there is no user-configuration.
This will allow us later to reset a default MTU based on the device
type.
This makes it easier to install the files with proper names.
Also, it makes the makefile rules slightly simpler.
Lastly, the documentation is now generated into docs/api, which makes it
possible to get rid of the awkward relative file names in docbook.
Keep the include paths clean and separate. We use directories to group source
files together. That makes sense (I guess), but then we should use this
grouping also when including files. Thus require to #include files with their
path relative to "src/".
Also, we build various artifacts from the "src/" tree. Instead of having
individual CFLAGS for each artifact in Makefile.am, the CFLAGS should be
unified. Previously, the CFLAGS for each artifact differ and are inconsistent
in which paths they add to the search path. Fix the inconsistency by just
don't add the paths at all.
We only needed proper glib enum types for having properties
and signal arguments. These got all converted to plain int,
so no longer generate such an enum type.
Had to rename "nm-enum-types.h" because it works badly with
"libnm/nm-enum-types.h". Maybe I could fix that differently,
but duplicate names is anyway error prone.
Note that "nm-core-enum-types.h" is already taken too, so
"nm-src-enum-types.h" it is.
An interface would make sense to allow the actual device-factory to inherit
from another type.
However, glib interfaces make code much harder to follow and less
efficient. The device factory shall be a very simple type with meta data
about supported device types and the ability to create device instances.
There is no need to make this an interface implementation, instead just
let the factories inherit from NM_TYPE_DEVICE_FACTORY directly.
- use _NM_GET_PRIVATE() and _NM_GET_PRIVATE_PTR() everywhere.
- reorder statements, to have GObject related functions (init, dispose,
constructed) at the bottom of each file and in a consistent order w.r.t.
each other.
- unify whitespaces in signal and properties declarations.
- use NM_GOBJECT_PROPERTIES_DEFINE() and _notify()
- drop unused signal slots in class structures
- drop unused header files for device factories
Previously we used defines for bond option names and used string
literals for their attribute names in sysfs. But they are the same by
definition so let's use defines also for attributes.
Even if the 'ad_actor_system' option is only valid for the 802.3ad
mode, the sysfs file is always present and has a default value of
''. But in 802.3ad mode the default value is
'00:00:00:00:00:00'. Return the correct value in
nm_setting_bond_get_option_default().
Furthermore, writing a empty string to the file will generate an
error, don't do it.
We print an error when the write of a bond options fails as this is
considered an effect of a wrong configuration (or a bug in the checks
done by NM) that the user should notice. But not all options are
supported in all bonding modes and so we ignore some unsupported
options for the current mode to avoid populating logs with useless
errors.
Improve the code there by using a more generic approach and
synchronize the mode/option compatibility table with kernel (file
drivers/net/bonding/bond_options.c).
https://bugzilla.gnome.org/show_bug.cgi?id=767776https://bugzilla.redhat.com/show_bug.cgi?id=1352131
When a value of a TYPE_BOTH option is read back from kernel it
contains both string and numeric values ("balance-rr 0"), so we must
chop off the number before adding the option to the setting. Also
change the default values of options to the string form so that the
option matching logic works.
- All internal source files (except "examples", which are not internal)
should include "config.h" first. As also all internal source
files should include "nm-default.h", let "config.h" be included
by "nm-default.h" and include "nm-default.h" as first in every
source file.
We already wanted to include "nm-default.h" before other headers
because it might contains some fixes (like "nm-glib.h" compatibility)
that is required first.
- After including "nm-default.h", we optinally allow for including the
corresponding header file for the source file at hand. The idea
is to ensure that each header file is self contained.
- Don't include "config.h" or "nm-default.h" in any header file
(except "nm-sd-adapt.h"). Public headers anyway must not include
these headers, and internal headers are never included after
"nm-default.h", as of the first previous point.
- Include all internal headers with quotes instead of angle brackets.
In practice it doesn't matter, because in our public headers we must
include other headers with angle brackets. As we use our public
headers also to compile our interal source files, effectively the
result must be the same. Still do it for consistency.
- Except for <config.h> itself. Include it with angle brackets as suggested by
https://www.gnu.org/software/autoconf/manual/autoconf.html#Configuration-Headers
When we decide to add a new link, we alredy checked that no such link exists
(ignoring race conditions).
It is wrong to accept a EXITS failure when adding the link. There is no guarantee
that the existing link has all the same properties as the one we intend to add.
More importantly, this link was added externally outside of NetworkManager and it
should not be taken over.
Just treat EXISTS as a failure as any other.
Link related functions should have a "nm_platform_link" prefix. Rename.
Naming is a subjective matter and one might argue that omitting
the "link" part from the name is shorter and even preferred.
However, I think functions related to links should have a common
prefix as the underlyings are strongly related.
Let the link-add functions return the internal pointer to the platform
link object. Similar to link-get, which doesn't copy the link either.
Also adjust the sole users of the add-functions (create-and-realize)
to take the pointer.
Eventually we still copy the returned data, because accessing platform can
invalidate the returned pointer. Thus we don't actually safe any copying
by this (at least every use of the function currently leads to the data
being copied).
Still change it, because I think the API of NMPlatform should look like that.
I found the handling of the master-device very confusing because it was
unclear who sets priv->master, and when it should be set.
Now:
- Setting priv->master (in a slave) always goes together with adding
the master to priv->slaves (in the master). Previously, this was
done at separate places, so it was not clear if master and slave
always agree on their relationship -- in fact, they did not.
- There are now three basic functions which do the enslaving/releasing:
(1) nm_device_master_add_slave()
(2) nm_device_master_enslave_slave()
(3) nm_device_master_release_one_slave()
Step 3/release basically undoes the 1/add and 2/enslave steps.
- completing the enslaving/releasing is now done by
(1) nm_device_slave_notify_enslave()
(2) nm_device_slave_notify_release()
These functions also emit signals like NM_DEVICE_MASTER.
- Derived classes no longer emit NM_DEVICE_SLAVES notification. Instead
the notification is emited together with NM_DEVICE_MASTER, whenever a
slaves changes state. Also, NM_DEVICE_SLAVES list now only exposes
slaves that are actually @is_enslaved.
Instead of reimplementing the slave property in bond, bridge
and team, just add the property to the parent class. It's not
that the parent class would be agnostic to the master/slave
implementation, all the slaves are known to the every device
type implementation.
Also, the derived class doesn't know the correct time when
to invoke the notify-changed for the slaves property.
E.g. it should be only invoked after nm_device_slave_notify_enslave()
when other components also consider the slave as enslaved.
Later this will be fixed so that the SLAVES property correspond
to what other master/slave related properties say.
release_slave() should do the right thing and handle errors as
good as it can. There is no value in propagating the error and
it's not clear what the caller should do in face of a failure
during release.
Ensure the platform link with the same interface name as the
NMDevice is actually compatible with it before using the link
for initialization of device properties. If not, remove the
NMDevice and create a new one since there are kernel resources
with a different type.
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
It might just be that we didn't see it yet; either on daemon startup on in a
race. The nm_platform_*_add() deals with the device already being there in
_link_add_check_existing().
NetworkManager:ERROR:devices/nm-device-bridge.c:402:create_and_realize: assertion failed: (nm_device_get_ifindex (device) <= 0)
Program received signal SIGABRT, Aborted.
0x00007ffff46965d7 in __GI_raise (sig=sig@entry=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:56
56 return INLINE_SYSCALL (tgkill, 3, pid, selftid, sig);
Missing separate debuginfos, use: debuginfo-install bluez-libs-5.23-4.el7.x86_64
(gdb) bt
#0 0x00007ffff46965d7 in __GI_raise (sig=sig@entry=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:56
#1 0x00007ffff4697cc8 in __GI_abort () at abort.c:90
#2 0x00007ffff4a916d5 in g_assertion_message (domain=domain@entry=0x5a5088 "NetworkManager", file=file@entry=0x59b0f2 "devices/nm-device-bridge.c", line=line@entry=402, func=func@entry=0x59b3f0 <__FUNCTION__.29169> "create_and_realize", message=message@entry=0xa76a30 "assertion failed: (nm_device_get_ifindex (device) <= 0)") at gtestutils.c:2292
#3 0x00007ffff4a9176a in g_assertion_message_expr (domain=domain@entry=0x5a5088 "NetworkManager", file=file@entry=0x59b0f2 "devices/nm-device-bridge.c", line=line@entry=402, func=func@entry=0x59b3f0 <__FUNCTION__.29169> "create_and_realize", expr=expr@entry=0x59aef8 "nm_device_get_ifindex (device) <= 0") at gtestutils.c:2307
#4 0x0000000000447cb6 in create_and_realize (device=0xa77f40 [NMDeviceBridge], connection=0x8d0200, parent=<optimized out>, out_plink=0x7fffffffd700, error=0x0) at devices/nm-device-bridge.c:402
#5 0x000000000045d560 in nm_device_create_and_realize (self=self@entry=0xa77f40 [NMDeviceBridge], connection=connection@entry=0x8d0200, parent=<optimized out>, error=error@entry=0x0)
at devices/nm-device.c:1594
#6 0x00000000004d4b64 in system_create_virtual_device (self=self@entry=0x8802b0 [NMManager], connection=connection@entry=0x8d0200, error=error@entry=0x0) at nm-manager.c:983
#7 0x00000000004d4d71 in system_create_virtual_devices (self=0x8802b0 [NMManager]) at nm-manager.c:1022
#8 0x00000000004d47a5 in add_device (self=<optimized out>, device=<optimized out>, try_assume=<optimized out>) at nm-manager.c:1785
#9 0x00000000004d501f in platform_link_added (self=self@entry=0x8802b0 [NMManager], ifindex=<optimized out>, plink=plink@entry=0xa7f810) at nm-manager.c:1887
#10 0x00000000004d7c24 in nm_manager_start (self=0x8802b0 [NMManager]) at nm-manager.c:1959
#11 0x00000000004d7c24 in nm_manager_start (self=self@entry=0x8802b0 [NMManager], error=error@entry=0x7fffffffd930) at nm-manager.c:4178
#12 0x00000000004459ec in main (argc=1, argv=0x7fffffffda88) at main.c:442
(gdb)
The localization headers are now included via "nm-default.h".
Also fixes several places, where we wrongly included <glib/gi18n-lib.h>
instead of <glib/gi18n.h>. For example under "clients/" directory.
