From 55afece5d291e692d3b54caa4e070f565c16e814 Mon Sep 17 00:00:00 2001

From: Thomas Haller <thaller@redhat.com>

Date: Wed, 31 Aug 2016 11:21:08 +0200

Subject: [PATCH 1/2] exported-object: fix source interface for

 PropertiesChanged D-Bus signal

nm_exported_object_notify() hooks GObject's property-change signal

and searches for the D-Bus interface to which to send the

PropertiesChanged signal.

Then it would enqueue the value encoded as GVariant in pending_notifications.

However, thereby the association between the property that changed and the

interface was lost. So later in idle_emit_properties_changed() it would

just pick the first interface with a properties-changed-id.

That is wrong. pending_notifications must be associated with the D-Bus

interface that we are going to notify. That is, each InterfaceData must

have its own separate list.

This is broken since introducing NMExportedObject and moving to gdbus.

Only now it was discovered as NMDevice itself has two D-Bus interfaces:

"Device" and "Device.Statistics".

Note that the order of the PropertiesChanged in our D-Bus API is not defined

so that later signals can reach the receiver before earlier signals.

Also, multiple change signals for one property may be combined.

That is not changed by this patch and is not considered a bug, but something

that our D-Bus API wrt. PropertiesChanged does not guarantee.

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

(cherry picked from commit 82e94390deb86eab25290e83ca0cd0e5a703fd73)

(cherry picked from commit 8d9ea18b3d4a3c76fea7a9f07cd02772cc906732)

---

 src/nm-exported-object.c | 104 ++++++++++++++++++++++++-----------------------

 1 file changed, 53 insertions(+), 51 deletions(-)

diff --git a/src/nm-exported-object.c b/src/nm-exported-object.c

index c4dbab8..31ca3f2 100644

--- a/src/nm-exported-object.c

+++ b/src/nm-exported-object.c

@@ -38,14 +38,13 @@ G_DEFINE_ABSTRACT_TYPE (NMExportedObject, nm_exported_object, G_TYPE_DBUS_OBJECT

 typedef struct {

 	GDBusInterfaceSkeleton *interface;

 	guint property_changed_signal_id;

+	GHashTable *pending_notifies;

 } InterfaceData;

 typedef struct {

 	NMBusManager *bus_mgr;

 	char *path;

-	GHashTable *pending_notifies;

-

 	InterfaceData *interfaces;

 	guint num_interfaces;

@@ -268,8 +267,8 @@ nm_exported_object_class_add_interface (NMExportedObjectClass *object_class,

 		                  nm_exported_object_class_info_quark (), classinfo);

 	}

-	classinfo->skeleton_types = g_slist_prepend (classinfo->skeleton_types,

-	                                             GSIZE_TO_POINTER (dbus_skeleton_type));

+	classinfo->skeleton_types = g_slist_append (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.

@@ -492,6 +491,11 @@ nm_exported_object_create_skeletons (NMExportedObject *self,

 		g_dbus_object_skeleton_add_interface ((GDBusObjectSkeleton *) self, ifdata->interface);

 		ifdata->property_changed_signal_id = g_signal_lookup ("properties-changed", G_OBJECT_TYPE (ifdata->interface));

+

+		ifdata->pending_notifies = g_hash_table_new_full (g_direct_hash,

+		                                                  g_direct_equal,

+		                                                  NULL,

+		                                                  (GDestroyNotify) g_variant_unref);

 	}

 	nm_assert (i == 0);

@@ -542,6 +546,7 @@ nm_exported_object_destroy_skeletons (NMExportedObject *self)

 		g_dbus_object_skeleton_remove_interface ((GDBusObjectSkeleton *) self, ifdata->interface);

 		nm_exported_object_skeleton_release (ifdata->interface);

+		g_hash_table_destroy (ifdata->pending_notifies);

 	}

 	g_slice_free1 (sizeof (InterfaceData) * n, priv->interfaces);

@@ -701,11 +706,7 @@ nm_exported_object_unexport (NMExportedObject *self)

 	g_clear_pointer (&priv->path, g_free);

-	if (nm_clear_g_source (&priv->notify_idle_id)) {

-		/* We had a notification queued. Since we removed all interfaces,

-		 * the notification is obsolete and must be cleaned up. */

-		g_hash_table_remove_all (priv->pending_notifies);

-	}

+	nm_clear_g_source (&priv->notify_idle_id);

 }

 /*****************************************************************************/

