dbus: fix emitting D-Bus NetworkManager's old-style PropertiesChange signal

Before switching to gdbus (before 1.2.0), NetworkManager used dbus-glib.
Most objects in the D-Bus API with properties had a signal
NetworkManager-specific "PropertiesChanged" signal. Nowadays, this way of
handling of property changes is deprecated for the common "PropertiesChanged"
signal on the "org.freedesktop.DBus.Properties" interface.

There were a few pecularities in 1.0.0 and earlier:

  (1) Due to the implementation with dbus-glib, a property-changed
    signal was emitted on *all* interfaces. For example:
      - a change on a NMDeviceVeth of "NMDeviceEthernet.HwAddress" would be
        emitted both for the interfaces "fdo.NM.Device.Ethernet" and
        "fdo.NM.Device.Veth". Note that NMDeviceVeth is derived from
        NMDeviceEthernet and there is no "HwAddress" on veth device.
      - a change of "NMVpnConnection.VpnState" was emitted on both
        interfaces "fdo.NM.VPN.Connection" and "fdo.NM.Connecion.Active".
        Note that NMActiveConnection is the parent type of NMVpnConnection and
        only the latter has a property "VpnState".
  (2) NMDevice's "fdo.NM.Device" interface  doesn't have a "PropertiesChanged"
    signal. From (1) follows that all property-changes for this type were instead
    invoked with an interface like "fdo.NM.Device.Ethernet" (or multiple
    interfaces in case of NMDeviceVeth).

1.2.0 introduced gdbus, which gives us the standard "fdo.DBus.Properties"
signal. However, it made the mistake of not realizing (1), thus instead
of emitting the signal once for each interface, it would pick the first
one in the inheritance tree.

With 1.4.0, a bug from merge commit 844345e caused signals for devices
to be only emitted for the interface "fdo.NM.Device.Statistics", instead
of "fdo.NM.Device.Ethernet" or "fdo.NM.Device.Veth" (or both).

The latter is what bgo#770629 is about and what commit 82e9439 tried to fix.
However, the fix was wrong because it tried to do the theoretically correct
thing of emitting the property-changed signal exactly once for the
interface that actually ontains the property. In addition, it missed that
NMDevice doesn't have a PropertiesChanged signal, which caused signals for
"fdo.NM.Device" to get lost *sigh*.

Now, restore the (broken) behavior of 1.0.0. These old-style property changed
signals are anyway considered deprecated and exist solely to satisfy old clients
and preserve the old API.

Fixes: 63fbfad3705db5901e6a2a6a2fc332da0f0ae4be

https://bugzilla.gnome.org/show_bug.cgi?id=770629
https://bugzilla.redhat.com/show_bug.cgi?id=1371920

2 files changed, 81 insertions, 12 deletions

diff --git a/src/nm-exported-object.c b/src/nm-exported-object.c
index d99a9ceeb6..6cb5157e9c 100644
--- a/src/nm-exported-object.c
+++ b/src/nm-exported-object.c
@@ -27,6 +27,10 @@
 
 #include "nm-bus-manager.h"
 
+#include "nm-device.h"
+#include "nm-active-connection.h"
+#include "nmdbus-device-statistics.h"
+
 #if NM_MORE_ASSERTS >= 2
 #define _ASSERT_NO_EARLY_EXPORT
 #endif
@@ -267,8 +271,8 @@ nm_exported_object_class_add_interface (NMExportedObjectClass *object_class,
 		                  nm_exported_object_class_info_quark (), classinfo);
 	}
 
-	classinfo->skeleton_types = g_slist_append (classinfo->skeleton_types,
-	                                            GSIZE_TO_POINTER (dbus_skeleton_type));
+	classinfo->skeleton_types = g_slist_prepend (classinfo->skeleton_types,
+	                                             GSIZE_TO_POINTER (dbus_skeleton_type));
 
 	/* Ensure @dbus_skeleton_type's class_init has run, so its signals/properties
 	 * will be defined.
@@ -499,6 +503,9 @@ nm_exported_object_create_skeletons (NMExportedObject *self,
 	}
 	nm_assert (i == 0);
 
+	/* The list of interfaces priv->interfaces is to be sorted from parent-class to derived-class.
+	 * On the other hand, if one class defines multiple interfaces, the interfaces are sorted in
+	 * the order of calls to nm_exported_object_class_add_interface(). */
 	if (priv->num_interfaces > 0) {
 		memcpy (&interfaces[num_interfaces], priv->interfaces, sizeof (InterfaceData) * priv->num_interfaces);
 		g_slice_free1 (sizeof (InterfaceData) * priv->num_interfaces, priv->interfaces);
@@ -801,12 +808,11 @@ idle_emit_properties_changed (gpointer self)
 		if (n == 0)
 			continue;
 
-		if (!ifdata->property_changed_signal_id)
-			goto next;
+		nm_assert (ifdata->property_changed_signal_id);
 
 		/* We use here alloca in a loop, something that is usually avoided.
 		 * But the number of interfaces "priv->num_interfaces" is small (determined by
-		 * the depth of the type inheritence) and the number of possible pending_notifies
+		 * the depth of the type inheritance) and the number of possible pending_notifies
 		 * "n" is small (determined by the number of GObject properties). */
 		values = g_alloca (sizeof (values[0]) * n);
 
@@ -834,7 +840,6 @@ idle_emit_properties_changed (gpointer self)
 
 		g_signal_emit (ifdata->interface, ifdata->property_changed_signal_id, 0, variant);
 
-next:
 		g_hash_table_remove_all (ifdata->pending_notifies);
 	}
 
