libnm: queue added/removed signals and suppress uninitialized notifications

Property notifications are queued during object initialization and
reloading, but the added/removed signals were emitted immediately
even before the object was fully initialized.

Additionally, depending on how long asynchronous initialization took,
the notifications could have been emitted before the object was
fully initialized as deferred_notify_cb() wasn't being suppressed
until all the properties were complete.

For synchronous intialization, signals could be emitted at various
times during initialization and not all of the object's properties
may be read.  Furthermore property notifications were queued in an
idle handler, which breaks users that may not use a mainloop.  All
signals and notifications should be emitted immediately after
initialization is complete for synchronous initialization.

To make things consistent and ensure that all signals and notifications
are emitted only when initialization is complete, queue signals for
deferred emission and only run notifications/signals when all the
object's properties have been read.  For synchronous initialization,
emit all notifications and signals immediately after initialization
and not from an idle handler.

libnm/nm-object.c

@@ -69,7 +69,7 @@ typedef struct {
 	const char *signal_prefix;
 } PropertyInfo;
 
-static void reload_complete (NMObject *object);
+static void reload_complete (NMObject *object, gboolean emit_now);
 static gboolean demarshal_generic (NMObject *object, GParamSpec *pspec, GVariant *value, gpointer field);
 
 typedef struct {
@@ -83,7 +83,7 @@ typedef struct {
 	NMObject *parent;
 	gboolean suppress_property_updates;
 
-	GSList *notify_props;
+	GSList *notify_items;
 	guint32 notify_id;
 
 	GSList *reload_results;
@@ -164,57 +164,180 @@ _nm_object_get_proxy (NMObject   *object,
 	return proxy;
 }
 
+typedef enum {
+	NOTIFY_SIGNAL_PENDING_NONE,
+	NOTIFY_SIGNAL_PENDING_ADDED,
+	NOTIFY_SIGNAL_PENDING_REMOVED,
+	NOTIFY_SIGNAL_PENDING_ADDED_REMOVED,
+} NotifySignalPending;
+
+typedef struct {
+	const char *property;
+	const char *signal_prefix;
+	NotifySignalPending pending;
+	NMObject *changed;
+} NotifyItem;
+
+static void
+notify_item_free (NotifyItem *item)
+{
+	g_clear_object (&item->changed);
+	g_slice_free (NotifyItem, item);
+}
+
 static gboolean
 deferred_notify_cb (gpointer data)
 {
 	NMObject *object = NM_OBJECT (data);
 	NMObjectPrivate *priv = NM_OBJECT_GET_PRIVATE (object);
+	NMObjectClass *object_class = NM_OBJECT_GET_CLASS (object);
 	GSList *props, *iter;
 
 	priv->notify_id = 0;
 
-	/* Clear priv->notify_props early so that an NMObject subclass that
+	/* Wait until all reloads are done before notifying */
+	if (priv->reload_remaining)
+		return G_SOURCE_REMOVE;
+
+	/* Clear priv->notify_items early so that an NMObject subclass that
 	 * listens to property changes can queue up other property changes
 	 * during the g_object_notify() call separately from the property
 	 * list we're iterating.
 	 */
-	props = g_slist_reverse (priv->notify_props);
+	props = g_slist_reverse (priv->notify_items);
-	priv->notify_props = NULL;
+	priv->notify_items = NULL;
 
 	g_object_ref (object);
+
+	/* Emit property change notifications first */
 	for (iter = props; iter; iter = g_slist_next (iter)) {
-		g_object_notify (G_OBJECT (object), (const char *) iter->data);
+		NotifyItem *item = iter->data;
-		g_free (iter->data);
+
+		if (item->property)
+			g_object_notify (G_OBJECT (object), item->property);
+	}
+
+	/* And added/removed signals second */
+	for (iter = props; iter; iter = g_slist_next (iter)) {
+		NotifyItem *item = iter->data;
+		char buf[50];
+		gint ret = 0;
+
+		switch (item->pending) {
+		case NOTIFY_SIGNAL_PENDING_ADDED:
+			ret = g_snprintf (buf, sizeof (buf), "%s-added", item->signal_prefix);
+			break;
+		case NOTIFY_SIGNAL_PENDING_REMOVED:
+			ret = g_snprintf (buf, sizeof (buf), "%s-removed", item->signal_prefix);
+			break;
+		case NOTIFY_SIGNAL_PENDING_ADDED_REMOVED:
+			if (object_class->object_creation_failed)
+				object_class->object_creation_failed (object, nm_object_get_path (item->changed));
+			break;
+		case NOTIFY_SIGNAL_PENDING_NONE:
+		default:
+			break;
+		}
+		if (ret > 0) {
+			g_assert (ret < sizeof (buf));
+			g_signal_emit_by_name (object, buf, item->changed);
+		}
 	}
 	g_object_unref (object);
 
-	g_slist_free (props);
+	g_slist_free_full (props, (GDestroyNotify) notify_item_free);
-	return FALSE;
+	return G_SOURCE_REMOVE;
+}
+
+static void
+_nm_object_defer_notify (NMObject *object)
+{
+	NMObjectPrivate *priv = NM_OBJECT_GET_PRIVATE (object);
+
+	if (!priv->notify_id)
+		priv->notify_id = g_idle_add_full (G_PRIORITY_LOW, deferred_notify_cb, object, NULL);
+}
+
+static void
+_nm_object_queue_notify_full (NMObject *object,
+                              const char *property,
+                              const char *signal_prefix,
+                              gboolean added,
+                              NMObject *changed)
+{
+	NMObjectPrivate *priv;
+	NotifyItem *item;
+	GSList *iter;
+
+	g_return_if_fail (NM_IS_OBJECT (object));
+	g_return_if_fail (!signal_prefix != !property);
+	g_return_if_fail (!signal_prefix == !changed);
+
+	priv = NM_OBJECT_GET_PRIVATE (object);
+	_nm_object_defer_notify (object);
+
+	property = g_intern_string (property);
+	signal_prefix = g_intern_string (signal_prefix);
+	for (iter = priv->notify_items; iter; iter = g_slist_next (iter)) {
+		item = iter->data;
+
+		if (property && (property == item->property))
+			return;
+
+		/* Collapse signals for the same object (such as "added->removed") to
+		 * ensure we don't emit signals when their sum should have no effect.
+		 * The "added->removed->removed" sequence requires special handling,
+		 * hence the addition of the ADDED_REMOVED state to ensure that no
+		 * signal is emitted in this case:
+		 *
+		 * Without the ADDED_REMOVED state:
+		 *     NONE          + added   -> ADDED
+		 *     ADDED         + removed -> NONE
+		 *     NONE          + removed -> REMOVED (would emit 'removed' signal)
+		 *
+		 * With the ADDED_REMOVED state:
+		 *     NONE | ADDED_REMOVED  + added   -> ADDED
+		 *     ADDED                 + removed -> ADDED_REMOVED
+		 *     ADDED_REMOVED         + removed -> ADDED_REMOVED (emits no signal)
+		 */
+		if (signal_prefix && (changed == item->changed) && (item->signal_prefix == signal_prefix)) {
+			switch (item->pending) {
+			case NOTIFY_SIGNAL_PENDING_ADDED:
+				if (!added)
+					item->pending = NOTIFY_SIGNAL_PENDING_ADDED_REMOVED;
+				break;
+			case NOTIFY_SIGNAL_PENDING_REMOVED:
+				if (added)
+					item->pending = NOTIFY_SIGNAL_PENDING_NONE;
+				break;
+			case NOTIFY_SIGNAL_PENDING_ADDED_REMOVED:
+				if (added)
+					item->pending = NOTIFY_SIGNAL_PENDING_ADDED;
+				break;
+			case NOTIFY_SIGNAL_PENDING_NONE:
+				item->pending = added ? NOTIFY_SIGNAL_PENDING_ADDED : NOTIFY_SIGNAL_PENDING_REMOVED;
+				break;
+			default:
+				g_assert_not_reached ();
+			}
+			return;
+		}
+	}
+
+	item = g_slice_new0 (NotifyItem);
+	item->property = property;
+	if (signal_prefix) {
+		item->signal_prefix = signal_prefix;
+		item->pending = added ? NOTIFY_SIGNAL_PENDING_ADDED : NOTIFY_SIGNAL_PENDING_REMOVED;
+		item->changed = changed ? g_object_ref (changed) : NULL;
+	}
+	priv->notify_items = g_slist_prepend (priv->notify_items, item);
 }
 
 void
 _nm_object_queue_notify (NMObject *object, const char *property)
 {
-	NMObjectPrivate *priv;
+	_nm_object_queue_notify_full (object, property, NULL, FALSE, NULL);
-	gboolean found = FALSE;
-	GSList *iter;
-
-	g_return_if_fail (NM_IS_OBJECT (object));
-	g_return_if_fail (property != NULL);
-
-	priv = NM_OBJECT_GET_PRIVATE (object);
-	if (!priv->notify_id)
-		priv->notify_id = g_idle_add_full (G_PRIORITY_LOW, deferred_notify_cb, object, NULL);
-
-	for (iter = priv->notify_props; iter; iter = g_slist_next (iter)) {
-		if (!strcmp ((char *) iter->data, property)) {
-			found = TRUE;
-			break;
-		}
-	}
-
-	if (!found)
-		priv->notify_props = g_slist_prepend (priv->notify_props, g_strdup (property));
 }
 
 void
@@ -526,17 +649,12 @@ array_diff (GPtrArray *needles, GPtrArray *haystack, GPtrArray *diff)
 }
 
 static void
-emit_added_removed_signal (NMObject *self,
+queue_added_removed_signal (NMObject *self,
-                           const char *signal_prefix,
+                            const char *signal_prefix,
-                           NMObject *changed,
+                            NMObject *changed,
-                           gboolean added)
+                            gboolean added)
 {
-	char buf[50];
+	_nm_object_queue_notify_full (self, NULL, signal_prefix, added, changed);
-	int ret;
-
-	ret = g_snprintf (buf, sizeof (buf), "%s-%s", signal_prefix, added ? "added" : "removed");
-	g_assert (ret < sizeof (buf));
-	g_signal_emit_by_name (self, buf, changed);
 }
 
 static void
@@ -576,17 +694,17 @@ object_property_complete (ObjectCreatedData *odata)
 
 			/* Emit added & removed */
 			for (i = 0; i < removed->len; i++) {
-				emit_added_removed_signal (self,
+				queue_added_removed_signal (self,
-				                           pi->signal_prefix,
+				                            pi->signal_prefix,
-				                           g_ptr_array_index (removed, i),
+				                            g_ptr_array_index (removed, i),
-				                           FALSE);
+				                            FALSE);
 			}
 
 			for (i = 0; i < added->len; i++) {
-				emit_added_removed_signal (self,
+				queue_added_removed_signal (self,
-				                           pi->signal_prefix,
+				                            pi->signal_prefix,
-				                           g_ptr_array_index (added, i),
+				                            g_ptr_array_index (added, i),
-				                           TRUE);
+				                            TRUE);
 			}
 
 			different = removed->len || added->len;
@@ -617,8 +735,8 @@ object_property_complete (ObjectCreatedData *odata)
 	if (different && odata->property_name)
 		_nm_object_queue_notify (self, odata->property_name);
 
-	if (priv->reload_results && --priv->reload_remaining == 0)
+	if (--priv->reload_remaining == 0)
-		reload_complete (self);
+		reload_complete (self, FALSE);
 
 	g_object_unref (self);
 	g_free (odata->objects);
@@ -661,8 +779,7 @@ handle_object_property (NMObject *self, const char *property_name, GVariant *val
 	odata->array = FALSE;
 	odata->property_name = property_name;
 
-	if (priv->reload_results)
+	priv->reload_remaining++;
-		priv->reload_remaining++;
 
 	path = g_variant_get_string (value, NULL);
 
@@ -709,8 +826,7 @@ handle_object_array_property (NMObject *self, const char *property_name, GVarian
 	odata->array = TRUE;
 	odata->property_name = property_name;
 
-	if (priv->reload_results)
+	priv->reload_remaining++;
-		priv->reload_remaining++;
 
 	if (npaths == 0) {
 		object_property_complete (odata);
@@ -1029,6 +1145,8 @@ _nm_object_reload_properties (NMObject *object, GError **error)
 	if (!g_hash_table_size (priv->proxies) || !priv->nm_running)
 		return TRUE;
 
+	priv->reload_remaining++;
+
 	g_hash_table_iter_init (&iter, priv->proxies);
 	while (g_hash_table_iter_next (&iter, (gpointer *) &interface, (gpointer *) &proxy)) {
 		ret = g_dbus_proxy_call_sync (priv->properties_proxy,
@@ -1048,6 +1166,9 @@ _nm_object_reload_properties (NMObject *object, GError **error)
 		g_variant_unref (ret);
 	}
 
+	if (--priv->reload_remaining == 0)
+		reload_complete (object, TRUE);
+
 	return TRUE;
 }
 
@@ -1131,13 +1252,22 @@ _nm_object_set_property (NMObject *object,
 }
 
 static void
-reload_complete (NMObject *object)
+reload_complete (NMObject *object, gboolean emit_now)
 {
 	NMObjectPrivate *priv = NM_OBJECT_GET_PRIVATE (object);
 	GSimpleAsyncResult *simple;
 	GSList *results, *iter;
 	GError *error;
 
+	if (emit_now) {
+		if (priv->notify_id) {
+			g_source_remove (priv->notify_id);
+			priv->notify_id = 0;
+		}
+		deferred_notify_cb (object);
+	} else
+		_nm_object_defer_notify (object);
+
 	results = priv->reload_results;
 	priv->reload_results = NULL;
 	error = priv->reload_error;
@@ -1183,7 +1313,7 @@ reload_got_properties (GObject *proxy,
 	}
 
 	if (--priv->reload_remaining == 0)
-		reload_complete (object);
+		reload_complete (object, FALSE);
 }
 
 void
@@ -1546,8 +1676,8 @@ dispose (GObject *object)
 		priv->notify_id = 0;
 	}
 
-	g_slist_free_full (priv->notify_props, g_free);
+	g_slist_free_full (priv->notify_items, (GDestroyNotify) notify_item_free);
-	priv->notify_props = NULL;
+	priv->notify_items = NULL;
 
 	g_clear_pointer (&priv->proxies, g_hash_table_unref);
 	g_clear_object (&priv->properties_proxy);

libnm/tests/test-nm-client.c

@@ -111,6 +111,104 @@ test_device_added (void)
 
 /*******************************************************************/
 
+typedef enum {
+	SIGNAL_FIRST  = 0x01,
+	SIGNAL_SECOND = 0x02,
+	SIGNAL_MASK   = 0x0F,
+	NOTIFY_FIRST  = 0x10,
+	NOTIFY_SECOND = 0x20,
+	NOTIFY_MASK   = 0xF0
+} DeviceSignaledAfterInitType;
+
+static void
+device_sai_added_cb (NMClient *c,
+                     NMDevice *device,
+                     gpointer user_data)
+{
+	guint *result = user_data;
+
+	g_assert (device);
+	g_assert_cmpstr (nm_device_get_iface (device), ==, "eth0");
+
+	g_assert ((*result & SIGNAL_MASK) == 0);
+	*result |= *result ? SIGNAL_SECOND : SIGNAL_FIRST;
+}
+
+static void
+devices_sai_notify_cb (NMClient *c,
+                       GParamSpec *pspec,
+                       gpointer user_data)
+{
+	guint *result = user_data;
+	const GPtrArray *devices;
+	NMDevice *device;
+
+	devices = nm_client_get_devices (c);
+	g_assert (devices);
+	g_assert_cmpint (devices->len, ==, 1);
+
+	device = g_ptr_array_index (devices, 0);
+	g_assert (device);
+	g_assert_cmpstr (nm_device_get_iface (device), ==, "eth0");
+
+	g_assert ((*result & NOTIFY_MASK) == 0);
+	*result |= *result ? NOTIFY_SECOND : NOTIFY_FIRST;
+}
+
+static void
+test_device_added_signal_after_init (void)
+{
+	NMClient *client;
+	const GPtrArray *devices;
+	NMDevice *device;
+	guint result = 0;
+	GError *error = NULL;
+
+	sinfo = nm_test_service_init ();
+	client = nm_client_new (NULL, &error);
+	g_assert_no_error (error);
+
+	devices = nm_client_get_devices (client);
+	g_assert (devices->len == 0);
+
+	g_signal_connect (client,
+	                  NM_CLIENT_DEVICE_ADDED,
+	                  (GCallback) device_sai_added_cb,
+	                  &result);
+
+	g_signal_connect (client,
+	                  "notify::" NM_CLIENT_DEVICES,
+	                  (GCallback) devices_sai_notify_cb,
+	                  &result);
+
+	/* Tell the test service to add a new device */
+	nm_test_service_add_device (sinfo, client, "AddWiredDevice", "eth0");
+
+	/* Ensure the 'device-added' signal doesn't show up before
+	 * the 'Devices' property change notification */
+	while (!(result & SIGNAL_MASK) && !(result & NOTIFY_MASK))
+		g_main_context_iteration (NULL, TRUE);
+
+	g_signal_handlers_disconnect_by_func (client, device_sai_added_cb, &result);
+	g_signal_handlers_disconnect_by_func (client, devices_sai_notify_cb, &result);
+
+	g_assert ((result & NOTIFY_MASK) == NOTIFY_FIRST);
+	g_assert ((result & SIGNAL_MASK) == SIGNAL_SECOND);
+
+	devices = nm_client_get_devices (client);
+	g_assert (devices);
+	g_assert_cmpint (devices->len, ==, 1);
+
+	device = g_ptr_array_index (devices, 0);
+	g_assert (device);
+	g_assert_cmpstr (nm_device_get_iface (device), ==, "eth0");
+
+	g_object_unref (client);
+	g_clear_pointer (&sinfo, nm_test_service_cleanup);
+}
+
+/*******************************************************************/
+
 static const char *expected_bssid = "66:55:44:33:22:11";
 
 typedef struct {
@@ -1077,6 +1175,7 @@ main (int argc, char **argv)
 	loop = g_main_loop_new (NULL, FALSE);
 
 	g_test_add_func ("/libnm/device-added", test_device_added);
+	g_test_add_func ("/libnm/device-added-signal-after-init", test_device_added_signal_after_init);
 	g_test_add_func ("/libnm/wifi-ap-added-removed", test_wifi_ap_added_removed);
 	g_test_add_func ("/libnm/wimax-nsp-added-removed", test_wimax_nsp_added_removed);
 	g_test_add_func ("/libnm/devices-array", test_devices_array);