diff --git a/src/nm-bus-manager.c b/src/nm-bus-manager.c index 57d77aae48..4f6ad24376 100644 --- a/src/nm-bus-manager.c +++ b/src/nm-bus-manager.c @@ -25,14 +25,14 @@ #include #include #include +#include #include "nm-default.h" #include "nm-dbus-interface.h" #include "nm-bus-manager.h" #include "nm-core-internal.h" #include "nm-dbus-compat.h" - -#include +#include "nm-exported-object.h" #include "NetworkManagerUtils.h" #define PRIV_SOCK_PATH NMRUNDIR "/private" @@ -72,7 +72,6 @@ typedef struct { static gboolean nm_bus_manager_init_bus (NMBusManager *self); static void nm_bus_manager_cleanup (NMBusManager *self); static void start_reconnection_timeout (NMBusManager *self); -static void object_destroyed (NMBusManager *self, gpointer object); NM_DEFINE_SINGLETON_REGISTER (NMBusManager); @@ -104,6 +103,60 @@ nm_bus_manager_setup (NMBusManager *instance) /**************************************************************/ +static void +nm_assert_exported (NMBusManager *self, const char *path, NMExportedObject *object) +{ +#ifdef NM_MORE_ASSERTS + NMBusManagerPrivate *priv; + const char *p2, *po; + NMExportedObject *o2; + + /* NMBusManager and NMExportedObject are tied closely together. For example, while + * being registered, NMBusManager uses the path from nm_exported_object_get_path() + * as index. It relies on the path being stable. + * + * The alternative would be that NMBusManager copies the path upon registration + * to support diversion of NMExportedObject's path while being registered. But such + * a inconsistency would already indicate a bug, or at least a strange situation. + * + * So instead require some close cooperation between the two classes and add an + * assert here... */ + + nm_assert (NM_IS_BUS_MANAGER (self)); + nm_assert (!path || *path); + nm_assert (!object || NM_IS_EXPORTED_OBJECT (object)); + nm_assert (!!path || !!object); + + priv = NM_BUS_MANAGER_GET_PRIVATE (self); + nm_assert (priv->exported); + + if (!path) { + nm_assert (NM_IS_EXPORTED_OBJECT (object)); + + po = nm_exported_object_get_path (object); + nm_assert (po && *po); + + if (!g_hash_table_lookup_extended (priv->exported, po, (gpointer *) &p2, (gpointer *) &o2)) + nm_assert (FALSE); + + nm_assert (object == o2); + nm_assert (po == p2); + } else { + nm_assert (path && *path); + + if (!g_hash_table_lookup_extended (priv->exported, path, (gpointer *) &p2, (gpointer *) &o2)) + nm_assert (FALSE); + + nm_assert (NM_IS_EXPORTED_OBJECT (o2)); + nm_assert (!object || object == o2); + nm_assert (!g_strcmp0 (path, p2)); + nm_assert (p2 == nm_exported_object_get_path (o2)); + } +#endif +} + +/**************************************************************/ + struct _PrivateServer { const char *tag; GQuark detail; @@ -488,21 +541,29 @@ private_connection_new (NMBusManager *self, GDBusConnection *connection) { NMBusManagerPrivate *priv = NM_BUS_MANAGER_GET_PRIVATE (self); GHashTableIter iter; - GDBusInterfaceSkeleton *interface; + NMExportedObject *object; const char *path; GError *error = NULL; /* Register all exported objects on this private connection */ g_hash_table_iter_init (&iter, priv->exported); - while (g_hash_table_iter_next (&iter, (gpointer) &interface, (gpointer) &path)) { - if (g_dbus_interface_skeleton_export (interface, connection, path, &error)) { - nm_log_trace (LOGD_CORE, "(%s) registered %p (%s) at '%s' on private socket.", - PRIV_SOCK_TAG, interface, G_OBJECT_TYPE_NAME (interface), path); - } else { - nm_log_warn (LOGD_CORE, "(%s) could not register %p (%s) at '%s' on private socket: %s.", - PRIV_SOCK_TAG, interface, G_OBJECT_TYPE_NAME (interface), path, - error->message); - g_clear_error (&error); + while (g_hash_table_iter_next (&iter, (gpointer *) &path, (gpointer *) &object)) { + GSList *interfaces = nm_exported_object_get_interfaces (object); + + nm_assert_exported (self, path, object); + + for (; interfaces; interfaces = interfaces->next) { + GDBusInterfaceSkeleton *interface = G_DBUS_INTERFACE_SKELETON (interfaces->data); + + if (g_dbus_interface_skeleton_export (interface, connection, path, &error)) { + nm_log_trace (LOGD_CORE, "(%s) registered %p (%s) at '%s' on private socket.", + PRIV_SOCK_TAG, object, G_OBJECT_TYPE_NAME (interface), path); + } else { + nm_log_warn (LOGD_CORE, "(%s) could not register %p (%s) at '%s' on private socket: %s.", + PRIV_SOCK_TAG, object, G_OBJECT_TYPE_NAME (interface), path, + error->message); + g_clear_error (&error); + } } } } @@ -536,7 +597,7 @@ nm_bus_manager_init (NMBusManager *self) { NMBusManagerPrivate *priv = NM_BUS_MANAGER_GET_PRIVATE (self); - priv->exported = g_hash_table_new_full (g_direct_hash, g_direct_equal, NULL, g_free); + priv->exported = g_hash_table_new (g_str_hash, g_str_equal); private_server_setup (self); } @@ -546,13 +607,13 @@ nm_bus_manager_dispose (GObject *object) { NMBusManager *self = NM_BUS_MANAGER (object); NMBusManagerPrivate *priv = NM_BUS_MANAGER_GET_PRIVATE (self); - GHashTableIter iter; - GObject *exported; if (priv->exported) { - g_hash_table_iter_init (&iter, priv->exported); - while (g_hash_table_iter_next (&iter, (gpointer) &exported, NULL)) - g_object_weak_unref (exported, (GWeakNotify) object_destroyed, self); + /* We don't take references to the registered objects. + * We rely on the objects to properly unregister. + * Especially, they must unregister before destroying the + * NMBusManager instance. */ + g_assert (g_hash_table_size (priv->exported) == 0); g_hash_table_destroy (priv->exported); priv->exported = NULL; @@ -784,74 +845,104 @@ nm_bus_manager_get_connection (NMBusManager *self) return NM_BUS_MANAGER_GET_PRIVATE (self)->connection; } -static void -object_destroyed (NMBusManager *self, gpointer object) -{ - g_hash_table_remove (NM_BUS_MANAGER_GET_PRIVATE (self)->exported, object); -} - void nm_bus_manager_register_object (NMBusManager *self, - const char *path, - gpointer object) + NMExportedObject *object) { - NMBusManagerPrivate *priv = NM_BUS_MANAGER_GET_PRIVATE (self); - GHashTableIter iter; + NMBusManagerPrivate *priv; GDBusConnection *connection; + GHashTableIter iter; + const char *path; + GSList *interfaces, *ifs; - g_assert (G_IS_DBUS_INTERFACE_SKELETON (object)); + g_return_if_fail (NM_IS_BUS_MANAGER (self)); + g_return_if_fail (NM_IS_EXPORTED_OBJECT (object)); - if (g_hash_table_lookup (priv->exported, G_OBJECT (object))) + path = nm_exported_object_get_path (object); + g_return_if_fail (path && *path); + + priv = NM_BUS_MANAGER_GET_PRIVATE (self); + + /* We hold a direct reference to the @path of the @object. Note that + * this requires the object not to modify the path as long as the object + * is registered. Especially, it must not free the path. + * + * This is a reasonable requirement, because having the object change + * the path while being registered is an awkward situation in the first + * place. While being registered, the @path and @interfaces must stay + * stable -- because the path is the identifier for the object in this + * situation. */ + + if (!g_hash_table_insert (priv->exported, (gpointer) path, object)) g_return_if_reached (); - g_hash_table_insert (priv->exported, G_OBJECT (object), g_strdup (path)); - g_object_weak_ref (G_OBJECT (object), (GWeakNotify) object_destroyed, self); + nm_assert_exported (self, path, object); + + interfaces = nm_exported_object_get_interfaces (object); if (priv->connection) { - g_dbus_interface_skeleton_export (G_DBUS_INTERFACE_SKELETON (object), - priv->connection, path, NULL); + for (ifs = interfaces; ifs; ifs = ifs->next) { + g_dbus_interface_skeleton_export (G_DBUS_INTERFACE_SKELETON (ifs->data), + priv->connection, path, NULL); + } } if (priv->priv_server) { g_hash_table_iter_init (&iter, priv->priv_server->connections); while (g_hash_table_iter_next (&iter, (gpointer) &connection, NULL)) { - g_dbus_interface_skeleton_export (G_DBUS_INTERFACE_SKELETON (object), - connection, path, NULL); + for (ifs = interfaces; ifs; ifs = ifs->next) { + g_dbus_interface_skeleton_export (G_DBUS_INTERFACE_SKELETON (ifs->data), + connection, path, NULL); + } } } } -gpointer +NMExportedObject * nm_bus_manager_get_registered_object (NMBusManager *self, const char *path) { - NMBusManagerPrivate *priv = NM_BUS_MANAGER_GET_PRIVATE (self); - GHashTableIter iter; - GObject *object; - const char *export_path; + NMBusManagerPrivate *priv; + NMExportedObject *object; - g_hash_table_iter_init (&iter, priv->exported); - while (g_hash_table_iter_next (&iter, (gpointer *) &object, (gpointer *) &export_path)) { - if (!strcmp (path, export_path)) - return object; - } - return NULL; + g_return_val_if_fail (NM_IS_BUS_MANAGER (self), NULL); + g_return_val_if_fail (path && *path, NULL); + + priv = NM_BUS_MANAGER_GET_PRIVATE (self); + + object = g_hash_table_lookup (priv->exported, path); + + if (object) + nm_assert_exported (self, path, object); + + return object; } void -nm_bus_manager_unregister_object (NMBusManager *self, gpointer object) +nm_bus_manager_unregister_object (NMBusManager *self, NMExportedObject *object) { - NMBusManagerPrivate *priv = NM_BUS_MANAGER_GET_PRIVATE (self); + NMBusManagerPrivate *priv; + GSList *interfaces; + const char *path; - g_assert (G_IS_DBUS_INTERFACE_SKELETON (object)); + g_return_if_fail (NM_IS_BUS_MANAGER (self)); + g_return_if_fail (NM_IS_EXPORTED_OBJECT (object)); - if (!g_hash_table_lookup (priv->exported, G_OBJECT (object))) + path = nm_exported_object_get_path (object); + g_return_if_fail (path && *path); + + nm_assert_exported (self, NULL, object); + + priv = NM_BUS_MANAGER_GET_PRIVATE (self); + + if (!g_hash_table_remove (priv->exported, path)) g_return_if_reached (); - g_hash_table_remove (priv->exported, G_OBJECT (object)); - g_object_weak_unref (G_OBJECT (object), (GWeakNotify) object_destroyed, self); + for (interfaces = nm_exported_object_get_interfaces (object); interfaces; interfaces = interfaces->next) { + GDBusInterfaceSkeleton *interface = G_DBUS_INTERFACE_SKELETON (interfaces->data); - g_dbus_interface_skeleton_unexport (G_DBUS_INTERFACE_SKELETON (object)); + g_dbus_interface_skeleton_unexport (interface); + } } gboolean diff --git a/src/nm-bus-manager.h b/src/nm-bus-manager.h index 86aa5df6eb..8dd4be7ba2 100644 --- a/src/nm-bus-manager.h +++ b/src/nm-bus-manager.h @@ -87,13 +87,12 @@ gboolean nm_bus_manager_get_caller_info_from_message (NMBusManager *self, gulong *out_pid); void nm_bus_manager_register_object (NMBusManager *self, - const char *path, - gpointer object); + NMExportedObject *object); -void nm_bus_manager_unregister_object (NMBusManager *self, gpointer object); +void nm_bus_manager_unregister_object (NMBusManager *self, NMExportedObject *object); -gpointer nm_bus_manager_get_registered_object (NMBusManager *self, - const char *path); +NMExportedObject *nm_bus_manager_get_registered_object (NMBusManager *self, + const char *path); void nm_bus_manager_private_server_register (NMBusManager *self, const char *path, diff --git a/src/nm-exported-object.c b/src/nm-exported-object.c index 65f33bd290..da83096c46 100644 --- a/src/nm-exported-object.c +++ b/src/nm-exported-object.c @@ -36,6 +36,7 @@ G_DEFINE_ABSTRACT_TYPE_WITH_CODE (NMExportedObject, nm_exported_object, G_TYPE_O typedef struct { GSList *interfaces; + NMBusManager *bus_mgr; char *path; GVariantBuilder pending_notifies; @@ -427,15 +428,14 @@ const char * nm_exported_object_export (NMExportedObject *self) { NMExportedObjectPrivate *priv; - NMBusManager *dbus_manager = nm_bus_manager_get (); const char *class_export_path, *p; - GSList *iter; GType type; g_return_val_if_fail (NM_IS_EXPORTED_OBJECT (self), NULL); priv = NM_EXPORTED_OBJECT_GET_PRIVATE (self); - g_return_val_if_fail (priv->path == NULL, priv->path); + g_return_val_if_fail (!priv->path, priv->path); + g_return_val_if_fail (!priv->bus_mgr, NULL); class_export_path = NM_EXPORTED_OBJECT_GET_CLASS (self)->export_path; p = strchr (class_export_path, '%'); @@ -461,8 +461,12 @@ nm_exported_object_export (NMExportedObject *self) type = g_type_parent (type); } - for (iter = priv->interfaces; iter; iter = iter->next) - nm_bus_manager_register_object (dbus_manager, priv->path, iter->data); + priv->bus_mgr = g_object_ref (nm_bus_manager_get ()); + + /* Important: priv->path and priv->interfaces must not change while + * the object is registered. */ + + nm_bus_manager_register_object (priv->bus_mgr, self); return priv->path; } @@ -510,20 +514,24 @@ void nm_exported_object_unexport (NMExportedObject *self) { NMExportedObjectPrivate *priv; - GSList *iter; g_return_if_fail (NM_IS_EXPORTED_OBJECT (self)); priv = NM_EXPORTED_OBJECT_GET_PRIVATE (self); - g_return_if_fail (priv->path != NULL); + g_return_if_fail (priv->path); + g_return_if_fail (priv->bus_mgr); - g_clear_pointer (&priv->path, g_free); + /* Important: priv->path and priv->interfaces must not change while + * the object is registered. */ + + nm_bus_manager_unregister_object (priv->bus_mgr, self); - for (iter = priv->interfaces; iter; iter = iter->next) - nm_bus_manager_unregister_object (nm_bus_manager_get (), iter->data); g_slist_free_full (priv->interfaces, g_object_unref); priv->interfaces = NULL; + g_clear_pointer (&priv->path, g_free); + g_clear_object (&priv->bus_mgr); + 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. */ @@ -532,6 +540,21 @@ nm_exported_object_unexport (NMExportedObject *self) } } +GSList * +nm_exported_object_get_interfaces (NMExportedObject *self) +{ + NMExportedObjectPrivate *priv; + + g_return_val_if_fail (NM_IS_EXPORTED_OBJECT (self), NULL); + + priv = NM_EXPORTED_OBJECT_GET_PRIVATE (self); + + g_return_val_if_fail (priv->path, NULL); + g_return_val_if_fail (priv->interfaces, NULL); + + return priv->interfaces; +} + static void nm_exported_object_init (NMExportedObject *self) { diff --git a/src/nm-exported-object.h b/src/nm-exported-object.h index b4f7f38660..ee870f154d 100644 --- a/src/nm-exported-object.h +++ b/src/nm-exported-object.h @@ -52,6 +52,7 @@ const char *nm_exported_object_export (NMExportedObject *self); const char *nm_exported_object_get_path (NMExportedObject *self); gboolean nm_exported_object_is_exported (NMExportedObject *self); void nm_exported_object_unexport (NMExportedObject *self); +GSList * nm_exported_object_get_interfaces (NMExportedObject *self); G_END_DECLS diff --git a/src/nm-manager.c b/src/nm-manager.c index 2199d7ad4b..7e5c39b031 100644 --- a/src/nm-manager.c +++ b/src/nm-manager.c @@ -4465,10 +4465,10 @@ do_set_property_check (gpointer user_data) const char *error_message = NULL; if (!pfd->object) { - GObject *object; + NMExportedObject *object; object = nm_bus_manager_get_registered_object (nm_bus_manager_get (), - g_dbus_message_get_path (pfd->message)); + g_dbus_message_get_path (pfd->message)); if (!object) { reply = g_dbus_message_new_method_error (pfd->message, "org.freedesktop.DBus.Error.UnknownObject",