@@ -844,15 +849,21 @@ next:
 static void
 nm_exported_object_notify (GObject *object, GParamSpec *pspec)
 {
+	NMExportedObject *self = (NMExportedObject *) object;
 	NMExportedObjectPrivate *priv = NM_EXPORTED_OBJECT_GET_PRIVATE (object);
 	NMExportedObjectClassInfo *classinfo;
 	GType type;
 	const char *dbus_property_name = NULL;
 	GValue value = G_VALUE_INIT;
+	GVariant *value_variant;
 	InterfaceData *ifdata = NULL;
 	const GVariantType *vtype;
 	guint i, j;
 
+	/* Hook to emit deprecated "PropertiesChanged" signal on NetworkManager interfaces.
+	 * This is to preserve deprecated D-Bus API, nowadays we use instead
+	 * the "PropertiesChanged" signal of "org.freedesktop.DBus.Properties". */
+
 	if (priv->num_interfaces == 0)
 		return;
 
@@ -888,14 +899,58 @@ nm_exported_object_notify (GObject *object, GParamSpec *pspec)
 vtype_found:
 	g_value_init (&value, pspec->value_type);
 	g_object_get_property (G_OBJECT (object), pspec->name, &value);
-
-	/* @dbus_property_name is inside classinfo and never freed, thus we don't clone it.
-	 * Also, we do a pointer, not string comparison. */
-	g_hash_table_insert (ifdata->pending_notifies,
-	                     (gpointer) dbus_property_name,
-	                     g_dbus_gvalue_to_gvariant (&value, vtype));
+	value_variant = g_dbus_gvalue_to_gvariant (&value, vtype);
 	g_value_unset (&value);
 
+	if (   (   NM_IS_DEVICE (self)
+	        && !NMDBUS_IS_DEVICE_STATISTICS_SKELETON (ifdata->interface))
+	    || NM_IS_ACTIVE_CONNECTION (self)) {
+		/* This PropertiesChanged signal is nodaways deprecated in favor
+		 * of "org.freedesktop.DBus.Properties"'s PropertiesChanged signal.
+		 * This function solely exists to raise the NM version of PropertiesChanged.
+		 *
+		 * With types exported on D-Bus that are implemented as derived
+		 * types in glib (NMDevice and NMActiveConnection), multiple types
+		 * in the inheritance tree define a "PropertiesChanged" signal.
+		 *
+		 * In 1.0.0 and earlier, the signal was emitted once for every interface
+		 * that had a "PropertiesChanged" signal. For example:
+		 *   - NMDeviceEthernet.HwAddress was emitted on "fdo.NM.Device.Ethernet"
+		 *     and "fdo.NM.Device.Veth" (if the device was of type NMDeviceVeth).
+		 *   - NMVpnConnection.VpnState was emitted on "fdo.NM.Connecion.Active"
+		 *     and "fdo.NM.VPN.Connection".
+		 *
+		 * NMDevice is special in that it didn't have a "PropertiesChanged" signal.
+		 * Thus, a change to "NMDevice.StateReason" would be emitted on "fdo.NM.Device.Ethernet"
+		 * and also on "fdo.NM.Device.Veth" (in case of a device of type NMDeviceVeth).
+		 *
+		 * The releases of 1.2.0 and 1.4.0 failed to realize above and broke this behavior.
+		 * This special handling here is to bring back the 1.0.0 behavior.
+		 *
+		 * The Device.Statistics signal is special, because it was only added with 1.4.0
+		 * and didn't have above behavior. So let's save the overhead of emitting multiple
+		 * deprecated signals for wrong interfaces. */
+		for (i = 0, j = 0; i < priv->num_interfaces; i++) {
+			ifdata = &priv->interfaces[i];
+			if (   ifdata->property_changed_signal_id
+			    && !NMDBUS_IS_DEVICE_STATISTICS_SKELETON (ifdata->interface)) {
+				if (j++ > 0)
+					g_variant_ref (value_variant);
+				g_hash_table_insert (ifdata->pending_notifies,
+				                     (gpointer) dbus_property_name,
+				                     value_variant);
+			}
+		}
+		nm_assert (j > 0);
+	} else if (ifdata->property_changed_signal_id) {
+		/* @dbus_property_name is inside classinfo and never freed, thus we don't clone it.
+		 * Also, we do a pointer, not string comparison. */
+		g_hash_table_insert (ifdata->pending_notifies,
+		                     (gpointer) dbus_property_name,
+		                     value_variant);
+	} else
+		nm_assert_not_reached ();
+
 	if (!priv->notify_idle_id)
 		priv->notify_idle_id = g_idle_add (idle_emit_properties_changed, object);
 }
diff --git a/src/nm-iface-helper.c b/src/nm-iface-helper.c
index 49672cf565..de035f1a94 100644
--- a/src/nm-iface-helper.c
+++ b/src/nm-iface-helper.c
@@ -534,6 +534,8 @@ gboolean nm_config_get_configure_and_quit (gpointer unused);
 gconstpointer nm_bus_manager_get (void);
 void nm_bus_manager_register_object (gpointer unused, gpointer object);
 void nm_bus_manager_unregister_object (gpointer unused, gpointer object);
+GType nm_device_get_type (void);
+GType nm_active_connection_get_type (void);
 
 gconstpointer
 nm_config_get (void)
@@ -569,3 +571,15 @@ nm_bus_manager_unregister_object (gpointer unused, gpointer object)
 {
 }
 
+GType
+nm_device_get_type (void)
+{
+	g_return_val_if_reached (0);
+}
+
+GType
+nm_active_connection_get_type (void)
+{
+	g_return_val_if_reached (0);
+}
+