2 files changed, 279 insertions, 50 deletions

diff --git a/libnm/nm-object.c b/libnm/nm-object.c
index b6e697c25d..263d7785bb 100644
--- a/libnm/nm-object.c
+++ b/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);
-	priv->notify_props = NULL;
+	props = g_slist_reverse (priv->notify_items);
+	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);
-		g_free (iter->data);
+		NotifyItem *item = 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);
-	return FALSE;
+	g_slist_free_full (props, (GDestroyNotify) notify_item_free);
+	return G_SOURCE_REMOVE;
 }
 
-void
-_nm_object_queue_notify (NMObject *object, const char *property)
+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;
-	gboolean found = FALSE;
+	NotifyItem *item;
 	GSList *iter;
 
 	g_return_if_fail (NM_IS_OBJECT (object));
-	g_return_if_fail (property != NULL);
+	g_return_if_fail (!signal_prefix != !property);
+	g_return_if_fail (!signal_prefix == !changed);
 
 	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;
+	_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;
 		}
 	}
 
-	if (!found)
-		priv->notify_props = g_slist_prepend (priv->notify_props, g_strdup (property));
+	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)
+{
+	_nm_object_queue_notify_full (object, property, NULL, FALSE, NULL);
 }
 
 void
@@ -526,17 +649,12 @@ array_diff (GPtrArray *needles, GPtrArray *haystack, GPtrArray *diff)
 }
 
 static void
-emit_added_removed_signal (NMObject *self,
-                           const char *signal_prefix,
-                           NMObject *changed,
-                           gboolean added)
+queue_added_removed_signal (NMObject *self,
+                            const char *signal_prefix,
+                            NMObject *changed,
+                            gboolean added)
 {
-	char buf[50];
-	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);
+	_nm_object_queue_notify_full (self, NULL, signal_prefix, added, 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,
-				                           pi->signal_prefix,
-				                           g_ptr_array_index (removed, i),
-				                           FALSE);
+				queue_added_removed_signal (self,
+				                            pi->signal_prefix,
+				                            g_ptr_array_index (removed, i),
+				                            FALSE);
 			}
 
 			for (i = 0; i < added->len; i++) {
-				emit_added_removed_signal (self,
-				                           pi->signal_prefix,
-				                           g_ptr_array_index (added, i),
-				                           TRUE);
+				queue_added_removed_signal (self,
+				                            pi->signal_prefix,
+				                            g_ptr_array_index (added, i),
+				                            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)
-		reload_complete (self);
+	if (--priv->reload_remaining == 0)
+		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);
-	priv->notify_props = NULL;
+	g_slist_free_full (priv->notify_items, (GDestroyNotify) notify_item_free);
+	priv->notify_items = NULL;
 
 	g_clear_pointer (&priv->proxies, g_hash_table_unref);
 	g_clear_object (&priv->properties_proxy);
diff --git a/libnm/tests/test-nm-client.c b/libnm/tests/test-nm-client.c
index e78e85c9e5..867a27933b 100644
--- a/libnm/tests/test-nm-client.c
+++ b/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);