@@ -784,53 +785,60 @@ static gboolean

 idle_emit_properties_changed (gpointer self)

 {

 	NMExportedObjectPrivate *priv = NM_EXPORTED_OBJECT_GET_PRIVATE (self);

-	gs_unref_variant GVariant *variant = NULL;

-	InterfaceData *ifdata = NULL;

-	GHashTableIter hash_iter;

-	GVariantBuilder notifies;

-	guint i, n;

-	PendingNotifiesItem *values;

+	guint k;

 	priv->notify_idle_id = 0;

+	for (k = 0; k < priv->num_interfaces; k++) {

+		InterfaceData *ifdata = &priv->interfaces[k];

+		gs_unref_variant GVariant *variant = NULL;

+		PendingNotifiesItem *values;

+		GVariantBuilder notifies;

+		GHashTableIter hash_iter;

+		guint i, n;

+

+		n = g_hash_table_size (ifdata->pending_notifies);

+		if (n == 0)

+			continue;

-	n = g_hash_table_size (priv->pending_notifies);

-	g_return_val_if_fail (n > 0, FALSE);

+		if (!ifdata->property_changed_signal_id)

+			goto next;

-	values = g_alloca (sizeof (values[0]) * n);

+		/* 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

+		 * "n" is small (determined by the number of GObject properties). */

+		values = g_alloca (sizeof (values[0]) * n);

-	i = 0;

-	g_hash_table_iter_init (&hash_iter, priv->pending_notifies);

-	while (g_hash_table_iter_next (&hash_iter, (gpointer) &values[i].property_name, (gpointer) &values[i].variant))

-		i++;

-	nm_assert (i == n);

+		i = 0;

+		g_hash_table_iter_init (&hash_iter, ifdata->pending_notifies);

+		while (g_hash_table_iter_next (&hash_iter, (gpointer) &values[i].property_name, (gpointer) &values[i].variant))

+			i++;

+		nm_assert (i == n);

-	g_qsort_with_data (values, n, sizeof (values[0]), _sort_pending_notifies, NULL);

+		g_qsort_with_data (values, n, sizeof (values[0]), _sort_pending_notifies, NULL);

-	g_variant_builder_init (&notifies, G_VARIANT_TYPE_VARDICT);

-	for (i = 0; i < n; i++)

-		g_variant_builder_add (&notifies, "{sv}", values[i].property_name, values[i].variant);

-	variant = g_variant_ref_sink (g_variant_builder_end (&notifies));

+		g_variant_builder_init (&notifies, G_VARIANT_TYPE_VARDICT);

+		for (i = 0; i < n; i++)

+			g_variant_builder_add (&notifies, "{sv}", values[i].property_name, values[i].variant);

+		variant = g_variant_ref_sink (g_variant_builder_end (&notifies));

-	g_hash_table_remove_all (priv->pending_notifies);

-	for (i = 0; i < priv->num_interfaces; i++) {

-		if (priv->interfaces[i].property_changed_signal_id != 0) {

-			ifdata = &priv->interfaces[i];

-			break;

+		if (nm_logging_enabled (LOGL_DEBUG, LOGD_DBUS_PROPS)) {

+			gs_free char *notification = g_variant_print (variant, TRUE);

+

+			nm_log_dbg (LOGD_DBUS_PROPS, "PropertiesChanged %s, %s, %p: %s",

+			            G_OBJECT_TYPE_NAME (self), G_OBJECT_TYPE_NAME (ifdata->interface),

+			            self, notification);

 		}

-	}

-	g_return_val_if_fail (ifdata, FALSE);

-	if (nm_logging_enabled (LOGL_DEBUG, LOGD_DBUS_PROPS)) {

-		gs_free char *notification = g_variant_print (variant, TRUE);

+		g_signal_emit (ifdata->interface, ifdata->property_changed_signal_id, 0, variant);

-		nm_log_dbg (LOGD_DBUS_PROPS, "PropertiesChanged %s %p: %s",

-		            G_OBJECT_TYPE_NAME (self), self, notification);

+next:

+		g_hash_table_remove_all (ifdata->pending_notifies);

 	}

-	g_signal_emit (ifdata->interface, ifdata->property_changed_signal_id, 0, variant);

-	return FALSE;

+	return G_SOURCE_REMOVE;

 }

 static void

@@ -841,6 +849,7 @@ nm_exported_object_notify (GObject *object, GParamSpec *pspec)

 	GType type;

 	const char *dbus_property_name = NULL;

 	GValue value = G_VALUE_INIT;

+	InterfaceData *ifdata = NULL;

 	const GVariantType *vtype;

 	guint i, j;

@@ -863,10 +872,10 @@ nm_exported_object_notify (GObject *object, GParamSpec *pspec)

 	}

 	for (i = 0; i < priv->num_interfaces; i++) {

-		GDBusInterfaceSkeleton *skel = priv->interfaces[i].interface;

 		GDBusInterfaceInfo *iinfo;

-		iinfo = g_dbus_interface_skeleton_get_info (skel);

+		ifdata = &priv->interfaces[i];

+		iinfo = g_dbus_interface_skeleton_get_info (ifdata->interface);

 		for (j = 0; iinfo->properties[j]; j++) {

 			if (nm_streq (iinfo->properties[j]->name, dbus_property_name)) {

 				vtype = G_VARIANT_TYPE (iinfo->properties[j]->signature);

@@ -882,7 +891,7 @@ vtype_found:

 	/* @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 (priv->pending_notifies,

+	g_hash_table_insert (ifdata->pending_notifies,

 	                     (gpointer) dbus_property_name,

 	                     g_dbus_gvalue_to_gvariant (&value, vtype));

 	g_value_unset (&value);

@@ -896,12 +905,6 @@ vtype_found:

 static void

 nm_exported_object_init (NMExportedObject *self)

 {

-	NMExportedObjectPrivate *priv = NM_EXPORTED_OBJECT_GET_PRIVATE (self);

-

-	priv->pending_notifies = g_hash_table_new_full (g_direct_hash,

-	                                                g_direct_equal,

-	                                                NULL,

-	                                                (GDestroyNotify) g_variant_unref);

 }

 static void

@@ -937,7 +940,6 @@ nm_exported_object_dispose (GObject *object)

 	} else

 		g_clear_pointer (&priv->path, g_free);

-	g_clear_pointer (&priv->pending_notifies, g_hash_table_destroy);

 	nm_clear_g_source (&priv->notify_idle_id);

 	G_OBJECT_CLASS (nm_exported_object_parent_class)->dispose (object);

-- 

2.7.4

From a7c15ef9713a25c34c244f3684df564da09af093 Mon Sep 17 00:00:00 2001

From: Thomas Haller <thaller@redhat.com>

Date: Wed, 31 Aug 2016 11:33:01 +0200

Subject: [PATCH 2/2] exported-object: cleanup logging about properties-changed

(cherry picked from commit 46f285e3d38d64af647495d0d83efa2f9a8ea1aa)

(cherry picked from commit 4f452eedab24f9c9a0d282691ae2e600c6b6c310)

---

 src/nm-exported-object.c | 10 +++++-----

 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/src/nm-exported-object.c b/src/nm-exported-object.c

index 31ca3f2..d99a9ce 100644

--- a/src/nm-exported-object.c

+++ b/src/nm-exported-object.c

@@ -827,9 +827,9 @@ idle_emit_properties_changed (gpointer self)

 		if (nm_logging_enabled (LOGL_DEBUG, LOGD_DBUS_PROPS)) {

 			gs_free char *notification = g_variant_print (variant, TRUE);

-			nm_log_dbg (LOGD_DBUS_PROPS, "PropertiesChanged %s, %s, %p: %s",

-			            G_OBJECT_TYPE_NAME (self), G_OBJECT_TYPE_NAME (ifdata->interface),

-			            self, notification);

+			nm_log_dbg (LOGD_DBUS_PROPS, "properties-changed[%p]: type %s, iface %s: %s",

+			            self, G_OBJECT_TYPE_NAME (self), G_OBJECT_TYPE_NAME (ifdata->interface),

+			            notification);

 		}

 		g_signal_emit (ifdata->interface, ifdata->property_changed_signal_id, 0, variant);

@@ -866,8 +866,8 @@ nm_exported_object_notify (GObject *object, GParamSpec *pspec)

 			break;

 	}

 	if (!dbus_property_name) {

-		nm_log_trace (LOGD_DBUS_PROPS, "ignoring notification for prop %s on type %s",

-		              pspec->name, G_OBJECT_TYPE_NAME (object));

+		nm_log_trace (LOGD_DBUS_PROPS, "properties-changed[%p]: ignoring notification for prop %s on type %s",

+		              object, pspec->name, G_OBJECT_TYPE_NAME (object));

 		return;

 	}

-- 

2.7.4