core/dbus: rework D-Bus implementation to use lower level GDBusConnection APIth/dbus

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, and had
ordering-issues and 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: interface-infos and fiends.

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 more hacks to bend the generated
skeletons.

Note that the current implementation now only supports one D-Bus connection
While was effectively the case already, 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 that. 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.
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
  - 2809488 bytes
  + 2536840 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    1m49.864s
  + real    1m45.023s (-4.4%)

  $ 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)
  # the variance is too large, the timing seem mostly the same.

- 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.

1 files changed, 319 insertions, 0 deletions

diff --git a/src/nm-dbus-object.c b/src/nm-dbus-object.c
new file mode 100644
index 0000000000..b8670041fb
--- /dev/null
+++ b/src/nm-dbus-object.c
@@ -0,0 +1,319 @@
+/* -*- Mode: C; tab-width: 4; indent-tabs-mode: t; c-basic-offset: 4 -*- */
+/* NetworkManager -- Network link manager
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Copyright 2018 Red Hat, Inc.
+ */
+
+#include "nm-default.h"
+
+#include "nm-dbus-object.h"
+
+#include "nm-dbus-manager.h"
+
+/*****************************************************************************/
+
+static gboolean quitting = FALSE;
+
+void
+nm_dbus_object_set_quitting (void)
+{
+	nm_assert (!quitting);
+	quitting = TRUE;
+}
+
+/*****************************************************************************/
+
+NM_GOBJECT_PROPERTIES_DEFINE (NMDBusObject,
+	PROP_PATH,
+);
+
+G_DEFINE_ABSTRACT_TYPE (NMDBusObject, nm_dbus_object, G_TYPE_OBJECT);
+
+/*****************************************************************************/
+
+#define _NMLOG_DOMAIN      LOGD_CORE
+#define _NMLOG(level, ...) __NMLOG_DEFAULT_WITH_ADDR (level, _NMLOG_DOMAIN, "dbus-object", __VA_ARGS__)
+
+#define _NMLOG2_DOMAIN      LOGD_DBUS_PROPS
+#define _NMLOG2(level, ...) __NMLOG_DEFAULT_WITH_ADDR (level, _NMLOG2_DOMAIN, "properties-changed", __VA_ARGS__)
+
+/*****************************************************************************/
+
+static char *
+_create_export_path (NMDBusObjectClass *klass)
+{
+	const char *class_export_path, *p;
+	static GHashTable *prefix_counters;
+	guint64 *counter;
+
+	class_export_path = klass->export_path;
+
+	nm_assert (class_export_path);
+
+	p = strchr (class_export_path, '%');
+	if (p) {
+		if (G_UNLIKELY (!prefix_counters))
+			prefix_counters = g_hash_table_new (nm_str_hash, g_str_equal);
+
+		nm_assert (p[1] == 'l');
+		nm_assert (p[2] == 'l');
+		nm_assert (p[3] == 'u');
+		nm_assert (p[4] == '\0');
+
+		counter = g_hash_table_lookup (prefix_counters, class_export_path);
+		if (!counter) {
+			counter = g_slice_new0 (guint64);
+			g_hash_table_insert (prefix_counters, (char *) class_export_path, counter);
+		}
+
+		NM_PRAGMA_WARNING_DISABLE("-Wformat-nonliteral")
+		return g_strdup_printf (class_export_path, (unsigned long long) (++(*counter)));
+		NM_PRAGMA_WARNING_REENABLE
+	}
+
+	return g_strdup (class_export_path);
+}
+
+/**
+ * nm_dbus_object_export:
+ * @self: an #NMDBusObject
+ *
+ * Exports @self on all active and future D-Bus connections.
+ *
+ * The path to export @self on is taken from its #NMObjectClass's %export_path
+ * member. If the %export_path contains "%u", then it will be replaced with a
+ * monotonically increasing integer ID (with each distinct %export_path having
+ * its own counter). Otherwise, %export_path will be used literally (implying
+ * that @self must be a singleton).
+ *
+ * Returns: the path @self was exported under
+ */
+const char *
+nm_dbus_object_export (NMDBusObject *self)
+{
+	static guint64 id_counter = 0;
+
+	g_return_val_if_fail (NM_IS_DBUS_OBJECT (self), NULL);
+
+	g_return_val_if_fail (!self->internal.path, self->internal.path);
+
+	self->internal.path = _create_export_path (NM_DBUS_OBJECT_GET_CLASS (self));
+
+	self->internal.export_version_id = ++id_counter;
+
+	_LOGT ("export: \"%s\"", self->internal.path);
+
+	_nm_dbus_manager_obj_export (self);
+
+	_notify (self, PROP_PATH);
+	return self->internal.path;
+}
+
+/**
+ * nm_dbus_object_unexport:
+ * @self: an #NMDBusObject
+ *
+ * Unexports @self on all active D-Bus connections (and prevents it from being
+ * auto-exported on future connections).
+ */
+void
+nm_dbus_object_unexport (NMDBusObject *self)
+{
+	g_return_if_fail (NM_IS_DBUS_OBJECT (self));
+
+	g_return_if_fail (self->internal.path);
+
+	_LOGT ("unexport: \"%s\"", self->internal.path);
+
+	_nm_dbus_manager_obj_unexport (self);
+
+	g_clear_pointer (&self->internal.path, g_free);
+	self->internal.export_version_id = 0;
+
+	_notify (self, PROP_PATH);
+}
+
+/*****************************************************************************/
+
+void
+_nm_dbus_object_clear_and_unexport (NMDBusObject **location)
+{
+	NMDBusObject *self;
+
+	g_return_if_fail (location);
+	if (!*location)
+		return;
+
+	self = g_steal_pointer (location);
+
+	g_return_if_fail (NM_IS_DBUS_OBJECT (self));
+
+	if (self->internal.path)
+		nm_dbus_object_unexport (self);
+
+	g_object_unref (self);
+}
+
+/*****************************************************************************/
+
+void
+nm_dbus_object_emit_signal_variant (NMDBusObject *self,
+                                    const NMDBusInterfaceInfoExtended *interface_info,
+                                    const GDBusSignalInfo *signal_info,
+                                    GVariant *args)
+{
+	if (!self->internal.path) {
+		nm_g_variant_unref_floating (args);
+		return;
+	}
+	_nm_dbus_manager_obj_emit_signal (self, interface_info, signal_info, args);
+}
+
+void
+nm_dbus_object_emit_signal (NMDBusObject *self,
+                            const NMDBusInterfaceInfoExtended *interface_info,
+                            const GDBusSignalInfo *signal_info,
+                            const char *format,
+                            ...)
+{
+	va_list ap;
+
+	nm_assert (NM_IS_DBUS_OBJECT (self));
+	nm_assert (format);
+
+	if (!self->internal.path)
+		return;
+
+	va_start (ap, format);
+	_nm_dbus_manager_obj_emit_signal (self,
+	                                 interface_info,
+	                                 signal_info,
+	                                 g_variant_new_va (format, NULL, &ap));
+	va_end (ap);
+}
+
+/*****************************************************************************/
+
+static void
+get_property (GObject *object, guint prop_id,
+              GValue *value, GParamSpec *pspec)
+{
+	NMDBusObject *self = NM_DBUS_OBJECT (object);
+
+	switch (prop_id) {
+	case PROP_PATH:
+		g_value_set_string (value, self->internal.path);
+		break;
+	default:
+		G_OBJECT_WARN_INVALID_PROPERTY_ID (object, prop_id, pspec);
+		break;
+	}
+}
+
+static void
+dispatch_properties_changed (GObject *object,
+                             guint n_pspecs,
+                             GParamSpec **pspecs)
+{
+	NMDBusObject *self = NM_DBUS_OBJECT (object);
+
+	if (self->internal.path)
+		_nm_dbus_manager_obj_notify (self, n_pspecs, (const GParamSpec *const*) pspecs);
+
+	G_OBJECT_CLASS (nm_dbus_object_parent_class)->dispatch_properties_changed (object, n_pspecs, pspecs);
+}
+
+/*****************************************************************************/
+
+static void
+nm_dbus_object_init (NMDBusObject *self)
+{
+	c_list_init (&self->internal.objects_lst);
+	c_list_init (&self->internal.registration_lst_head);
+	self->internal.bus_manager = nm_g_object_ref (nm_dbus_manager_get ());
+}
+
+static void
+constructed (GObject *object)
+{
+	NMDBusObjectClass *klass;
+
+	G_OBJECT_CLASS (nm_dbus_object_parent_class)->constructed (object);
+
+	klass = NM_DBUS_OBJECT_GET_CLASS (object);
+
+	if (klass->export_on_construction)
+		nm_dbus_object_export ((NMDBusObject *) object);
+
+	/* NMDBusObject types should be very careful when overwriting notify().
+	 * It is possible to do, but this is a reminder that it's probably not
+	 * a good idea.
+	 *
+	 * It's not a good idea, because NMDBusObject uses dispatch_properties_changed()
+	 * to emit signals about a bunch of property changes. So, we want to make
+	 * use of g_object_freeze_notify() / g_object_thaw_notify() to combine multiple
+	 * property changes in one signal on D-Bus. Note that notify() is not invoked
+	 * while the signal is frozen, that means, whatever you do inside notify()
+	 * will not make it into the same batch of PropertiesChanged signal. That is
+	 * confusing, and probably not what you want.
+	 *
+	 * Simple solution: don't overwrite notify(). */
+	nm_assert (!G_OBJECT_CLASS (klass)->notify);
+}
+
+static void
+dispose (GObject *object)
+{
+	NMDBusObject *self = NM_DBUS_OBJECT (object);
+
+	/* Objects should have already been unexported by their owner, unless
+	 * we are quitting, where many objects stick around until exit.
+	 */
+	if (!quitting) {
+		if (self->internal.path) {
+			g_warn_if_reached ();
+			nm_dbus_object_unexport (self);
+		}
+	} else if (nm_clear_g_free (&self->internal.path)) {
+		/* FIXME: do a proper, coordinate shutdown, so that no objects stay
+		 * alive nor exported. */
+		_notify (self, PROP_PATH);
+	}
+
+	G_OBJECT_CLASS (nm_dbus_object_parent_class)->dispose (object);
+
+	g_clear_object (&self->internal.bus_manager);
+}
+
+static void
+nm_dbus_object_class_init (NMDBusObjectClass *klass)
+{
+	GObjectClass *object_class = G_OBJECT_CLASS (klass);
+
+	object_class->constructed = constructed;
+	object_class->dispose = dispose;
+	object_class->get_property = get_property;
+	object_class->dispatch_properties_changed = dispatch_properties_changed;
+
+	obj_properties[PROP_PATH] =
+	    g_param_spec_string (NM_DBUS_OBJECT_PATH, "", "",
+	                         NULL,
+	                         G_PARAM_READABLE |
+	                         G_PARAM_STATIC_STRINGS);
+
+	g_object_class_install_properties (object_class, _PROPERTY_ENUMS_LAST, obj_properties);
+}