From f2182fbf9b2423bd8509b2f0cf218edd96dac32c Mon Sep 17 00:00:00 2001 From: Dan Williams Date: Mon, 21 Mar 2016 16:09:30 -0500 Subject: [PATCH 1/3] core: don't emit double PropertiesChanged signal for new active connections When porting to GDBus property change notifications were converted from a hash table to a GVariantBuilder. GVariantBuilder doesn't care about duplicated properties in the dict so each g_object_notify() will add an additional item with possibly different values: signal time=1458571005.592811 sender=:1.10 -> destination=(null destination) serial=64451 path=/org/freedesktop/NetworkManager; interface=org.freedesktop.NetworkManager; member=PropertiesChanged array [ dict entry( string "ActiveConnections" variant array [ object path "/org/freedesktop/NetworkManager/ActiveConnection/19" object path "/org/freedesktop/NetworkManager/ActiveConnection/18" object path "/org/freedesktop/NetworkManager/ActiveConnection/15" object path "/org/freedesktop/NetworkManager/ActiveConnection/0" ] ) dict entry( string "ActiveConnections" variant array [ object path "/org/freedesktop/NetworkManager/ActiveConnection/24" object path "/org/freedesktop/NetworkManager/ActiveConnection/19" object path "/org/freedesktop/NetworkManager/ActiveConnection/18" object path "/org/freedesktop/NetworkManager/ActiveConnection/15" object path "/org/freedesktop/NetworkManager/ActiveConnection/0" ] ) ] Fix that by not emitting notify events for the manager's ActiveConnections property until the property has actually been updated in active_connection_add(). The unexport also isn't required for VPN connections since it will get unexported when it's disposed after _internal_activation_failed() gets called. --- src/nm-manager.c | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/src/nm-manager.c b/src/nm-manager.c index dc394a4904..8b978647b2 100644 --- a/src/nm-manager.c +++ b/src/nm-manager.c @@ -2716,19 +2716,12 @@ autoconnect_slaves (NMManager *self, static gboolean _internal_activate_vpn (NMManager *self, NMActiveConnection *active, GError **error) { - gboolean success; - g_assert (NM_IS_VPN_CONNECTION (active)); nm_exported_object_export (NM_EXPORTED_OBJECT (active)); - success = nm_vpn_manager_activate_connection (NM_MANAGER_GET_PRIVATE (self)->vpn_manager, - NM_VPN_CONNECTION (active), - error); - if (success) - g_object_notify (G_OBJECT (self), NM_MANAGER_ACTIVE_CONNECTIONS); - else - nm_exported_object_unexport (NM_EXPORTED_OBJECT (active)); - return success; + return nm_vpn_manager_activate_connection (NM_MANAGER_GET_PRIVATE (self)->vpn_manager, + NM_VPN_CONNECTION (active), + error); } /* Traverse the device to disconnected state. This means that the device is ready @@ -2951,7 +2944,6 @@ _internal_activate_device (NMManager *self, NMActiveConnection *active, GError * /* Export the new ActiveConnection to clients and start it on the device */ nm_exported_object_export (NM_EXPORTED_OBJECT (active)); - g_object_notify (G_OBJECT (self), NM_MANAGER_ACTIVE_CONNECTIONS); nm_device_queue_activation (device, NM_ACT_REQUEST (active)); return TRUE; } From 8b6a1ac62f2cb39b1246d7dff3525b1a8bb48f2c Mon Sep 17 00:00:00 2001 From: Dan Williams Date: Mon, 21 Mar 2016 16:24:12 -0500 Subject: [PATCH 2/3] core: don't leak ActiveConnection object on AddAndActivate failure --- src/nm-manager.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/nm-manager.c b/src/nm-manager.c index 8b978647b2..44a77916fa 100644 --- a/src/nm-manager.c +++ b/src/nm-manager.c @@ -3514,6 +3514,7 @@ activation_add_done (NMSettings *settings, FALSE, nm_active_connection_get_subject (active), error->message); + g_object_unref (active); g_clear_error (&local); } From 415e9441ab77a8a24d51e5119112beb0d47a5545 Mon Sep 17 00:00:00 2001 From: Dan Williams Date: Tue, 22 Mar 2016 13:00:56 -0500 Subject: [PATCH 3/3] core: fix duplicate values in NM-specific PropertiesChanged signals GVariantBuilder doesn't care if you give it multiple dict entries with the same key. But that causes duplicate dict entries in the legacy D-Bus PropertiesChanged signals that NM emits. So instead go back to a hash table to ensure that any previous value is dropped in favor of the new one. --- src/nm-exported-object.c | 43 +++++++++++++++++++++++----------------- 1 file changed, 25 insertions(+), 18 deletions(-) diff --git a/src/nm-exported-object.c b/src/nm-exported-object.c index b6d30aaba5..38a6271cb7 100644 --- a/src/nm-exported-object.c +++ b/src/nm-exported-object.c @@ -44,7 +44,7 @@ typedef struct { NMBusManager *bus_mgr; char *path; - GVariantBuilder pending_notifies; + GHashTable *pending_notifies; guint notify_idle_id; #ifdef _ASSERT_NO_EARLY_EXPORT @@ -659,8 +659,7 @@ nm_exported_object_unexport (NMExportedObject *self) 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_variant_builder_clear (&priv->pending_notifies); - g_variant_builder_init (&priv->pending_notifies, G_VARIANT_TYPE_VARDICT); + g_hash_table_remove_all (priv->pending_notifies); } } @@ -719,22 +718,33 @@ nm_exported_object_init (NMExportedObject *self) { NMExportedObjectPrivate *priv = NM_EXPORTED_OBJECT_GET_PRIVATE (self); - g_variant_builder_init (&priv->pending_notifies, G_VARIANT_TYPE_VARDICT); + priv->pending_notifies = g_hash_table_new_full (g_direct_hash, + g_direct_equal, + NULL, + (GDestroyNotify) g_variant_unref); } static gboolean idle_emit_properties_changed (gpointer self) { NMExportedObjectPrivate *priv = NM_EXPORTED_OBJECT_GET_PRIVATE (self); - GVariant *notifies; + GVariant *variant; GSList *iter; GDBusInterfaceSkeleton *interface = NULL; guint signal_id = 0; + GHashTableIter hash_iter; + const char *dbus_property_name; + GVariantBuilder notifies; priv->notify_idle_id = 0; - notifies = g_variant_builder_end (&priv->pending_notifies); - g_variant_ref_sink (notifies); - g_variant_builder_init (&priv->pending_notifies, G_VARIANT_TYPE_VARDICT); + + g_variant_builder_init (¬ifies, G_VARIANT_TYPE_VARDICT); + g_hash_table_iter_init (&hash_iter, priv->pending_notifies); + while (g_hash_table_iter_next (&hash_iter, (gpointer) &dbus_property_name, (gpointer) &variant)) + g_variant_builder_add (¬ifies, "{sv}", dbus_property_name, variant); + g_hash_table_remove_all (priv->pending_notifies); + variant = g_variant_builder_end (¬ifies); + g_variant_ref_sink (variant); for (iter = priv->interfaces; iter; iter = iter->next) { signal_id = g_signal_lookup ("properties-changed", G_OBJECT_TYPE (iter->data)); @@ -746,16 +756,14 @@ idle_emit_properties_changed (gpointer self) g_return_val_if_fail (signal_id != 0, FALSE); if (nm_logging_enabled (LOGL_DEBUG, LOGD_DBUS_PROPS)) { - char *notification; + gs_free char *notification = g_variant_print (variant, TRUE); - notification = g_variant_print (notifies, TRUE); nm_log_dbg (LOGD_DBUS_PROPS, "PropertiesChanged %s %p: %s", G_OBJECT_TYPE_NAME (self), self, notification); - g_free (notification); } - g_signal_emit (interface, signal_id, 0, notifies); - g_variant_unref (notifies); + g_signal_emit (interface, signal_id, 0, variant); + g_variant_unref (variant); return FALSE; } @@ -815,11 +823,10 @@ nm_exported_object_notify (GObject *object, GParamSpec *pspec) g_return_if_fail (vtype != NULL); variant = g_dbus_gvalue_to_gvariant (&value, vtype); - g_variant_builder_add (&priv->pending_notifies, "{sv}", - dbus_property_name, - variant); + /* @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, (gpointer) dbus_property_name, variant); g_value_unset (&value); - g_variant_unref (variant); if (!priv->notify_idle_id) priv->notify_idle_id = g_idle_add (idle_emit_properties_changed, object); @@ -858,7 +865,7 @@ nm_exported_object_dispose (GObject *object) } else g_clear_pointer (&priv->path, g_free); - g_variant_builder_clear (&priv->pending_notifies); + 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);