From 30e6c71fad2e9383c7430ddf3f9af49c2585baef Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 15 Sep 2015 11:30:08 +0200 Subject: [PATCH 1/5] core: forward declare NMExportedObject in "nm-types.h" --- src/nm-exported-object.h | 5 ++--- src/nm-types.h | 1 + 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/nm-exported-object.h b/src/nm-exported-object.h index 64e2a5b7a3..b4f7f38660 100644 --- a/src/nm-exported-object.h +++ b/src/nm-exported-object.h @@ -22,7 +22,6 @@ #define NM_EXPORTED_OBJECT_H #include "nm-default.h" -#include "nm-types.h" G_BEGIN_DECLS @@ -33,9 +32,9 @@ G_BEGIN_DECLS #define NM_IS_EXPORTED_OBJECT_CLASS(klass) (G_TYPE_CHECK_CLASS_TYPE ((klass), NM_TYPE_EXPORTED_OBJECT)) #define NM_EXPORTED_OBJECT_GET_CLASS(obj) (G_TYPE_INSTANCE_GET_CLASS ((obj), NM_TYPE_EXPORTED_OBJECT, NMExportedObjectClass)) -typedef struct { +struct _NMExportedObject { GObject parent; -} NMExportedObject; +}; typedef struct { GObjectClass parent; diff --git a/src/nm-types.h b/src/nm-types.h index b8ed972ee9..8fd4c36aef 100644 --- a/src/nm-types.h +++ b/src/nm-types.h @@ -26,6 +26,7 @@ #endif /* core */ +typedef struct _NMExportedObject NMExportedObject; typedef struct _NMActiveConnection NMActiveConnection; typedef struct _NMAuditManager NMAuditManager; typedef struct _NMVpnConnection NMVpnConnection; From a55c87a2c0f8fc63753d10d24a530e5adc237e35 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 15 Sep 2015 11:32:43 +0200 Subject: [PATCH 2/5] core: refactor NMBusManager to hold reference to NMExportedObject directly Previously, nm_bus_manager_register_object() would take various D-Bus skeleton objects that were associated with one NMExportedObject. This was confusing, in that these skeleton objects are all for the same NMObject but correspond to different D-Bus interfaces. Also, setting the D-Bus property "Managed" of a Device is broken because we might retrieve the wrong skeleton. Now, the NMBusManager has a reference to the exported object directly. The skeleton interface instances instead are now exposed by the NMExportedObject. --- src/nm-bus-manager.c | 201 ++++++++++++++++++++++++++++----------- src/nm-bus-manager.h | 9 +- src/nm-exported-object.c | 43 +++++++-- src/nm-exported-object.h | 1 + src/nm-manager.c | 4 +- 5 files changed, 186 insertions(+), 72 deletions(-) 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", From a33fc00239edde8a1e8ab91314e86e34a6daea2f Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 15 Sep 2015 11:34:00 +0200 Subject: [PATCH 3/5] core: refactor setting of D-Bus properties via NMManager - Also if the target object is the NMManager instance itself, re-fetch the manager via nm_bus_manager_get_registered_object(). This way, we only set the property on the manager, if it's also exported according to the bus-manager. Also, we don't treat the manager instance special. - Move fetching the object (nm_bus_manager_get_registered_object()) from do_set_property_check() to prop_set_auth_done_cb(). Otherwise, we fetch the object first, but there is no guarantee that the object is still exported after the asynchronous authentication succeeds. --- src/nm-exported-object.c | 13 +++++++ src/nm-exported-object.h | 1 + src/nm-manager.c | 83 ++++++++++++++++++++-------------------- 3 files changed, 56 insertions(+), 41 deletions(-) diff --git a/src/nm-exported-object.c b/src/nm-exported-object.c index da83096c46..d1e8782cdf 100644 --- a/src/nm-exported-object.c +++ b/src/nm-exported-object.c @@ -555,6 +555,19 @@ nm_exported_object_get_interfaces (NMExportedObject *self) return priv->interfaces; } +GDBusInterfaceSkeleton * +nm_exported_object_get_interface_by_type (NMExportedObject *self, GType interface_type) +{ + GSList *interfaces; + + interfaces = nm_exported_object_get_interfaces (self); + for (; interfaces; interfaces = interfaces->next) { + if (G_TYPE_CHECK_INSTANCE_TYPE (interfaces->data, interface_type)) + return interfaces->data; + } + return NULL; +} + static void nm_exported_object_init (NMExportedObject *self) { diff --git a/src/nm-exported-object.h b/src/nm-exported-object.h index ee870f154d..65f0eb6563 100644 --- a/src/nm-exported-object.h +++ b/src/nm-exported-object.h @@ -53,6 +53,7 @@ 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); +GDBusInterfaceSkeleton *nm_exported_object_get_interface_by_type (NMExportedObject *self, GType interface_type); G_END_DECLS diff --git a/src/nm-manager.c b/src/nm-manager.c index 7e5c39b031..e72d705438 100644 --- a/src/nm-manager.c +++ b/src/nm-manager.c @@ -56,6 +56,7 @@ #include "NetworkManagerUtils.h" #include "nmdbus-manager.h" +#include "nmdbus-device.h" static void add_device (NMManager *self, NMDevice *device, gboolean try_assume); @@ -4404,8 +4405,8 @@ typedef struct { const char *permission; const char *audit_op; char *audit_prop_value; - GObject *object; - const char *property; + GType interface_type; + const char *glib_propname; gboolean set_enable; } PropertyFilterData; @@ -4416,7 +4417,6 @@ free_property_filter_data (PropertyFilterData *pfd) g_object_unref (pfd->connection); g_object_unref (pfd->message); g_clear_object (&pfd->subject); - g_clear_object (&pfd->object); g_free (pfd->audit_prop_value); g_slice_free (PropertyFilterData, pfd); } @@ -4431,21 +4431,46 @@ prop_set_auth_done_cb (NMAuthChain *chain, NMManagerPrivate *priv = NM_MANAGER_GET_PRIVATE (pfd->self); NMAuthCallResult result; GDBusMessage *reply; + const char *error_message; + NMExportedObject *object; priv->auth_chains = g_slist_remove (priv->auth_chains, chain); result = nm_auth_chain_get_result (chain, pfd->permission); if (error || (result != NM_AUTH_CALL_RESULT_YES)) { reply = g_dbus_message_new_method_error (pfd->message, NM_PERM_DENIED_ERROR, - "Not authorized to perform this operation"); - nm_audit_log_control_op (pfd->audit_op, pfd->audit_prop_value, FALSE, pfd->subject, error ? error->message : NULL); - } else { - g_object_set (pfd->object, pfd->property, pfd->set_enable, NULL); - reply = g_dbus_message_new_method_reply (pfd->message); - g_dbus_message_set_body (reply, g_variant_new_tuple (NULL, 0)); - nm_audit_log_control_op (pfd->audit_op, pfd->audit_prop_value, TRUE, pfd->subject, NULL); + (error_message = "Not authorized to perform this operation")); + if (error) + error_message = error->message; + goto done; } + object = nm_bus_manager_get_registered_object (priv->dbus_mgr, + g_dbus_message_get_path (pfd->message)); + if (!object) { + reply = g_dbus_message_new_method_error (pfd->message, + "org.freedesktop.DBus.Error.UnknownObject", + (error_message = "Object doesn't exist.")); + goto done; + } + + /* do some extra type checking... */ + if (!nm_exported_object_get_interface_by_type (object, pfd->interface_type)) { + reply = g_dbus_message_new_method_error (pfd->message, + "org.freedesktop.DBus.Error.InvalidArgs", + (error_message = "Object is of unexpected type.")); + goto done; + } + + /* ... but set the property on the @object itself. It would be correct to set the property + * on the skeleton interface, but as it is now, the result is the same. */ + g_object_set (object, pfd->glib_propname, pfd->set_enable, NULL); + reply = g_dbus_message_new_method_reply (pfd->message); + g_dbus_message_set_body (reply, g_variant_new_tuple (NULL, 0)); + error_message = NULL; +done: + nm_audit_log_control_op (pfd->audit_op, pfd->audit_prop_value, !error_message, pfd->subject, error_message); + g_dbus_connection_send_message (pfd->connection, reply, G_DBUS_SEND_MESSAGE_FLAGS_NONE, NULL, NULL); @@ -4464,30 +4489,6 @@ do_set_property_check (gpointer user_data) NMAuthChain *chain; const char *error_message = NULL; - if (!pfd->object) { - NMExportedObject *object; - - object = nm_bus_manager_get_registered_object (nm_bus_manager_get (), - g_dbus_message_get_path (pfd->message)); - if (!object) { - reply = g_dbus_message_new_method_error (pfd->message, - "org.freedesktop.DBus.Error.UnknownObject", - (error_message = "Object doesn't exist.")); - goto out; - } - - /* If we lookup the object, we expect the object to be of a certain type. - * Only NMDevice type have settable properties. */ - if (!NM_IS_DEVICE (object)) { - reply = g_dbus_message_new_method_error (pfd->message, - "org.freedesktop.DBus.Error.InvalidArgs", - (error_message = "Object is of unexpected type.")); - goto out; - } - - pfd->object = g_object_ref (object); - } - pfd->subject = nm_auth_subject_new_unix_process_from_message (pfd->connection, pfd->message); if (!pfd->subject) { reply = g_dbus_message_new_method_error (pfd->message, @@ -4534,7 +4535,7 @@ prop_filter (GDBusConnection *connection, const char *glib_propname = NULL, *permission = NULL; const char *audit_op = NULL; gboolean set_enable; - GObject *object = NULL; + GType interface_type = G_TYPE_INVALID; PropertyFilterData *pfd; self = g_weak_ref_get (user_data); @@ -4580,8 +4581,7 @@ prop_filter (GDBusConnection *connection, audit_op = NM_AUDIT_OP_RADIO_CONTROL; } else return message; - - object = g_object_ref (self); + interface_type = NMDBUS_TYPE_MANAGER_SKELETON; } else if (!strcmp (propiface, NM_DBUS_INTERFACE_DEVICE)) { if (!strcmp (propname, "Autoconnect")) { glib_propname = NM_DEVICE_AUTOCONNECT; @@ -4589,6 +4589,7 @@ prop_filter (GDBusConnection *connection, audit_op = NM_AUDIT_OP_DEVICE_AUTOCONNECT; } else return message; + interface_type = NMDBUS_TYPE_DEVICE_SKELETON; } else return message; @@ -4602,11 +4603,11 @@ prop_filter (GDBusConnection *connection, pfd->connection = g_object_ref (connection); pfd->message = message; pfd->permission = permission; - pfd->object = object; - pfd->property = glib_propname; + pfd->interface_type = interface_type; + pfd->glib_propname = glib_propname; pfd->set_enable = set_enable; pfd->audit_op = audit_op; - pfd->audit_prop_value = g_strdup_printf ("%s:%d", pfd->property, pfd->set_enable); + pfd->audit_prop_value = g_strdup_printf ("%s:%d", pfd->glib_propname, pfd->set_enable); g_idle_add (do_set_property_check, pfd); return NULL; @@ -4996,7 +4997,7 @@ nm_manager_init (NMManager *manager) static void get_property (GObject *object, guint prop_id, - GValue *value, GParamSpec *pspec) + GValue *value, GParamSpec *pspec) { NMManager *self = NM_MANAGER (object); NMManagerPrivate *priv = NM_MANAGER_GET_PRIVATE (self); From fd8dde5c68425ceff199a4c7715eb2a64a48acb2 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 15 Sep 2015 14:06:15 +0200 Subject: [PATCH 4/5] device: add explicit NM_UNMANAGED_LOOPBACK flag for not managing "lo" --- src/devices/nm-device.c | 10 ++++------ src/devices/nm-device.h | 2 ++ 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index d4aa2ae06f..573270fcfb 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -1125,12 +1125,10 @@ nm_device_finish_init (NMDevice *self) if (priv->ifindex > 0) { if (priv->ifindex == 1) { - /* keep 'lo' as default-unmanaged. */ - - /* FIXME: either find a better way to unmange 'lo' that cannot be changed - * by user configuration (NM_UNMANGED_LOOPBACK?) or fix managing 'lo'. - * Currently it can happen that NM deletes 127.0.0.1 address. */ - nm_device_set_initial_unmanaged_flag (self, NM_UNMANAGED_DEFAULT, TRUE); + /* Unmanaged the loopback device with an explicit NM_UNMANAGED_LOOPBACK flag. + * Later we might want to manage 'lo' too. Currently that doesn't work because + * NetworkManager might down the interface or remove the 127.0.0.1 address. */ + nm_device_set_initial_unmanaged_flag (self, NM_UNMANAGED_LOOPBACK, TRUE); } else if (priv->platform_link_initialized || (priv->is_nm_owned && nm_device_is_software (self))) { gboolean platform_unmanaged = FALSE; diff --git a/src/devices/nm-device.h b/src/devices/nm-device.h index 29d521353a..6d254d7330 100644 --- a/src/devices/nm-device.h +++ b/src/devices/nm-device.h @@ -412,6 +412,7 @@ RfKillType nm_device_get_rfkill_type (NMDevice *device); * @NM_UNMANAGED_EXTERNAL_DOWN: %TRUE when unmanaged because !IFF_UP and not created by NM * @NM_UNMANAGED_PLATFORM_INIT: %TRUE when unmanaged because platform link not * yet initialized + * @NM_UNMANAGED_LOOPBACK: %TRUE for unmanaging loopback device */ typedef enum { NM_UNMANAGED_NONE = 0, @@ -421,6 +422,7 @@ typedef enum { NM_UNMANAGED_PARENT = (1LL << 3), NM_UNMANAGED_EXTERNAL_DOWN = (1LL << 4), NM_UNMANAGED_PLATFORM_INIT = (1LL << 5), + NM_UNMANAGED_LOOPBACK = (1LL << 6), /* Boundary value */ __NM_UNMANAGED_LAST, From 612552374044fe6b4b999ab7faf698ca409ec2e5 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 15 Sep 2015 23:53:46 +0200 Subject: [PATCH 5/5] device: refactor setting unmanaged based on device-spec --- src/devices/nm-device.c | 19 +++++++++++++++++++ src/devices/nm-device.h | 1 + src/nm-manager.c | 13 ++----------- 3 files changed, 22 insertions(+), 11 deletions(-) diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index 573270fcfb..22a235d40e 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -7759,6 +7759,25 @@ nm_device_set_unmanaged (NMDevice *self, } } +void +nm_device_set_unmanaged_by_device_spec (NMDevice *self, const GSList *unmanaged_specs) +{ + NMDevicePrivate *priv; + gboolean unmanaged; + + g_return_if_fail (NM_IS_DEVICE (self)); + + priv = NM_DEVICE_GET_PRIVATE (self); + + unmanaged = nm_device_spec_match_list (self, unmanaged_specs); + nm_device_set_unmanaged (self, + NM_UNMANAGED_USER, + unmanaged, + unmanaged + ? NM_DEVICE_STATE_REASON_NOW_UNMANAGED + : NM_DEVICE_STATE_REASON_NOW_MANAGED); +} + void nm_device_set_unmanaged_quitting (NMDevice *self) { diff --git a/src/devices/nm-device.h b/src/devices/nm-device.h index 6d254d7330..00ea568e52 100644 --- a/src/devices/nm-device.h +++ b/src/devices/nm-device.h @@ -436,6 +436,7 @@ void nm_device_set_unmanaged (NMDevice *device, NMUnmanagedFlags flag, gboolean unmanaged, NMDeviceStateReason reason); +void nm_device_set_unmanaged_by_device_spec (NMDevice *self, const GSList *unmanaged_specs); void nm_device_set_unmanaged_quitting (NMDevice *device); void nm_device_set_initial_unmanaged_flag (NMDevice *device, NMUnmanagedFlags flag, diff --git a/src/nm-manager.c b/src/nm-manager.c index e72d705438..0e13f3b1e3 100644 --- a/src/nm-manager.c +++ b/src/nm-manager.c @@ -1084,17 +1084,8 @@ system_unmanaged_devices_changed_cb (NMSettings *settings, const GSList *unmanaged_specs, *iter; unmanaged_specs = nm_settings_get_unmanaged_specs (priv->settings); - for (iter = priv->devices; iter; iter = g_slist_next (iter)) { - NMDevice *device = NM_DEVICE (iter->data); - gboolean unmanaged; - - unmanaged = nm_device_spec_match_list (device, unmanaged_specs); - nm_device_set_unmanaged (device, - NM_UNMANAGED_USER, - unmanaged, - unmanaged ? NM_DEVICE_STATE_REASON_NOW_UNMANAGED : - NM_DEVICE_STATE_REASON_NOW_MANAGED); - } + for (iter = priv->devices; iter; iter = g_slist_next (iter)) + nm_device_set_unmanaged_by_device_spec (NM_DEVICE (iter->data), unmanaged_specs); } static void