From b0f02075deb02d91361bdd53c31469b3b2b98618 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 29 Mar 2018 11:04:37 +0200 Subject: [PATCH 01/26] core: add nm_dbus_utils_g_value_set_object_path_from_hash() helper --- src/nm-dbus-utils.c | 34 ++++++++++++++++++++++++++++++++-- src/nm-dbus-utils.h | 4 ++++ 2 files changed, 36 insertions(+), 2 deletions(-) diff --git a/src/nm-dbus-utils.c b/src/nm-dbus-utils.c index eab6cb6125..f9c70e4953 100644 --- a/src/nm-dbus-utils.c +++ b/src/nm-dbus-utils.c @@ -147,6 +147,36 @@ nm_dbus_utils_g_value_set_object_path_array (GValue *value, g_value_take_boxed (value, paths); } +void +nm_dbus_utils_g_value_set_object_path_from_hash (GValue *value, + GHashTable *hash /* has keys of NMDBusObject type. */, + gboolean expect_all_exported) +{ + NMDBusObject *obj; + char **strv; + guint i, n; + GHashTableIter iter; + + nm_assert (value); + nm_assert (hash); + + n = g_hash_table_size (hash); + strv = g_new (char *, n + 1); + i = 0; + g_hash_table_iter_init (&iter, hash); + while (g_hash_table_iter_next (&iter, (gpointer *) &obj, NULL)) { + const char *path; + + path = nm_dbus_object_get_path (obj); + if (!path) { + nm_assert (!expect_all_exported); + continue; + } + strv[i++] = g_strdup (path); + } + nm_assert (i <= n); + strv[i] = NULL; + g_value_take_boxed (value, strv); +} + /*****************************************************************************/ - - diff --git a/src/nm-dbus-utils.h b/src/nm-dbus-utils.h index 4b0ec5c477..e556cdd1b0 100644 --- a/src/nm-dbus-utils.h +++ b/src/nm-dbus-utils.h @@ -171,4 +171,8 @@ void nm_dbus_utils_g_value_set_object_path_array (GValue *value, gboolean (*filter_func) (GObject *object, gpointer user_data), gpointer user_data); +void nm_dbus_utils_g_value_set_object_path_from_hash (GValue *value, + GHashTable *hash, + gboolean expect_all_exported); + #endif /* __NM_DBUS_UTILS_H__ */ From 03b1a1011f416999d8cb199d98fec2a12485c7ab Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 29 Mar 2018 08:18:59 +0200 Subject: [PATCH 02/26] checkpoint: rework getting NM_CHECKPOINT_DEVICES property --- src/nm-checkpoint.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/src/nm-checkpoint.c b/src/nm-checkpoint.c index d85d2d5499..631765c2af 100644 --- a/src/nm-checkpoint.c +++ b/src/nm-checkpoint.c @@ -434,16 +434,12 @@ get_property (GObject *object, guint prop_id, { NMCheckpoint *self = NM_CHECKPOINT (object); NMCheckpointPrivate *priv = NM_CHECKPOINT_GET_PRIVATE (self); - gs_free_slist GSList *devices = NULL; - GHashTableIter iter; - NMDevice *device; switch (prop_id) { case PROP_DEVICES: - g_hash_table_iter_init (&iter, priv->devices); - while (g_hash_table_iter_next (&iter, (gpointer *) &device, NULL)) - devices = g_slist_append (devices, device); - nm_dbus_utils_g_value_set_object_path_array (value, devices, NULL, NULL); + nm_dbus_utils_g_value_set_object_path_from_hash (value, + priv->devices, + FALSE); break; case PROP_CREATED: g_value_set_int64 (value, priv->created); From a399e2b8ffc10510023c0e4671f18d6554d899e0 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 29 Mar 2018 08:19:34 +0200 Subject: [PATCH 03/26] core: drop unused nm_dbus_utils_g_value_set_object_path_array() function --- src/nm-dbus-utils.c | 27 --------------------------- src/nm-dbus-utils.h | 5 ----- 2 files changed, 32 deletions(-) diff --git a/src/nm-dbus-utils.c b/src/nm-dbus-utils.c index f9c70e4953..2464ff79f7 100644 --- a/src/nm-dbus-utils.c +++ b/src/nm-dbus-utils.c @@ -120,33 +120,6 @@ nm_dbus_utils_g_value_set_object_path (GValue *value, gpointer object) g_value_set_string (value, "/"); } -void -nm_dbus_utils_g_value_set_object_path_array (GValue *value, - GSList *objects, - gboolean (*filter_func) (GObject *object, gpointer user_data), - gpointer user_data) -{ - char **paths; - guint i; - GSList *iter; - - paths = g_new (char *, g_slist_length (objects) + 1); - for (i = 0, iter = objects; iter; iter = iter->next) { - NMDBusObject *object = iter->data; - const char *path; - - path = nm_dbus_object_get_path (object); - if (!path) - continue; - if ( filter_func - && !filter_func ((GObject *) object, user_data)) - continue; - paths[i++] = g_strdup (path); - } - paths[i] = NULL; - g_value_take_boxed (value, paths); -} - void nm_dbus_utils_g_value_set_object_path_from_hash (GValue *value, GHashTable *hash /* has keys of NMDBusObject type. */, diff --git a/src/nm-dbus-utils.h b/src/nm-dbus-utils.h index e556cdd1b0..1d9c92ec36 100644 --- a/src/nm-dbus-utils.h +++ b/src/nm-dbus-utils.h @@ -166,11 +166,6 @@ GVariant *nm_dbus_utils_get_property (GObject *obj, void nm_dbus_utils_g_value_set_object_path (GValue *value, gpointer object); -void nm_dbus_utils_g_value_set_object_path_array (GValue *value, - GSList *objects, - gboolean (*filter_func) (GObject *object, gpointer user_data), - gpointer user_data); - void nm_dbus_utils_g_value_set_object_path_from_hash (GValue *value, GHashTable *hash, gboolean expect_all_exported); From 8de522fad0144e1ae2ba80dfb9d6b1da31b42866 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 29 Mar 2018 08:52:45 +0200 Subject: [PATCH 04/26] core: add macro for iterating CList of devices of NMManager I find it slightly nicer and explict. Also, the list elements are strictly speaking private, we should better not explicitly use them outside of NMManager/NMDevice. The macro hides this. --- src/devices/wifi/nm-device-olpc-mesh.c | 5 ++--- src/devices/wifi/nm-iwd-manager.c | 14 ++++++-------- src/nm-checkpoint-manager.c | 5 ++--- src/nm-checkpoint.c | 5 ++--- src/nm-manager.h | 15 ++++++++++++++- src/nm-policy.c | 25 ++++++++++--------------- 6 files changed, 36 insertions(+), 33 deletions(-) diff --git a/src/devices/wifi/nm-device-olpc-mesh.c b/src/devices/wifi/nm-device-olpc-mesh.c index d655406437..cd2c68af86 100644 --- a/src/devices/wifi/nm-device-olpc-mesh.c +++ b/src/devices/wifi/nm-device-olpc-mesh.c @@ -387,7 +387,7 @@ static void find_companion (NMDeviceOlpcMesh *self) { NMDeviceOlpcMeshPrivate *priv = NM_DEVICE_OLPC_MESH_GET_PRIVATE (self); - const CList *all_devices; + const CList *tmp_lst; NMDevice *candidate; if (priv->companion) @@ -396,8 +396,7 @@ find_companion (NMDeviceOlpcMesh *self) nm_device_add_pending_action (NM_DEVICE (self), NM_PENDING_ACTION_WAITING_FOR_COMPANION, TRUE); /* Try to find the companion if it's already known to the NMManager */ - all_devices = nm_manager_get_devices (priv->manager); - c_list_for_each_entry (candidate, all_devices, devices_lst) { + nm_manager_for_each_device (priv->manager, candidate, tmp_lst) { if (check_companion (self, candidate)) { nm_device_queue_recheck_available (NM_DEVICE (self), NM_DEVICE_STATE_REASON_NONE, diff --git a/src/devices/wifi/nm-iwd-manager.c b/src/devices/wifi/nm-iwd-manager.c index ea903bc724..1cf91bbc53 100644 --- a/src/devices/wifi/nm-iwd-manager.c +++ b/src/devices/wifi/nm-iwd-manager.c @@ -460,7 +460,7 @@ name_owner_changed (GObject *object, GParamSpec *pspec, gpointer user_data) g_clear_object (&priv->object_manager); prepare_object_manager (self); } else { - const CList *all_devices; + const CList *tmp_lst; NMDevice *device; if (!priv->running) @@ -468,13 +468,11 @@ name_owner_changed (GObject *object, GParamSpec *pspec, gpointer user_data) priv->running = false; - all_devices = nm_manager_get_devices (priv->nm_manager); - c_list_for_each_entry (device, all_devices, devices_lst) { - if (!NM_IS_DEVICE_IWD (device)) - continue; - - nm_device_iwd_set_dbus_object (NM_DEVICE_IWD (device), - NULL); + nm_manager_for_each_device (priv->nm_manager, device, tmp_lst) { + if (NM_IS_DEVICE_IWD (device)) { + nm_device_iwd_set_dbus_object (NM_DEVICE_IWD (device), + NULL); + } } } } diff --git a/src/nm-checkpoint-manager.c b/src/nm-checkpoint-manager.c index 7345f9a41d..2e4b2f1400 100644 --- a/src/nm-checkpoint-manager.c +++ b/src/nm-checkpoint-manager.c @@ -174,12 +174,11 @@ nm_checkpoint_manager_create (NMCheckpointManager *self, if (!device_paths || !device_paths[0]) { const char *device_path; - const CList *all_devices; + const CList *tmp_lst; GPtrArray *paths; paths = g_ptr_array_new (); - all_devices = nm_manager_get_devices (manager); - c_list_for_each_entry (device, all_devices, devices_lst) { + nm_manager_for_each_device (manager, device, tmp_lst) { if (!nm_device_is_real (device)) continue; device_path = nm_dbus_object_get_path (NM_DBUS_OBJECT (device)); diff --git a/src/nm-checkpoint.c b/src/nm-checkpoint.c index 631765c2af..c117ab9960 100644 --- a/src/nm-checkpoint.c +++ b/src/nm-checkpoint.c @@ -348,11 +348,10 @@ next_dev: } if (NM_FLAGS_HAS (priv->flags, NM_CHECKPOINT_CREATE_FLAG_DISCONNECT_NEW_DEVICES)) { - const CList *all_devices; + const CList *tmp_lst; NMDeviceState state; - all_devices = nm_manager_get_devices (priv->manager); - c_list_for_each_entry (device, all_devices, devices_lst) { + nm_manager_for_each_device (priv->manager, device, tmp_lst) { if (g_hash_table_contains (priv->devices, device)) continue; state = nm_device_get_state (device); diff --git a/src/nm-manager.h b/src/nm-manager.h index 638ceed6ea..3d651c7f7d 100644 --- a/src/nm-manager.h +++ b/src/nm-manager.h @@ -83,13 +83,14 @@ gboolean nm_manager_start (NMManager *manager, GError **error); void nm_manager_stop (NMManager *manager); NMState nm_manager_get_state (NMManager *manager); + const CList * nm_manager_get_active_connections (NMManager *manager); #define nm_manager_for_each_active_connection(manager, iter, tmp_list) \ for (tmp_list = nm_manager_get_active_connections (manager), \ iter = c_list_entry (tmp_list->next, NMActiveConnection, active_connections_lst); \ ({ \ - gboolean _has_next = (&iter->active_connections_lst != tmp_list); \ + const gboolean _has_next = (&iter->active_connections_lst != tmp_list); \ \ if (!_has_next) \ iter = NULL; \ @@ -107,6 +108,18 @@ void nm_manager_write_device_state (NMManager *manager); const CList * nm_manager_get_devices (NMManager *manager); +#define nm_manager_for_each_device(manager, iter, tmp_list) \ + for (tmp_list = nm_manager_get_devices (manager), \ + iter = c_list_entry (tmp_list->next, NMDevice, devices_lst); \ + ({ \ + const gboolean _has_next = (&iter->devices_lst != tmp_list); \ + \ + if (!_has_next) \ + iter = NULL; \ + _has_next; \ + }); \ + iter = c_list_entry (iter->devices_lst.next, NMDevice, devices_lst)) + NMDevice * nm_manager_get_device_by_ifindex (NMManager *manager, int ifindex); NMDevice * nm_manager_get_device_by_path (NMManager *manager, diff --git a/src/nm-policy.c b/src/nm-policy.c index 72351efe06..3984feb326 100644 --- a/src/nm-policy.c +++ b/src/nm-policy.c @@ -385,7 +385,7 @@ get_best_ip_device (NMPolicy *self, gboolean fully_activated) { NMPolicyPrivate *priv = NM_POLICY_GET_PRIVATE (self); - const CList *all_devices; + const CList *tmp_lst; NMDevice *device; NMDevice *best_device; NMDevice *prev_device; @@ -401,8 +401,7 @@ get_best_ip_device (NMPolicy *self, ? (fully_activated ? priv->default_device4 : priv->activating_device4) : (fully_activated ? priv->default_device6 : priv->activating_device6); - all_devices = nm_manager_get_devices (priv->manager); - c_list_for_each_entry (device, all_devices, devices_lst) { + nm_manager_for_each_device (priv->manager, device, tmp_lst) { NMDeviceState state; const NMPObject *r; NMConnection *connection; @@ -462,11 +461,10 @@ static gboolean all_devices_not_active (NMPolicy *self) { NMPolicyPrivate *priv = NM_POLICY_GET_PRIVATE (self); - const CList *all_devices; + const CList *tmp_lst; NMDevice *device; - all_devices = nm_manager_get_devices (priv->manager); - c_list_for_each_entry (device, all_devices, devices_lst) { + nm_manager_for_each_device (priv->manager, device, tmp_lst) { NMDeviceState state; state = nm_device_get_state (device); @@ -2186,13 +2184,12 @@ schedule_activate_all_cb (gpointer user_data) { NMPolicy *self = user_data; NMPolicyPrivate *priv = NM_POLICY_GET_PRIVATE (self); - const CList *all_devices; + const CList *tmp_lst; NMDevice *device; priv->schedule_activate_all_id = 0; - all_devices = nm_manager_get_devices (priv->manager); - c_list_for_each_entry (device, all_devices, devices_lst) + nm_manager_for_each_device (priv->manager, device, tmp_lst) schedule_activate_check (self, device); return G_SOURCE_REMOVE; @@ -2227,7 +2224,7 @@ firewall_state_changed (NMFirewallManager *manager, { NMPolicy *self = (NMPolicy *) user_data; NMPolicyPrivate *priv = NM_POLICY_GET_PRIVATE (self); - const CList *all_devices; + const CList *tmp_lst; NMDevice *device; if (initialized_now) { @@ -2241,8 +2238,7 @@ firewall_state_changed (NMFirewallManager *manager, return; /* add interface of each device to correct zone */ - all_devices = nm_manager_get_devices (priv->manager); - c_list_for_each_entry (device, all_devices, devices_lst) + nm_manager_for_each_device (priv->manager, device, tmp_lst) nm_device_update_firewall_zone (device); } @@ -2288,14 +2284,13 @@ connection_updated (NMSettings *settings, { NMPolicyPrivate *priv = user_data; NMPolicy *self = _PRIV_TO_SELF (priv); - const CList *all_devices; + const CList *tmp_lst; NMDevice *device = NULL; NMDevice *dev; if (by_user) { /* find device with given connection */ - all_devices = nm_manager_get_devices (priv->manager); - c_list_for_each_entry (dev, all_devices, devices_lst) { + nm_manager_for_each_device (priv->manager, dev, tmp_lst) { if (nm_device_get_settings_connection (dev) == connection) { device = dev; break; From 21b262f2680d8d1c433c1852a58d4906554453d9 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 29 Mar 2018 09:02:02 +0200 Subject: [PATCH 05/26] device/trival: rename NMIwdManagerPrivate.nm_manager field to "manager" Similar cases of such a field are named "manager". Also, internal names shall not have an "nm" prefix, contrary to names in a header file, which shall have such a prefix. --- src/devices/wifi/nm-iwd-manager.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/devices/wifi/nm-iwd-manager.c b/src/devices/wifi/nm-iwd-manager.c index 1cf91bbc53..450009f0c6 100644 --- a/src/devices/wifi/nm-iwd-manager.c +++ b/src/devices/wifi/nm-iwd-manager.c @@ -39,7 +39,7 @@ typedef struct { } KnownNetworkData; typedef struct { - NMManager *nm_manager; + NMManager *manager; GCancellable *cancellable; gboolean running; GDBusObjectManager *object_manager; @@ -138,7 +138,7 @@ psk_agent_dbus_method_cb (GDBusConnection *connection, goto return_error; } - device = nm_manager_get_device_by_ifindex (priv->nm_manager, ifindex); + device = nm_manager_get_device_by_ifindex (priv->manager, ifindex); if (!NM_IS_DEVICE_IWD (device)) { _LOGE ("IWD device named %s is not a Wifi device in IWD Agent request", ifname); @@ -291,7 +291,7 @@ set_device_dbus_object (NMIwdManager *self, GDBusInterface *interface, return; } - device = nm_manager_get_device_by_ifindex (priv->nm_manager, ifindex); + device = nm_manager_get_device_by_ifindex (priv->manager, ifindex); if (!NM_IS_DEVICE_IWD (device)) { _LOGE ("IWD device named %s is not a Wifi device", ifname); return; @@ -468,7 +468,7 @@ name_owner_changed (GObject *object, GParamSpec *pspec, gpointer user_data) priv->running = false; - nm_manager_for_each_device (priv->nm_manager, device, tmp_lst) { + nm_manager_for_each_device (priv->manager, device, tmp_lst) { if (NM_IS_DEVICE_IWD (device)) { nm_device_iwd_set_dbus_object (NM_DEVICE_IWD (device), NULL); @@ -637,8 +637,8 @@ nm_iwd_manager_init (NMIwdManager *self) { NMIwdManagerPrivate *priv = NM_IWD_MANAGER_GET_PRIVATE (self); - priv->nm_manager = g_object_ref (nm_manager_get ()); - g_signal_connect (priv->nm_manager, NM_MANAGER_DEVICE_ADDED, + priv->manager = g_object_ref (nm_manager_get ()); + g_signal_connect (priv->manager, NM_MANAGER_DEVICE_ADDED, G_CALLBACK (device_added), self); priv->cancellable = g_cancellable_new (); @@ -677,9 +677,9 @@ dispose (GObject *object) g_slist_free_full (priv->known_networks, (GDestroyNotify) known_network_free); priv->known_networks = NULL; - if (priv->nm_manager) { - g_signal_handlers_disconnect_by_data (priv->nm_manager, self); - g_clear_object (&priv->nm_manager); + if (priv->manager) { + g_signal_handlers_disconnect_by_data (priv->manager, self); + g_clear_object (&priv->manager); } G_OBJECT_CLASS (nm_iwd_manager_parent_class)->dispose (object); From 4c67e0d5ece8cb20ba4b479685c3d295a5f682f0 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 28 Mar 2018 16:59:39 +0200 Subject: [PATCH 06/26] libnm: minor cleanup of nm_manager_get_device_by_path() --- libnm/nm-manager.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/libnm/nm-manager.c b/libnm/nm-manager.c index 6ad5ad9d38..7c65f3906b 100644 --- a/libnm/nm-manager.c +++ b/libnm/nm-manager.c @@ -790,8 +790,7 @@ NMDevice * nm_manager_get_device_by_path (NMManager *manager, const char *object_path) { const GPtrArray *devices; - int i; - NMDevice *device = NULL; + guint i; g_return_val_if_fail (NM_IS_MANAGER (manager), NULL); g_return_val_if_fail (object_path, NULL); @@ -800,12 +799,11 @@ nm_manager_get_device_by_path (NMManager *manager, const char *object_path) for (i = 0; i < devices->len; i++) { NMDevice *candidate = g_ptr_array_index (devices, i); if (!strcmp (nm_object_get_path (NM_OBJECT (candidate)), object_path)) { - device = candidate; - break; + return candidate; } } - return device; + return NULL; } NMDevice * From 6f85d3e0b9bd80a74f4c20888103544f95be12a4 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 28 Mar 2018 12:31:43 +0200 Subject: [PATCH 07/26] libnm: fix crash creating checkpoint during find_checkpoint_info() Now that the D-Bus signals in server are reordered, creating a checkpoint in libnm crashes: $ examples/python/gi/checkpoint.py create 4 #0 0x00007ffff6d011ee in __strcmp_sse2_unaligned () at /lib64/libc.so.6 #1 0x00007fffeb611c90 in find_checkpoint_info (manager=manager@entry=0x5555559e5110 [NMManager], path=0x7fffdc0092f0 "/org/freedesktop/NetworkManager/Checkpoint/6") at libnm/nm-manager.c:153 #2 0x00007fffeb611d8f in checkpoint_added (manager=0x5555559e5110 [NMManager], checkpoint=checkpoint@entry=0x555555a122d0 [NMCheckpoint]) at libnm/nm-manager.c:1194 #3 0x00007fffef7db929 in g_cclosure_marshal_VOID__OBJECTv (closure=0x5555559e4b30, return_value=, instance=, args=, marshal_data=, n_params=, param_types=0x5555559e2fc0) at gmarshal.c:2102 #4 0x00007fffef7d8976 in _g_closure_invoke_va (closure=0x5555559e4b30, return_value=0x0, instance=0x5555559e5110, args=0x7fffffffc1c8, n_params=1, param_types=0x5555559e2fc0) at gclosure.c:867 #5 0x00007fffef7f3ff4 in g_signal_emit_valist (instance=instance@entry=0x5555559e5110, signal_id=signal_id@entry=97, detail=0, var_args=var_args@entry=0x7fffffffc1c8) at gsignal.c:3300 #6 0x00007fffef7f4b48 in g_signal_emit_by_name (instance=instance@entry=0x5555559e5110, detailed_signal=detailed_signal@entry=0x7fffffffc310 "checkpoint-added") at gsignal.c:3487 #7 0x00007fffeb6156d1 in deferred_notify_cb (data=0x5555559e5110) at libnm/nm-object.c:219 #8 0x00007fffeb615ae7 in object_property_maybe_complete (self=0x5555559e5110 [NMManager]) at libnm/nm-object.c:555 #9 0x00007fffeb615e5d in object_created (obj=, path=, user_data=) at libnm/nm-object.c:576 #10 0x00007fffeb61648b in handle_object_array_property (pi=, value=0x7fffdc075070, property_name=0x7fffeb67f117 "checkpoints", self=0x5555559e5110 [NMManager]) at libnm/nm-object.c:671 #11 0x00007fffeb61648b in handle_property_changed (self=self@entry=0x5555559e5110 [NMManager], dbus_name=, value=) at libnm/nm-object.c:740 #12 0x00007fffeb6166e9 in properties_changed (proxy=, changed_properties=, invalidated_properties=, user_data=0x5555559e5110) at libnm/nm-object.c:772 ... That is, because NetworkManager now first emits signals that the checkpoint object was created, before answering the D-Bus request. That makes more sense, but leads to this crash. The ugliness of how libnm handles object visibility is considerable. libnm hides objects until they are fully initialized. So, when the async create-checkpoint operation returns, the object might not yet be ready to be exposed. We need to delay the result. It would be better if the API would simply return the created path. --- libnm/nm-manager.c | 40 ++++++++++++++++++++++++++++++---------- 1 file changed, 30 insertions(+), 10 deletions(-) diff --git a/libnm/nm-manager.c b/libnm/nm-manager.c index 7c65f3906b..b8a0b9c046 100644 --- a/libnm/nm-manager.c +++ b/libnm/nm-manager.c @@ -150,7 +150,7 @@ find_checkpoint_info (NMManager *manager, const char *path) for (iter = priv->added_checkpoints; iter; iter = g_slist_next (iter)) { info = iter->data; - if (nm_streq (path, info->path)) + if (nm_streq0 (path, info->path)) return info; } @@ -802,7 +802,21 @@ nm_manager_get_device_by_path (NMManager *manager, const char *object_path) return candidate; } } + return NULL; +} +static NMCheckpoint * +get_checkpoint_by_path (NMManager *manager, const char *object_path) +{ + NMManagerPrivate *priv = NM_MANAGER_GET_PRIVATE (manager); + NMCheckpoint *candidate; + guint i; + + for (i = 0; i < priv->checkpoints->len; i++) { + candidate = priv->checkpoints->pdata[i]; + if (nm_streq (nm_object_get_path (NM_OBJECT (candidate)), object_path)) + return candidate; + } return NULL; } @@ -1194,11 +1208,6 @@ checkpoint_added (NMManager *manager, NMCheckpoint *checkpoint) checkpoint_info_complete (manager, info, checkpoint, NULL); } -static void -checkpoint_removed (NMManager *manager, NMCheckpoint *checkpoint) -{ -} - gboolean nm_manager_deactivate_connection (NMManager *manager, NMActiveConnection *active, @@ -1329,15 +1338,27 @@ checkpoint_created_cb (GObject *object, gpointer user_data) { CheckpointInfo *info = user_data; - GError *error = NULL; + NMManager *self = info->manager; + gs_free_error GError *error = NULL; + NMCheckpoint *checkpoint; nmdbus_manager_call_checkpoint_create_finish (NMDBUS_MANAGER (object), &info->path, result, &error); if (error) { g_dbus_error_strip_remote_error (error); - checkpoint_info_complete (info->manager, info, NULL, error); - g_clear_error (&error); + checkpoint_info_complete (self, info, NULL, error); + return; } + + checkpoint = get_checkpoint_by_path (self, info->path); + if (!checkpoint) { + /* this is really problematic. The async request returned, but + * we don't yet have a visible (fully initialized) NMCheckpoint instance + * to return. Wait longer for it to appear. However, it's ugly. */ + return; + } + + checkpoint_info_complete (self, info, checkpoint, NULL); } void @@ -1830,7 +1851,6 @@ nm_manager_class_init (NMManagerClass *manager_class) manager_class->active_connection_added = active_connection_added; manager_class->active_connection_removed = active_connection_removed; manager_class->checkpoint_added = checkpoint_added; - manager_class->checkpoint_removed = checkpoint_removed; /* properties */ From ffb492678ec5c22626f334fa7b3b769d284f074c Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 27 Mar 2018 12:45:23 +0200 Subject: [PATCH 08/26] checkpoint: embed CList in NMCheckpoint instance We don't need an external CheckpointItem, just to wrap the CList instance. Embed it directly in NMCheckpoint. Sure, that exposes the checkpoints_lst field in the (internal) header file, hiding the private member less. --- Makefile.am | 4 +-- src/nm-checkpoint-manager.c | 65 ++++++++++++++++--------------------- src/nm-checkpoint.c | 21 +++++++----- src/nm-checkpoint.h | 9 ++++- 4 files changed, 51 insertions(+), 48 deletions(-) diff --git a/Makefile.am b/Makefile.am index f52e1f0248..61b89bd9f8 100644 --- a/Makefile.am +++ b/Makefile.am @@ -1466,10 +1466,10 @@ src_libNetworkManager_la_CPPFLAGS = $(src_cppflags) src_libNetworkManager_la_SOURCES = \ \ - src/nm-checkpoint-manager.c \ - src/nm-checkpoint-manager.h \ src/nm-checkpoint.c \ src/nm-checkpoint.h \ + src/nm-checkpoint-manager.c \ + src/nm-checkpoint-manager.h \ \ src/devices/nm-device.c \ src/devices/nm-device.h \ diff --git a/src/nm-checkpoint-manager.c b/src/nm-checkpoint-manager.c index 2e4b2f1400..911577f46d 100644 --- a/src/nm-checkpoint-manager.c +++ b/src/nm-checkpoint-manager.c @@ -36,7 +36,7 @@ struct _NMCheckpointManager { NMManager *_manager; GParamSpec *property_spec; GHashTable *checkpoints; - CList list; + CList checkpoints_lst_head; guint rollback_timeout_id; }; @@ -58,11 +58,6 @@ struct _NMCheckpointManager { /*****************************************************************************/ -typedef struct { - CList list; - NMCheckpoint *checkpoint; -} CheckpointItem; - static void update_rollback_timeout (NMCheckpointManager *self); static void @@ -74,18 +69,17 @@ notify_checkpoints (NMCheckpointManager *self) { static void item_destroy (gpointer data) { - CheckpointItem *item = data; + NMCheckpoint *checkpoint = data; - c_list_unlink_stale (&item->list); - nm_dbus_object_unexport (NM_DBUS_OBJECT (item->checkpoint)); - g_object_unref (G_OBJECT (item->checkpoint)); - g_slice_free (CheckpointItem, item); + c_list_unlink (&checkpoint->checkpoints_lst); + nm_dbus_object_unexport (NM_DBUS_OBJECT (checkpoint)); + g_object_unref (checkpoint); } static gboolean rollback_timeout_cb (NMCheckpointManager *self) { - CheckpointItem *item, *safe; + NMCheckpoint *checkpoint, *checkpoint_safe; GVariant *result; gint64 ts, now; const char *path; @@ -93,13 +87,13 @@ rollback_timeout_cb (NMCheckpointManager *self) now = nm_utils_get_monotonic_timestamp_ms (); - c_list_for_each_entry_safe (item, safe, &self->list, list) { - ts = nm_checkpoint_get_rollback_ts (item->checkpoint); + c_list_for_each_entry_safe (checkpoint, checkpoint_safe, &self->checkpoints_lst_head, checkpoints_lst) { + ts = nm_checkpoint_get_rollback_ts (checkpoint); if (ts && ts <= now) { - result = nm_checkpoint_rollback (item->checkpoint); + result = nm_checkpoint_rollback (checkpoint); if (result) g_variant_unref (result); - path = nm_dbus_object_get_path (NM_DBUS_OBJECT (item->checkpoint)); + path = nm_dbus_object_get_path (NM_DBUS_OBJECT (checkpoint)); if (!g_hash_table_remove (self->checkpoints, path)) nm_assert_not_reached(); removed = TRUE; @@ -118,11 +112,11 @@ rollback_timeout_cb (NMCheckpointManager *self) static void update_rollback_timeout (NMCheckpointManager *self) { - CheckpointItem *item; + NMCheckpoint *checkpoint; gint64 ts, delta, next = G_MAXINT64; - c_list_for_each_entry (item, &self->list, list) { - ts = nm_checkpoint_get_rollback_ts (item->checkpoint); + c_list_for_each_entry (checkpoint, &self->checkpoints_lst_head, checkpoints_lst) { + ts = nm_checkpoint_get_rollback_ts (checkpoint); if (ts && ts < next) next = ts; } @@ -141,11 +135,11 @@ update_rollback_timeout (NMCheckpointManager *self) static NMCheckpoint * find_checkpoint_for_device (NMCheckpointManager *self, NMDevice *device) { - CheckpointItem *item; + NMCheckpoint *checkpoint; - c_list_for_each_entry (item, &self->list, list) { - if (nm_checkpoint_includes_device (item->checkpoint, device)) - return item->checkpoint; + c_list_for_each_entry (checkpoint, &self->checkpoints_lst_head, checkpoints_lst) { + if (nm_checkpoint_includes_device (checkpoint, device)) + return checkpoint; } return NULL; @@ -160,7 +154,6 @@ nm_checkpoint_manager_create (NMCheckpointManager *self, { NMManager *manager; NMCheckpoint *checkpoint; - CheckpointItem *item; const char * const *path; gs_unref_ptrarray GPtrArray *devices = NULL; NMDevice *device; @@ -228,11 +221,9 @@ nm_checkpoint_manager_create (NMCheckpointManager *self, checkpoint_path = nm_dbus_object_export (NM_DBUS_OBJECT (checkpoint)); - item = g_slice_new0 (CheckpointItem); - item->checkpoint = checkpoint; - c_list_link_tail (&self->list, &item->list); + c_list_link_tail (&self->checkpoints_lst_head, &checkpoint->checkpoints_lst); - if (!g_hash_table_insert (self->checkpoints, (gpointer) checkpoint_path, item)) + if (!g_hash_table_insert (self->checkpoints, (gpointer) checkpoint_path, checkpoint)) g_return_val_if_reached (NULL); notify_checkpoints (self); @@ -285,21 +276,21 @@ nm_checkpoint_manager_rollback (NMCheckpointManager *self, GVariant **results, GError **error) { - CheckpointItem *item; + NMCheckpoint *checkpoint; g_return_val_if_fail (self, FALSE); g_return_val_if_fail (checkpoint_path && checkpoint_path[0] == '/', FALSE); g_return_val_if_fail (results, FALSE); g_return_val_if_fail (!error || !*error, FALSE); - item = g_hash_table_lookup (self->checkpoints, checkpoint_path); - if (!item) { + checkpoint = g_hash_table_lookup (self->checkpoints, checkpoint_path); + if (!checkpoint) { g_set_error (error, NM_MANAGER_ERROR, NM_MANAGER_ERROR_FAILED, "checkpoint %s does not exist", checkpoint_path); return FALSE; } - *results = nm_checkpoint_rollback (item->checkpoint); + *results = nm_checkpoint_rollback (checkpoint); g_hash_table_remove (self->checkpoints, checkpoint_path); notify_checkpoints (self); @@ -309,19 +300,19 @@ nm_checkpoint_manager_rollback (NMCheckpointManager *self, char ** nm_checkpoint_manager_get_checkpoint_paths (NMCheckpointManager *self) { - CheckpointItem *item; + NMCheckpoint *checkpoint; char **strv; guint num, i = 0; num = g_hash_table_size (self->checkpoints); if (!num) { - nm_assert (c_list_is_empty (&self->list)); + nm_assert (c_list_is_empty (&self->checkpoints_lst_head)); return NULL; } strv = g_new (char *, num + 1); - c_list_for_each_entry (item, &self->list, list) - strv[i++] = g_strdup (nm_dbus_object_get_path (NM_DBUS_OBJECT (item->checkpoint))); + c_list_for_each_entry (checkpoint, &self->checkpoints_lst_head, checkpoints_lst) + strv[i++] = g_strdup (nm_dbus_object_get_path (NM_DBUS_OBJECT (checkpoint))); nm_assert (i == num); strv[i] = NULL; @@ -349,7 +340,7 @@ nm_checkpoint_manager_new (NMManager *manager, GParamSpec *spec) self->checkpoints = g_hash_table_new_full (nm_str_hash, g_str_equal, NULL, item_destroy); self->property_spec = spec; - c_list_init (&self->list); + c_list_init (&self->checkpoints_lst_head); return self; } diff --git a/src/nm-checkpoint.c b/src/nm-checkpoint.c index c117ab9960..ce36c7c9aa 100644 --- a/src/nm-checkpoint.c +++ b/src/nm-checkpoint.c @@ -54,7 +54,7 @@ NM_GOBJECT_PROPERTIES_DEFINE_BASE ( PROP_ROLLBACK_TIMEOUT, ); -typedef struct { +struct _NMCheckpointPrivate { /* properties */ GHashTable *devices; gint64 created; @@ -64,11 +64,6 @@ typedef struct { gint64 rollback_ts; NMCheckpointCreateFlags flags; GHashTable *connection_uuids; -} NMCheckpointPrivate; - -struct _NMCheckpoint { - NMDBusObject parent; - NMCheckpointPrivate _priv; }; struct _NMCheckpointClass { @@ -77,7 +72,7 @@ struct _NMCheckpointClass { G_DEFINE_TYPE (NMCheckpoint, nm_checkpoint, NM_TYPE_DBUS_OBJECT) -#define NM_CHECKPOINT_GET_PRIVATE(self) _NM_GET_PRIVATE (self, NMCheckpoint, NM_IS_CHECKPOINT) +#define NM_CHECKPOINT_GET_PRIVATE(self) _NM_GET_PRIVATE_PTR (self, NMCheckpoint, NM_IS_CHECKPOINT) /*****************************************************************************/ @@ -457,7 +452,13 @@ get_property (GObject *object, guint prop_id, static void nm_checkpoint_init (NMCheckpoint *self) { - NMCheckpointPrivate *priv = NM_CHECKPOINT_GET_PRIVATE (self); + NMCheckpointPrivate *priv; + + priv = G_TYPE_INSTANCE_GET_PRIVATE (self, NM_TYPE_CHECKPOINT, NMCheckpointPrivate); + + self->_priv = priv; + + c_list_init (&self->checkpoints_lst); priv->devices = g_hash_table_new_full (nm_direct_hash, NULL, NULL, device_checkpoint_destroy); @@ -525,6 +526,8 @@ dispose (GObject *object) NMCheckpoint *self = NM_CHECKPOINT (object); NMCheckpointPrivate *priv = NM_CHECKPOINT_GET_PRIVATE (self); + nm_assert (c_list_is_empty (&self->checkpoints_lst)); + g_clear_pointer (&priv->devices, g_hash_table_unref); g_clear_pointer (&priv->connection_uuids, g_hash_table_unref); @@ -552,6 +555,8 @@ nm_checkpoint_class_init (NMCheckpointClass *checkpoint_class) GObjectClass *object_class = G_OBJECT_CLASS (checkpoint_class); NMDBusObjectClass *dbus_object_class = NM_DBUS_OBJECT_CLASS (checkpoint_class); + g_type_class_add_private (object_class, sizeof (NMCheckpointPrivate)); + dbus_object_class->export_path = NM_DBUS_EXPORT_PATH_NUMBERED (NM_DBUS_PATH"/Checkpoint"); dbus_object_class->interface_infos = NM_DBUS_INTERFACE_INFOS (&interface_info_checkpoint); diff --git a/src/nm-checkpoint.h b/src/nm-checkpoint.h index 5d0f73295a..186c2f3803 100644 --- a/src/nm-checkpoint.h +++ b/src/nm-checkpoint.h @@ -35,7 +35,14 @@ #define NM_CHECKPOINT_CREATED "created" #define NM_CHECKPOINT_ROLLBACK_TIMEOUT "rollback-timeout" -typedef struct _NMCheckpoint NMCheckpoint; +typedef struct _NMCheckpointPrivate NMCheckpointPrivate; + +typedef struct { + NMDBusObject parent; + NMCheckpointPrivate *_priv; + CList checkpoints_lst; +} NMCheckpoint; + typedef struct _NMCheckpointClass NMCheckpointClass; GType nm_checkpoint_get_type (void); From 45c24fb9397d49c08a6d37b8ddcc7c06a5af0b58 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 27 Mar 2018 16:13:17 +0200 Subject: [PATCH 09/26] checkpoint/trivial: rename nm_checkpoint_manager_unref() to nm_checkpoint_manager_free() NMCheckpointManager was added and is not ref-countable, because it is not needed. I still often like for such objects (that are not ref-countable), that their destroy function is called "unref". Both for consistency, and also if we would later add ref-counting to the object. However, NMCheckpointManager keeps a pointer to NMManager. So, when NMManager gets destroyed, it *MUST* destroy the NMCheckpointManager. It cannot accept that the checkpoint manager outlives NMManager, but the "unref" name suggests that somebody else might have still a reference to this object keeping it alive. That is not the case. Rename so that this is clear. I would name it nm_checkpoint_manager_destroy(), but "destroy" already has a meaning for NMCheckpoint instances, so use "free". --- src/nm-checkpoint-manager.c | 2 +- src/nm-checkpoint-manager.h | 3 ++- src/nm-manager.c | 2 +- 3 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/nm-checkpoint-manager.c b/src/nm-checkpoint-manager.c index 911577f46d..fa163527a2 100644 --- a/src/nm-checkpoint-manager.c +++ b/src/nm-checkpoint-manager.c @@ -346,7 +346,7 @@ nm_checkpoint_manager_new (NMManager *manager, GParamSpec *spec) } void -nm_checkpoint_manager_unref (NMCheckpointManager *self) +nm_checkpoint_manager_free (NMCheckpointManager *self) { if (!self) return; diff --git a/src/nm-checkpoint-manager.h b/src/nm-checkpoint-manager.h index 4ff75303ed..a78743ac9e 100644 --- a/src/nm-checkpoint-manager.h +++ b/src/nm-checkpoint-manager.h @@ -28,7 +28,8 @@ typedef struct _NMCheckpointManager NMCheckpointManager; NMCheckpointManager *nm_checkpoint_manager_new (NMManager *manager, GParamSpec *spec); -void nm_checkpoint_manager_unref (NMCheckpointManager *self); + +void nm_checkpoint_manager_free (NMCheckpointManager *self); NMCheckpoint *nm_checkpoint_manager_create (NMCheckpointManager *self, const char *const*device_names, diff --git a/src/nm-manager.c b/src/nm-manager.c index 7629d0df34..4c4313a55a 100644 --- a/src/nm-manager.c +++ b/src/nm-manager.c @@ -6666,7 +6666,7 @@ dispose (GObject *object) if (priv->checkpoint_mgr) { nm_checkpoint_manager_destroy_all (priv->checkpoint_mgr, NULL); - g_clear_pointer (&priv->checkpoint_mgr, nm_checkpoint_manager_unref); + g_clear_pointer (&priv->checkpoint_mgr, nm_checkpoint_manager_free); } if (priv->auth_mgr) { From 6d6b3890867e39ac4448614e35315a0c12ab391e Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 27 Mar 2018 16:25:22 +0200 Subject: [PATCH 10/26] checkpoint/trivial: rename local variable @checkpoint_path path is long enough and (in this context) it consistently references the checkpoint "path". --- src/nm-checkpoint-manager.c | 34 +++++++++++++++++----------------- src/nm-checkpoint-manager.h | 4 ++-- 2 files changed, 19 insertions(+), 19 deletions(-) diff --git a/src/nm-checkpoint-manager.c b/src/nm-checkpoint-manager.c index fa163527a2..72066f7d44 100644 --- a/src/nm-checkpoint-manager.c +++ b/src/nm-checkpoint-manager.c @@ -154,10 +154,10 @@ nm_checkpoint_manager_create (NMCheckpointManager *self, { NMManager *manager; NMCheckpoint *checkpoint; - const char * const *path; + const char *const *dev_paths; gs_unref_ptrarray GPtrArray *devices = NULL; NMDevice *device; - const char *checkpoint_path; + const char *path; gs_free const char **device_paths_free = NULL; guint i; @@ -188,11 +188,11 @@ nm_checkpoint_manager_create (NMCheckpointManager *self, } devices = g_ptr_array_new (); - for (path = device_paths; *path; path++) { - device = nm_manager_get_device_by_path (manager, *path); + for (dev_paths = device_paths; *dev_paths; dev_paths++) { + device = nm_manager_get_device_by_path (manager, *dev_paths); if (!device) { g_set_error (error, NM_MANAGER_ERROR, NM_MANAGER_ERROR_UNKNOWN_DEVICE, - "device %s does not exist", *path); + "device %s does not exist", *dev_paths); return NULL; } g_ptr_array_add (devices, device); @@ -219,11 +219,11 @@ nm_checkpoint_manager_create (NMCheckpointManager *self, if (NM_FLAGS_HAS (flags, NM_CHECKPOINT_CREATE_FLAG_DESTROY_ALL)) g_hash_table_remove_all (self->checkpoints); - checkpoint_path = nm_dbus_object_export (NM_DBUS_OBJECT (checkpoint)); + path = nm_dbus_object_export (NM_DBUS_OBJECT (checkpoint)); c_list_link_tail (&self->checkpoints_lst_head, &checkpoint->checkpoints_lst); - if (!g_hash_table_insert (self->checkpoints, (gpointer) checkpoint_path, checkpoint)) + if (!g_hash_table_insert (self->checkpoints, (gpointer) path, checkpoint)) g_return_val_if_reached (NULL); notify_checkpoints (self); @@ -246,24 +246,24 @@ nm_checkpoint_manager_destroy_all (NMCheckpointManager *self, gboolean nm_checkpoint_manager_destroy (NMCheckpointManager *self, - const char *checkpoint_path, + const char *path, GError **error) { gboolean ret; g_return_val_if_fail (self, FALSE); - g_return_val_if_fail (checkpoint_path && checkpoint_path[0] == '/', FALSE); + g_return_val_if_fail (path && path[0] == '/', FALSE); g_return_val_if_fail (!error || !*error, FALSE); - if (!nm_streq (checkpoint_path, "/")) { - ret = g_hash_table_remove (self->checkpoints, checkpoint_path); + if (!nm_streq (path, "/")) { + ret = g_hash_table_remove (self->checkpoints, path); if (ret) { notify_checkpoints (self); } else { g_set_error (error, NM_MANAGER_ERROR, NM_MANAGER_ERROR_INVALID_ARGUMENTS, - "checkpoint %s does not exist", checkpoint_path); + "checkpoint %s does not exist", path); } return ret; } else @@ -272,26 +272,26 @@ nm_checkpoint_manager_destroy (NMCheckpointManager *self, gboolean nm_checkpoint_manager_rollback (NMCheckpointManager *self, - const char *checkpoint_path, + const char *path, GVariant **results, GError **error) { NMCheckpoint *checkpoint; g_return_val_if_fail (self, FALSE); - g_return_val_if_fail (checkpoint_path && checkpoint_path[0] == '/', FALSE); + g_return_val_if_fail (path && path[0] == '/', FALSE); g_return_val_if_fail (results, FALSE); g_return_val_if_fail (!error || !*error, FALSE); - checkpoint = g_hash_table_lookup (self->checkpoints, checkpoint_path); + checkpoint = g_hash_table_lookup (self->checkpoints, path); if (!checkpoint) { g_set_error (error, NM_MANAGER_ERROR, NM_MANAGER_ERROR_FAILED, - "checkpoint %s does not exist", checkpoint_path); + "checkpoint %s does not exist", path); return FALSE; } *results = nm_checkpoint_rollback (checkpoint); - g_hash_table_remove (self->checkpoints, checkpoint_path); + g_hash_table_remove (self->checkpoints, path); notify_checkpoints (self); return TRUE; diff --git a/src/nm-checkpoint-manager.h b/src/nm-checkpoint-manager.h index a78743ac9e..812c1a0060 100644 --- a/src/nm-checkpoint-manager.h +++ b/src/nm-checkpoint-manager.h @@ -41,10 +41,10 @@ gboolean nm_checkpoint_manager_destroy_all (NMCheckpointManager *self, GError **error); gboolean nm_checkpoint_manager_destroy (NMCheckpointManager *self, - const char *checkpoint_path, + const char *path, GError **error); gboolean nm_checkpoint_manager_rollback (NMCheckpointManager *self, - const char *checkpoint_path, + const char *path, GVariant **results, GError **error); From daf3d5cb535b23f94532483287497b95ef4ddfe0 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 27 Mar 2018 16:39:57 +0200 Subject: [PATCH 11/26] checkpoint: skip unrealized devices in nm_checkpoint_manager_create() We already do it for the case where no paths are provided. --- src/nm-checkpoint-manager.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/nm-checkpoint-manager.c b/src/nm-checkpoint-manager.c index 72066f7d44..5b434a1271 100644 --- a/src/nm-checkpoint-manager.c +++ b/src/nm-checkpoint-manager.c @@ -195,6 +195,11 @@ nm_checkpoint_manager_create (NMCheckpointManager *self, "device %s does not exist", *dev_paths); return NULL; } + if (!nm_device_is_real (device)) { + g_set_error (error, NM_MANAGER_ERROR, NM_MANAGER_ERROR_UNKNOWN_DEVICE, + "device %s is not realized", *dev_paths); + return NULL; + } g_ptr_array_add (devices, device); } From 63e3bff916d4df1968367421b0ba8df7986c1c3f Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 27 Mar 2018 16:42:26 +0200 Subject: [PATCH 12/26] checkpoint: refactor nm_checkpoint_manager_create() to simplify creating device list If no device paths are given, we can take the devices directly. We don't need to first create a list of paths, and then look them up by path again to add them to the list. --- src/nm-checkpoint-manager.c | 48 ++++++++++++++++--------------------- 1 file changed, 20 insertions(+), 28 deletions(-) diff --git a/src/nm-checkpoint-manager.c b/src/nm-checkpoint-manager.c index 5b434a1271..324a4ef571 100644 --- a/src/nm-checkpoint-manager.c +++ b/src/nm-checkpoint-manager.c @@ -154,53 +154,45 @@ nm_checkpoint_manager_create (NMCheckpointManager *self, { NMManager *manager; NMCheckpoint *checkpoint; - const char *const *dev_paths; gs_unref_ptrarray GPtrArray *devices = NULL; NMDevice *device; const char *path; - gs_free const char **device_paths_free = NULL; guint i; g_return_val_if_fail (self, FALSE); g_return_val_if_fail (!error || !*error, FALSE); manager = GET_MANAGER (self); - if (!device_paths || !device_paths[0]) { - const char *device_path; - const CList *tmp_lst; - GPtrArray *paths; + devices = g_ptr_array_new (); + + if (!device_paths || !device_paths[0]) { + const CList *tmp_lst; - paths = g_ptr_array_new (); nm_manager_for_each_device (manager, device, tmp_lst) { if (!nm_device_is_real (device)) continue; - device_path = nm_dbus_object_get_path (NM_DBUS_OBJECT (device)); - if (device_path) - g_ptr_array_add (paths, (gpointer) device_path); + nm_assert (nm_dbus_object_get_path (NM_DBUS_OBJECT (device))); + g_ptr_array_add (devices, device); } - g_ptr_array_add (paths, NULL); - device_paths_free = (const char **) g_ptr_array_free (paths, FALSE); - device_paths = (const char *const *) device_paths_free; } else if (NM_FLAGS_HAS (flags, NM_CHECKPOINT_CREATE_FLAG_DISCONNECT_NEW_DEVICES)) { g_set_error_literal (error, NM_MANAGER_ERROR, NM_MANAGER_ERROR_INVALID_ARGUMENTS, "the DISCONNECT_NEW_DEVICES flag can only be used with an empty device list"); return NULL; - } - - devices = g_ptr_array_new (); - for (dev_paths = device_paths; *dev_paths; dev_paths++) { - device = nm_manager_get_device_by_path (manager, *dev_paths); - if (!device) { - g_set_error (error, NM_MANAGER_ERROR, NM_MANAGER_ERROR_UNKNOWN_DEVICE, - "device %s does not exist", *dev_paths); - return NULL; + } else { + for (; *device_paths; device_paths++) { + device = nm_manager_get_device_by_path (manager, *device_paths); + if (!device) { + g_set_error (error, NM_MANAGER_ERROR, NM_MANAGER_ERROR_UNKNOWN_DEVICE, + "device %s does not exist", *device_paths); + return NULL; + } + if (!nm_device_is_real (device)) { + g_set_error (error, NM_MANAGER_ERROR, NM_MANAGER_ERROR_UNKNOWN_DEVICE, + "device %s is not realized", *device_paths); + return NULL; + } + g_ptr_array_add (devices, device); } - if (!nm_device_is_real (device)) { - g_set_error (error, NM_MANAGER_ERROR, NM_MANAGER_ERROR_UNKNOWN_DEVICE, - "device %s is not realized", *dev_paths); - return NULL; - } - g_ptr_array_add (devices, device); } if (!NM_FLAGS_HAS (flags, NM_CHECKPOINT_CREATE_FLAG_DESTROY_ALL)) { From 79458a558bdf45a789df3024f84942f85eb15875 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 27 Mar 2018 16:19:20 +0200 Subject: [PATCH 13/26] checkpoint: don't explicitly track checkpoints in a GHashTable We already have a GHashTable for exported objects. We can use that if we want to look up by path efficiently. --- src/nm-checkpoint-manager.c | 124 +++++++++++++++++++----------------- src/nm-checkpoint-manager.h | 9 ++- src/nm-manager.c | 15 ++--- 3 files changed, 76 insertions(+), 72 deletions(-) diff --git a/src/nm-checkpoint-manager.c b/src/nm-checkpoint-manager.c index 324a4ef571..c43c8f0fbd 100644 --- a/src/nm-checkpoint-manager.c +++ b/src/nm-checkpoint-manager.c @@ -35,7 +35,6 @@ struct _NMCheckpointManager { NMManager *_manager; GParamSpec *property_spec; - GHashTable *checkpoints; CList checkpoints_lst_head; guint rollback_timeout_id; }; @@ -60,6 +59,8 @@ struct _NMCheckpointManager { static void update_rollback_timeout (NMCheckpointManager *self); +/*****************************************************************************/ + static void notify_checkpoints (NMCheckpointManager *self) { g_object_notify_by_pspec ((GObject *) GET_MANAGER (self), @@ -67,11 +68,16 @@ notify_checkpoints (NMCheckpointManager *self) { } static void -item_destroy (gpointer data) +destroy_checkpoint (NMCheckpointManager *self, NMCheckpoint *checkpoint) { - NMCheckpoint *checkpoint = data; + nm_assert (NM_IS_CHECKPOINT (checkpoint)); + nm_assert (nm_dbus_object_is_exported (NM_DBUS_OBJECT (checkpoint))); + nm_assert (c_list_contains (&self->checkpoints_lst_head, &checkpoint->checkpoints_lst)); c_list_unlink (&checkpoint->checkpoints_lst); + + notify_checkpoints (self); + nm_dbus_object_unexport (NM_DBUS_OBJECT (checkpoint)); g_object_unref (checkpoint); } @@ -82,8 +88,8 @@ rollback_timeout_cb (NMCheckpointManager *self) NMCheckpoint *checkpoint, *checkpoint_safe; GVariant *result; gint64 ts, now; - const char *path; - gboolean removed = FALSE; + + self->rollback_timeout_id = 0; now = nm_utils_get_monotonic_timestamp_ms (); @@ -93,19 +99,12 @@ rollback_timeout_cb (NMCheckpointManager *self) result = nm_checkpoint_rollback (checkpoint); if (result) g_variant_unref (result); - path = nm_dbus_object_get_path (NM_DBUS_OBJECT (checkpoint)); - if (!g_hash_table_remove (self->checkpoints, path)) - nm_assert_not_reached(); - removed = TRUE; + destroy_checkpoint (self, checkpoint); } } - self->rollback_timeout_id = 0; update_rollback_timeout (self); - if (removed) - notify_checkpoints (self); - return G_SOURCE_REMOVE; } @@ -156,7 +155,6 @@ nm_checkpoint_manager_create (NMCheckpointManager *self, NMCheckpoint *checkpoint; gs_unref_ptrarray GPtrArray *devices = NULL; NMDevice *device; - const char *path; guint i; g_return_val_if_fail (self, FALSE); @@ -214,31 +212,25 @@ nm_checkpoint_manager_create (NMCheckpointManager *self, return NULL; if (NM_FLAGS_HAS (flags, NM_CHECKPOINT_CREATE_FLAG_DESTROY_ALL)) - g_hash_table_remove_all (self->checkpoints); + nm_checkpoint_manager_destroy_all (self); - path = nm_dbus_object_export (NM_DBUS_OBJECT (checkpoint)); + nm_dbus_object_export (NM_DBUS_OBJECT (checkpoint)); c_list_link_tail (&self->checkpoints_lst_head, &checkpoint->checkpoints_lst); - - if (!g_hash_table_insert (self->checkpoints, (gpointer) path, checkpoint)) - g_return_val_if_reached (NULL); - notify_checkpoints (self); update_rollback_timeout (self); - return checkpoint; } -gboolean -nm_checkpoint_manager_destroy_all (NMCheckpointManager *self, - GError **error) +void +nm_checkpoint_manager_destroy_all (NMCheckpointManager *self) { - g_return_val_if_fail (self, FALSE); + NMCheckpoint *checkpoint; - g_hash_table_remove_all (self->checkpoints); - notify_checkpoints (self); + g_return_if_fail (self); - return TRUE; + while ((checkpoint = c_list_first_entry (&self->checkpoints_lst_head, NMCheckpoint, checkpoints_lst))) + destroy_checkpoint (self, checkpoint); } gboolean @@ -246,25 +238,28 @@ nm_checkpoint_manager_destroy (NMCheckpointManager *self, const char *path, GError **error) { - gboolean ret; + NMCheckpoint *checkpoint; g_return_val_if_fail (self, FALSE); g_return_val_if_fail (path && path[0] == '/', FALSE); g_return_val_if_fail (!error || !*error, FALSE); if (!nm_streq (path, "/")) { - ret = g_hash_table_remove (self->checkpoints, path); - if (ret) { - notify_checkpoints (self); - } else { - g_set_error (error, - NM_MANAGER_ERROR, - NM_MANAGER_ERROR_INVALID_ARGUMENTS, - "checkpoint %s does not exist", path); - } - return ret; - } else - return nm_checkpoint_manager_destroy_all (self, error); + nm_checkpoint_manager_destroy_all (self); + return TRUE; + } + + checkpoint = nm_checkpoint_manager_lookup_by_path (self, path); + if (!checkpoint) { + g_set_error (error, + NM_MANAGER_ERROR, + NM_MANAGER_ERROR_INVALID_ARGUMENTS, + "checkpoint %s does not exist", path); + return FALSE; + } + + destroy_checkpoint (self, checkpoint); + return TRUE; } gboolean @@ -280,7 +275,7 @@ nm_checkpoint_manager_rollback (NMCheckpointManager *self, g_return_val_if_fail (results, FALSE); g_return_val_if_fail (!error || !*error, FALSE); - checkpoint = g_hash_table_lookup (self->checkpoints, path); + checkpoint = nm_checkpoint_manager_lookup_by_path (self, path); if (!checkpoint) { g_set_error (error, NM_MANAGER_ERROR, NM_MANAGER_ERROR_FAILED, "checkpoint %s does not exist", path); @@ -288,31 +283,44 @@ nm_checkpoint_manager_rollback (NMCheckpointManager *self, } *results = nm_checkpoint_rollback (checkpoint); - g_hash_table_remove (self->checkpoints, path); - notify_checkpoints (self); - + destroy_checkpoint (self, checkpoint); return TRUE; } -char ** -nm_checkpoint_manager_get_checkpoint_paths (NMCheckpointManager *self) +NMCheckpoint * +nm_checkpoint_manager_lookup_by_path (NMCheckpointManager *self, const char *path) { NMCheckpoint *checkpoint; - char **strv; + + g_return_val_if_fail (self, NULL); + + checkpoint = (NMCheckpoint *) nm_dbus_manager_lookup_object (nm_dbus_object_get_manager (NM_DBUS_OBJECT (GET_MANAGER (self))), + path); + if ( !checkpoint + || !NM_IS_CHECKPOINT (checkpoint)) + return NULL; + + nm_assert (c_list_contains (&self->checkpoints_lst_head, &checkpoint->checkpoints_lst)); + return checkpoint; +} + +const char ** +nm_checkpoint_manager_get_checkpoint_paths (NMCheckpointManager *self, guint *out_length) +{ + NMCheckpoint *checkpoint; + const char **strv; guint num, i = 0; - num = g_hash_table_size (self->checkpoints); - if (!num) { - nm_assert (c_list_is_empty (&self->checkpoints_lst_head)); + num = c_list_length (&self->checkpoints_lst_head); + NM_SET_OUT (out_length, num); + if (!num) return NULL; - } - strv = g_new (char *, num + 1); + strv = g_new (const char *, num + 1); c_list_for_each_entry (checkpoint, &self->checkpoints_lst_head, checkpoints_lst) - strv[i++] = g_strdup (nm_dbus_object_get_path (NM_DBUS_OBJECT (checkpoint))); + strv[i++] = nm_dbus_object_get_path (NM_DBUS_OBJECT (checkpoint)); nm_assert (i == num); strv[i] = NULL; - return strv; } @@ -334,11 +342,8 @@ nm_checkpoint_manager_new (NMManager *manager, GParamSpec *spec) * of NMManager shall surpass the lifetime of the NMCheckpointManager * instance. */ self->_manager = manager; - self->checkpoints = g_hash_table_new_full (nm_str_hash, g_str_equal, - NULL, item_destroy); self->property_spec = spec; c_list_init (&self->checkpoints_lst_head); - return self; } @@ -348,8 +353,7 @@ nm_checkpoint_manager_free (NMCheckpointManager *self) if (!self) return; + nm_checkpoint_manager_destroy_all (self); nm_clear_g_source (&self->rollback_timeout_id); - g_hash_table_destroy (self->checkpoints); - g_slice_free (NMCheckpointManager, self); } diff --git a/src/nm-checkpoint-manager.h b/src/nm-checkpoint-manager.h index 812c1a0060..bcdf0d3602 100644 --- a/src/nm-checkpoint-manager.h +++ b/src/nm-checkpoint-manager.h @@ -31,14 +31,16 @@ NMCheckpointManager *nm_checkpoint_manager_new (NMManager *manager, GParamSpec * void nm_checkpoint_manager_free (NMCheckpointManager *self); +NMCheckpoint *nm_checkpoint_manager_lookup_by_path (NMCheckpointManager *self, + const char *path); + NMCheckpoint *nm_checkpoint_manager_create (NMCheckpointManager *self, const char *const*device_names, guint32 rollback_timeout, NMCheckpointCreateFlags flags, GError **error); -gboolean nm_checkpoint_manager_destroy_all (NMCheckpointManager *self, - GError **error); +void nm_checkpoint_manager_destroy_all (NMCheckpointManager *self); gboolean nm_checkpoint_manager_destroy (NMCheckpointManager *self, const char *path, @@ -48,6 +50,7 @@ gboolean nm_checkpoint_manager_rollback (NMCheckpointManager *self, GVariant **results, GError **error); -char **nm_checkpoint_manager_get_checkpoint_paths (NMCheckpointManager *self); +const char **nm_checkpoint_manager_get_checkpoint_paths (NMCheckpointManager *self, + guint *out_length); #endif /* __NM_CHECKPOINT_MANAGER_H__ */ diff --git a/src/nm-manager.c b/src/nm-manager.c index 4c4313a55a..71ccb55864 100644 --- a/src/nm-manager.c +++ b/src/nm-manager.c @@ -6471,7 +6471,6 @@ get_property (GObject *object, guint prop_id, NMConfigData *config_data; const NMGlobalDnsConfig *dns_config; const char *type; - char **strv; const char *path; NMActiveConnection *ac; GPtrArray *ptrarr; @@ -6578,10 +6577,11 @@ get_property (GObject *object, guint prop_id, TRUE))); break; case PROP_CHECKPOINTS: - strv = NULL; - if (priv->checkpoint_mgr) - strv = nm_checkpoint_manager_get_checkpoint_paths (priv->checkpoint_mgr); - g_value_take_boxed (value, strv); + g_value_take_boxed (value, + priv->checkpoint_mgr + ? nm_utils_strv_make_deep_copied (nm_checkpoint_manager_get_checkpoint_paths (priv->checkpoint_mgr, + NULL)) + : NULL); break; default: G_OBJECT_WARN_INVALID_PROPERTY_ID (object, prop_id, pspec); @@ -6664,10 +6664,7 @@ dispose (GObject *object) nm_clear_g_source (&priv->devices_inited_id); - if (priv->checkpoint_mgr) { - nm_checkpoint_manager_destroy_all (priv->checkpoint_mgr, NULL); - g_clear_pointer (&priv->checkpoint_mgr, nm_checkpoint_manager_free); - } + g_clear_pointer (&priv->checkpoint_mgr, nm_checkpoint_manager_free); if (priv->auth_mgr) { g_signal_handlers_disconnect_by_func (priv->auth_mgr, From e6e0eb92b9bac246d3d4a1cc2b2b7c2c091cc0d0 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 27 Mar 2018 19:02:15 +0200 Subject: [PATCH 14/26] checkpoint: minor cleanup rolling back checkpoints --- src/nm-checkpoint-manager.c | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/src/nm-checkpoint-manager.c b/src/nm-checkpoint-manager.c index c43c8f0fbd..c200545c82 100644 --- a/src/nm-checkpoint-manager.c +++ b/src/nm-checkpoint-manager.c @@ -82,11 +82,20 @@ destroy_checkpoint (NMCheckpointManager *self, NMCheckpoint *checkpoint) g_object_unref (checkpoint); } +static GVariant * +rollback_checkpoint (NMCheckpointManager *self, NMCheckpoint *checkpoint) +{ + GVariant *result; + + result = nm_checkpoint_rollback (checkpoint); + destroy_checkpoint (self, checkpoint); + return result; +} + static gboolean rollback_timeout_cb (NMCheckpointManager *self) { NMCheckpoint *checkpoint, *checkpoint_safe; - GVariant *result; gint64 ts, now; self->rollback_timeout_id = 0; @@ -96,10 +105,9 @@ rollback_timeout_cb (NMCheckpointManager *self) c_list_for_each_entry_safe (checkpoint, checkpoint_safe, &self->checkpoints_lst_head, checkpoints_lst) { ts = nm_checkpoint_get_rollback_ts (checkpoint); if (ts && ts <= now) { - result = nm_checkpoint_rollback (checkpoint); - if (result) - g_variant_unref (result); - destroy_checkpoint (self, checkpoint); + gs_unref_variant GVariant *result = NULL; + + result = rollback_checkpoint (self, checkpoint); } } @@ -282,8 +290,7 @@ nm_checkpoint_manager_rollback (NMCheckpointManager *self, return FALSE; } - *results = nm_checkpoint_rollback (checkpoint); - destroy_checkpoint (self, checkpoint); + *results = rollback_checkpoint (self, checkpoint); return TRUE; } From 6f28749ad425dff2a745e74009dd792930f64d5d Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 27 Mar 2018 19:26:11 +0200 Subject: [PATCH 15/26] checkpoint: simplify device_checkpoint_create() to never fail It never failed already. Don't pretend it could. --- src/nm-checkpoint.c | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/src/nm-checkpoint.c b/src/nm-checkpoint.c index ce36c7c9aa..a202c0639f 100644 --- a/src/nm-checkpoint.c +++ b/src/nm-checkpoint.c @@ -365,8 +365,7 @@ next_dev: } static DeviceCheckpoint * -device_checkpoint_create (NMDevice *device, - GError **error) +device_checkpoint_create (NMDevice *device) { DeviceCheckpoint *dev_checkpoint; NMConnection *applied_connection; @@ -374,6 +373,8 @@ device_checkpoint_create (NMDevice *device, const char *path; NMActRequest *act_request; + nm_assert (NM_IS_DEVICE (device)); + path = nm_dbus_object_get_path (NM_DBUS_OBJECT (device)); dev_checkpoint = g_slice_new0 (DeviceCheckpoint); @@ -471,8 +472,6 @@ nm_checkpoint_new (NMManager *manager, GPtrArray *devices, guint32 rollback_time NMCheckpoint *self; NMCheckpointPrivate *priv; NMSettingsConnection *const *con; - DeviceCheckpoint *dev_checkpoint; - NMDevice *device; guint i; g_return_val_if_fail (manager, NULL); @@ -508,13 +507,11 @@ nm_checkpoint_new (NMManager *manager, GPtrArray *devices, guint32 rollback_time } for (i = 0; i < devices->len; i++) { - device = (NMDevice *) devices->pdata[i]; - dev_checkpoint = device_checkpoint_create (device, error); - if (!dev_checkpoint) { - g_object_unref (self); - return NULL; - } - g_hash_table_insert (priv->devices, device, dev_checkpoint); + NMDevice *device = devices->pdata[i]; + + g_hash_table_insert (priv->devices, + device, + device_checkpoint_create (device)); } return self; From 5fb65b7f96346f04051ebfedd1f6b6e202e727dd Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 27 Mar 2018 19:02:15 +0200 Subject: [PATCH 16/26] checkpoint: let each checkpoint schedule its own timeout Instead of scheduling one timeout only, let each checkpoint instance individually schedule a timeout. This has some overhead, but glib is supposed to make scheduling many timers efficient. Otherwise, glib should be fixed. This simplifies in my opinion the code, because it's up to each checkpoint to maintain its own timeout. Later we will also add a AdjustRollbackTimeout operation, which allow to reschedule the timeout. It also seems slightly simpler, if scheduling of the timeout is done by the NMCheckpoint instance itself. --- src/nm-checkpoint-manager.c | 57 +++++--------------------------- src/nm-checkpoint.c | 66 ++++++++++++++++++++++++++++--------- src/nm-checkpoint.h | 8 ++++- 3 files changed, 65 insertions(+), 66 deletions(-) diff --git a/src/nm-checkpoint-manager.c b/src/nm-checkpoint-manager.c index c200545c82..a88c4a763c 100644 --- a/src/nm-checkpoint-manager.c +++ b/src/nm-checkpoint-manager.c @@ -36,7 +36,6 @@ struct _NMCheckpointManager { NMManager *_manager; GParamSpec *property_spec; CList checkpoints_lst_head; - guint rollback_timeout_id; }; #define GET_MANAGER(self) \ @@ -57,10 +56,6 @@ struct _NMCheckpointManager { /*****************************************************************************/ -static void update_rollback_timeout (NMCheckpointManager *self); - -/*****************************************************************************/ - static void notify_checkpoints (NMCheckpointManager *self) { g_object_notify_by_pspec ((GObject *) GET_MANAGER (self), @@ -74,6 +69,8 @@ destroy_checkpoint (NMCheckpointManager *self, NMCheckpoint *checkpoint) nm_assert (nm_dbus_object_is_exported (NM_DBUS_OBJECT (checkpoint))); nm_assert (c_list_contains (&self->checkpoints_lst_head, &checkpoint->checkpoints_lst)); + nm_checkpoint_set_timeout_callback (checkpoint, NULL, NULL); + c_list_unlink (&checkpoint->checkpoints_lst); notify_checkpoints (self); @@ -92,51 +89,14 @@ rollback_checkpoint (NMCheckpointManager *self, NMCheckpoint *checkpoint) return result; } -static gboolean -rollback_timeout_cb (NMCheckpointManager *self) -{ - NMCheckpoint *checkpoint, *checkpoint_safe; - gint64 ts, now; - - self->rollback_timeout_id = 0; - - now = nm_utils_get_monotonic_timestamp_ms (); - - c_list_for_each_entry_safe (checkpoint, checkpoint_safe, &self->checkpoints_lst_head, checkpoints_lst) { - ts = nm_checkpoint_get_rollback_ts (checkpoint); - if (ts && ts <= now) { - gs_unref_variant GVariant *result = NULL; - - result = rollback_checkpoint (self, checkpoint); - } - } - - update_rollback_timeout (self); - - return G_SOURCE_REMOVE; -} - static void -update_rollback_timeout (NMCheckpointManager *self) +rollback_timeout_cb (NMCheckpoint *checkpoint, + gpointer user_data) { - NMCheckpoint *checkpoint; - gint64 ts, delta, next = G_MAXINT64; + NMCheckpointManager *self = user_data; + gs_unref_variant GVariant *result = NULL; - c_list_for_each_entry (checkpoint, &self->checkpoints_lst_head, checkpoints_lst) { - ts = nm_checkpoint_get_rollback_ts (checkpoint); - if (ts && ts < next) - next = ts; - } - - nm_clear_g_source (&self->rollback_timeout_id); - - if (next != G_MAXINT64) { - delta = MAX (next - nm_utils_get_monotonic_timestamp_ms (), 0); - self->rollback_timeout_id = g_timeout_add (delta, - (GSourceFunc) rollback_timeout_cb, - self); - _LOGT ("update timeout: next check in %" G_GINT64_FORMAT " ms", delta); - } + result = rollback_checkpoint (self, checkpoint); } static NMCheckpoint * @@ -224,9 +184,9 @@ nm_checkpoint_manager_create (NMCheckpointManager *self, nm_dbus_object_export (NM_DBUS_OBJECT (checkpoint)); + nm_checkpoint_set_timeout_callback (checkpoint, rollback_timeout_cb, self); c_list_link_tail (&self->checkpoints_lst_head, &checkpoint->checkpoints_lst); notify_checkpoints (self); - update_rollback_timeout (self); return checkpoint; } @@ -361,6 +321,5 @@ nm_checkpoint_manager_free (NMCheckpointManager *self) return; nm_checkpoint_manager_destroy_all (self); - nm_clear_g_source (&self->rollback_timeout_id); g_slice_free (NMCheckpointManager, self); } diff --git a/src/nm-checkpoint.c b/src/nm-checkpoint.c index a202c0639f..ed70ff4bfd 100644 --- a/src/nm-checkpoint.c +++ b/src/nm-checkpoint.c @@ -57,13 +57,17 @@ NM_GOBJECT_PROPERTIES_DEFINE_BASE ( struct _NMCheckpointPrivate { /* properties */ GHashTable *devices; - gint64 created; - guint32 rollback_timeout; + gint64 created_at_ms; + guint32 rollback_timeout_s; + guint timeout_id; + /* private members */ /* private members */ NMManager *manager; - gint64 rollback_ts; NMCheckpointCreateFlags flags; GHashTable *connection_uuids; + + NMCheckpointTimeoutCallback timeout_cb; + gpointer timeout_data; }; struct _NMCheckpointClass { @@ -96,12 +100,18 @@ G_DEFINE_TYPE (NMCheckpoint, nm_checkpoint, NM_TYPE_DBUS_OBJECT) /*****************************************************************************/ -guint64 -nm_checkpoint_get_rollback_ts (NMCheckpoint *self) +void +nm_checkpoint_set_timeout_callback (NMCheckpoint *self, + NMCheckpointTimeoutCallback callback, + gpointer user_data) { - g_return_val_if_fail (NM_IS_CHECKPOINT (self), 0); + NMCheckpointPrivate *priv = NM_CHECKPOINT_GET_PRIVATE (self); - return NM_CHECKPOINT_GET_PRIVATE (self)->rollback_ts; + /* in glib world, we would have a GSignal for this. But as there + * is only one subscriber, it's simpler to just set and unset(!) + * the callback this way. */ + priv->timeout_cb = callback; + priv->timeout_data = user_data; } gboolean @@ -374,6 +384,7 @@ device_checkpoint_create (NMDevice *device) NMActRequest *act_request; nm_assert (NM_IS_DEVICE (device)); + nm_assert (nm_device_is_real (device)); path = nm_dbus_object_get_path (NM_DBUS_OBJECT (device)); @@ -421,6 +432,21 @@ device_checkpoint_destroy (gpointer data) g_slice_free (DeviceCheckpoint, dev_checkpoint); } +static gboolean +_timeout_cb (gpointer user_data) +{ + NMCheckpoint *self = user_data; + NMCheckpointPrivate *priv = NM_CHECKPOINT_GET_PRIVATE (self); + + priv->timeout_id = 0; + + if (priv->timeout_cb) + priv->timeout_cb (self, priv->timeout_data); + + /* beware, @self likely got destroyed! */ + return G_SOURCE_REMOVE; +} + /*****************************************************************************/ static void @@ -437,10 +463,12 @@ get_property (GObject *object, guint prop_id, FALSE); break; case PROP_CREATED: - g_value_set_int64 (value, priv->created); + g_value_set_int64 (value, + nm_utils_monotonic_timestamp_as_boottime (priv->created_at_ms, + NM_UTILS_NS_PER_MSEC)); break; case PROP_ROLLBACK_TIMEOUT: - g_value_set_uint (value, priv->rollback_timeout); + g_value_set_uint (value, priv->rollback_timeout_s); break; default: G_OBJECT_WARN_INVALID_PROPERTY_ID (object, prop_id, pspec); @@ -466,12 +494,13 @@ nm_checkpoint_init (NMCheckpoint *self) } NMCheckpoint * -nm_checkpoint_new (NMManager *manager, GPtrArray *devices, guint32 rollback_timeout, +nm_checkpoint_new (NMManager *manager, GPtrArray *devices, guint32 rollback_timeout_s, NMCheckpointCreateFlags flags, GError **error) { NMCheckpoint *self; NMCheckpointPrivate *priv; NMSettingsConnection *const *con; + gint64 rollback_timeout_ms; guint i; g_return_val_if_fail (manager, NULL); @@ -490,14 +519,17 @@ nm_checkpoint_new (NMManager *manager, GPtrArray *devices, guint32 rollback_time priv = NM_CHECKPOINT_GET_PRIVATE (self); priv->manager = manager; - priv->created = nm_utils_monotonic_timestamp_as_boottime (nm_utils_get_monotonic_timestamp_ms (), - NM_UTILS_NS_PER_MSEC); - priv->rollback_timeout = rollback_timeout; - priv->rollback_ts = rollback_timeout ? - (nm_utils_get_monotonic_timestamp_ms () + ((gint64) rollback_timeout * 1000)) : - 0; + priv->rollback_timeout_s = rollback_timeout_s; + priv->created_at_ms = nm_utils_get_monotonic_timestamp_ms (); priv->flags = flags; + if (rollback_timeout_s != 0) { + rollback_timeout_ms = ((gint64) rollback_timeout_s) * 1000; + priv->timeout_id = g_timeout_add (NM_MIN (rollback_timeout_ms, (gint64) G_MAXUINT32), + _timeout_cb, + self); + } + if (NM_FLAGS_HAS (flags, NM_CHECKPOINT_CREATE_FLAG_DELETE_NEW_CONNECTIONS)) { priv->connection_uuids = g_hash_table_new_full (nm_str_hash, g_str_equal, g_free, NULL); for (con = nm_settings_get_connections (nm_settings_get (), NULL); *con; con++) { @@ -528,6 +560,8 @@ dispose (GObject *object) g_clear_pointer (&priv->devices, g_hash_table_unref); g_clear_pointer (&priv->connection_uuids, g_hash_table_unref); + nm_clear_g_source (&priv->timeout_id); + G_OBJECT_CLASS (nm_checkpoint_parent_class)->dispose (object); } diff --git a/src/nm-checkpoint.h b/src/nm-checkpoint.h index 186c2f3803..59490322e3 100644 --- a/src/nm-checkpoint.h +++ b/src/nm-checkpoint.h @@ -50,7 +50,13 @@ GType nm_checkpoint_get_type (void); NMCheckpoint *nm_checkpoint_new (NMManager *manager, GPtrArray *devices, guint32 rollback_timeout, NMCheckpointCreateFlags flags, GError **error); -guint64 nm_checkpoint_get_rollback_ts (NMCheckpoint *checkpoint); +typedef void (*NMCheckpointTimeoutCallback) (NMCheckpoint *self, + gpointer user_data); + +void nm_checkpoint_set_timeout_callback (NMCheckpoint *self, + NMCheckpointTimeoutCallback callback, + gpointer user_data); + gboolean nm_checkpoint_includes_device (NMCheckpoint *checkpoint, NMDevice *device); GVariant *nm_checkpoint_rollback (NMCheckpoint *self); From e5fc9a307d036fe814145a5100bb6bdf28216cc6 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 28 Mar 2018 07:08:54 +0200 Subject: [PATCH 17/26] checkpoint: don't let nm_checkpoint_new() fail We already do error checking in nm_checkpoint_manager_create(). No need to split it in two places. Let all error conditions be handled by nm_checkpoint_manager_create() first, and then once we decide all is good, nm_checkpoint_new() can no longer fail. --- src/nm-checkpoint-manager.c | 12 +++++++++--- src/nm-checkpoint.c | 12 ++---------- src/nm-checkpoint.h | 2 +- 3 files changed, 12 insertions(+), 14 deletions(-) diff --git a/src/nm-checkpoint-manager.c b/src/nm-checkpoint-manager.c index a88c4a763c..1086a0cb46 100644 --- a/src/nm-checkpoint-manager.c +++ b/src/nm-checkpoint-manager.c @@ -161,6 +161,14 @@ nm_checkpoint_manager_create (NMCheckpointManager *self, } } + if (!devices->len) { + g_set_error_literal (error, + NM_MANAGER_ERROR, + NM_MANAGER_ERROR_INVALID_ARGUMENTS, + "no device available"); + return NULL; + } + if (!NM_FLAGS_HAS (flags, NM_CHECKPOINT_CREATE_FLAG_DESTROY_ALL)) { for (i = 0; i < devices->len; i++) { device = devices->pdata[i]; @@ -175,9 +183,7 @@ nm_checkpoint_manager_create (NMCheckpointManager *self, } } - checkpoint = nm_checkpoint_new (manager, devices, rollback_timeout, flags, error); - if (!checkpoint) - return NULL; + checkpoint = nm_checkpoint_new (manager, devices, rollback_timeout, flags); if (NM_FLAGS_HAS (flags, NM_CHECKPOINT_CREATE_FLAG_DESTROY_ALL)) nm_checkpoint_manager_destroy_all (self); diff --git a/src/nm-checkpoint.c b/src/nm-checkpoint.c index ed70ff4bfd..55968843e1 100644 --- a/src/nm-checkpoint.c +++ b/src/nm-checkpoint.c @@ -495,7 +495,7 @@ nm_checkpoint_init (NMCheckpoint *self) NMCheckpoint * nm_checkpoint_new (NMManager *manager, GPtrArray *devices, guint32 rollback_timeout_s, - NMCheckpointCreateFlags flags, GError **error) + NMCheckpointCreateFlags flags) { NMCheckpoint *self; NMCheckpointPrivate *priv; @@ -505,15 +505,7 @@ nm_checkpoint_new (NMManager *manager, GPtrArray *devices, guint32 rollback_time g_return_val_if_fail (manager, NULL); g_return_val_if_fail (devices, NULL); - g_return_val_if_fail (!error || !*error, NULL); - - if (!devices->len) { - g_set_error_literal (error, - NM_MANAGER_ERROR, - NM_MANAGER_ERROR_INVALID_ARGUMENTS, - "no device available"); - return NULL; - } + g_return_val_if_fail (devices->len > 0, NULL); self = g_object_new (NM_TYPE_CHECKPOINT, NULL); diff --git a/src/nm-checkpoint.h b/src/nm-checkpoint.h index 59490322e3..49783ca533 100644 --- a/src/nm-checkpoint.h +++ b/src/nm-checkpoint.h @@ -48,7 +48,7 @@ typedef struct _NMCheckpointClass NMCheckpointClass; GType nm_checkpoint_get_type (void); NMCheckpoint *nm_checkpoint_new (NMManager *manager, GPtrArray *devices, guint32 rollback_timeout, - NMCheckpointCreateFlags flags, GError **error); + NMCheckpointCreateFlags flags); typedef void (*NMCheckpointTimeoutCallback) (NMCheckpoint *self, gpointer user_data); From 5c283356a1df42e5b94cf0efb5a0d352c8343216 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 28 Mar 2018 07:06:10 +0200 Subject: [PATCH 18/26] checkpoint: allow overlapping checkpoints Introduce a new flag NM_CHECKPOINT_CREATE_FLAG_ALLOW_OVERLAPPING that allows the creation of overlapping checkpoints. Before, and by default, checkpoints that reference a same device conflict, and creating such a checkpoint failed. Now, allow this. But during rollback automatically destroy all overlapping checkpoints that were created after the checkpoint that is about to rollback. With this, you can create a series of checkpoints, and rollback them individually. With the restriction, that if you once rolled back to an older checkpoint, you no longer can roll"forward" to a younger one. What this implies and what is new here, is that the checkpoint might be automatically destroyed by NetworkManager before the timeout expires. When the user later would try to manually destroy/rollback such a checkpoint, it would fail because the checkpoint no longer exists. --- libnm-core/nm-dbus-interface.h | 13 ++++++++ src/nm-checkpoint-manager.c | 56 ++++++++++++++++++---------------- src/nm-checkpoint.c | 33 ++++++++++++++++++-- src/nm-checkpoint.h | 6 +++- 4 files changed, 78 insertions(+), 30 deletions(-) diff --git a/libnm-core/nm-dbus-interface.h b/libnm-core/nm-dbus-interface.h index b2e448dfa3..0a9c41916d 100644 --- a/libnm-core/nm-dbus-interface.h +++ b/libnm-core/nm-dbus-interface.h @@ -842,6 +842,18 @@ typedef enum { * delete any new connection added after the checkpoint (Since: 1.6) * @NM_CHECKPOINT_CREATE_FLAG_DISCONNECT_NEW_DEVICES: upon rollback, * disconnect any new device appeared after the checkpoint (Since: 1.6) + * @NM_CHECKPOINT_CREATE_FLAG_ALLOW_OVERLAPPING: by default, creating + * a checkpoint fails if there are already existing checkoints that + * reference the same devices. With this flag, creation of such + * checkpoints is allowed, however, if an older checkpoint + * that references overlapping devices gets rolled back, it will + * automatically destroy this checkpoint during rollback. This + * allows to create several overlapping checkpoints in parallel, + * and rollback to them at will. With the special case that + * rolling back to an older checkpoint will invalidate all + * overlapping younger checkpoints. This opts-in that the + * checkpoint can be automatically destroyed by the rollback + * of an older checkpoint. (Since: 1.12) * * The flags for CheckpointCreate call * @@ -852,6 +864,7 @@ typedef enum { /*< skip >*/ NM_CHECKPOINT_CREATE_FLAG_DESTROY_ALL = 0x01, NM_CHECKPOINT_CREATE_FLAG_DELETE_NEW_CONNECTIONS = 0x02, NM_CHECKPOINT_CREATE_FLAG_DISCONNECT_NEW_DEVICES = 0x04, + NM_CHECKPOINT_CREATE_FLAG_ALLOW_OVERLAPPING = 0x08, } NMCheckpointCreateFlags; /** diff --git a/src/nm-checkpoint-manager.c b/src/nm-checkpoint-manager.c index 1086a0cb46..173eaf5a48 100644 --- a/src/nm-checkpoint-manager.c +++ b/src/nm-checkpoint-manager.c @@ -63,7 +63,7 @@ notify_checkpoints (NMCheckpointManager *self) { } static void -destroy_checkpoint (NMCheckpointManager *self, NMCheckpoint *checkpoint) +destroy_checkpoint (NMCheckpointManager *self, NMCheckpoint *checkpoint, gboolean log_destroy) { nm_assert (NM_IS_CHECKPOINT (checkpoint)); nm_assert (nm_dbus_object_is_exported (NM_DBUS_OBJECT (checkpoint))); @@ -73,6 +73,9 @@ destroy_checkpoint (NMCheckpointManager *self, NMCheckpoint *checkpoint) c_list_unlink (&checkpoint->checkpoints_lst); + if (log_destroy) + nm_checkpoint_log_destroy (checkpoint); + notify_checkpoints (self); nm_dbus_object_unexport (NM_DBUS_OBJECT (checkpoint)); @@ -83,9 +86,26 @@ static GVariant * rollback_checkpoint (NMCheckpointManager *self, NMCheckpoint *checkpoint) { GVariant *result; + const CList *iter; + + nm_assert (c_list_contains (&self->checkpoints_lst_head, &checkpoint->checkpoints_lst)); + + /* we destroy first all overlapping checkpoints that are younger/newer. */ + for (iter = checkpoint->checkpoints_lst.next; + iter != &self->checkpoints_lst_head; + ) { + NMCheckpoint *cp = c_list_entry (iter, NMCheckpoint, checkpoints_lst); + + iter = iter->next; + if (nm_checkpoint_includes_devices_of (cp, checkpoint)) { + /* the younger checkpoint has overlapping devices and gets obsoleted. + * Destroy it. */ + destroy_checkpoint (self, cp, TRUE); + } + } result = nm_checkpoint_rollback (checkpoint); - destroy_checkpoint (self, checkpoint); + destroy_checkpoint (self, checkpoint, FALSE); return result; } @@ -99,19 +119,6 @@ rollback_timeout_cb (NMCheckpoint *checkpoint, result = rollback_checkpoint (self, checkpoint); } -static NMCheckpoint * -find_checkpoint_for_device (NMCheckpointManager *self, NMDevice *device) -{ - NMCheckpoint *checkpoint; - - c_list_for_each_entry (checkpoint, &self->checkpoints_lst_head, checkpoints_lst) { - if (nm_checkpoint_includes_device (checkpoint, device)) - return checkpoint; - } - - return NULL; -} - NMCheckpoint * nm_checkpoint_manager_create (NMCheckpointManager *self, const char *const *device_paths, @@ -123,7 +130,6 @@ nm_checkpoint_manager_create (NMCheckpointManager *self, NMCheckpoint *checkpoint; gs_unref_ptrarray GPtrArray *devices = NULL; NMDevice *device; - guint i; g_return_val_if_fail (self, FALSE); g_return_val_if_fail (!error || !*error, FALSE); @@ -169,11 +175,12 @@ nm_checkpoint_manager_create (NMCheckpointManager *self, return NULL; } - if (!NM_FLAGS_HAS (flags, NM_CHECKPOINT_CREATE_FLAG_DESTROY_ALL)) { - for (i = 0; i < devices->len; i++) { - device = devices->pdata[i]; - checkpoint = find_checkpoint_for_device (self, device); - if (checkpoint) { + if (NM_FLAGS_HAS (flags, NM_CHECKPOINT_CREATE_FLAG_DESTROY_ALL)) + nm_checkpoint_manager_destroy_all (self); + else if (!NM_FLAGS_HAS (flags, NM_CHECKPOINT_CREATE_FLAG_ALLOW_OVERLAPPING)) { + c_list_for_each_entry (checkpoint, &self->checkpoints_lst_head, checkpoints_lst) { + device = nm_checkpoint_includes_devices (checkpoint, (NMDevice *const*) devices->pdata, devices->len); + if (device) { g_set_error (error, NM_MANAGER_ERROR, NM_MANAGER_ERROR_INVALID_ARGUMENTS, "device '%s' is already included in checkpoint %s", nm_device_get_iface (device), @@ -185,9 +192,6 @@ nm_checkpoint_manager_create (NMCheckpointManager *self, checkpoint = nm_checkpoint_new (manager, devices, rollback_timeout, flags); - if (NM_FLAGS_HAS (flags, NM_CHECKPOINT_CREATE_FLAG_DESTROY_ALL)) - nm_checkpoint_manager_destroy_all (self); - nm_dbus_object_export (NM_DBUS_OBJECT (checkpoint)); nm_checkpoint_set_timeout_callback (checkpoint, rollback_timeout_cb, self); @@ -204,7 +208,7 @@ nm_checkpoint_manager_destroy_all (NMCheckpointManager *self) g_return_if_fail (self); while ((checkpoint = c_list_first_entry (&self->checkpoints_lst_head, NMCheckpoint, checkpoints_lst))) - destroy_checkpoint (self, checkpoint); + destroy_checkpoint (self, checkpoint, TRUE); } gboolean @@ -232,7 +236,7 @@ nm_checkpoint_manager_destroy (NMCheckpointManager *self, return FALSE; } - destroy_checkpoint (self, checkpoint); + destroy_checkpoint (self, checkpoint, TRUE); return TRUE; } diff --git a/src/nm-checkpoint.c b/src/nm-checkpoint.c index 55968843e1..29e748bb39 100644 --- a/src/nm-checkpoint.c +++ b/src/nm-checkpoint.c @@ -100,6 +100,12 @@ G_DEFINE_TYPE (NMCheckpoint, nm_checkpoint, NM_TYPE_DBUS_OBJECT) /*****************************************************************************/ +void +nm_checkpoint_log_destroy (NMCheckpoint *self) +{ + _LOGI ("destroy %s", nm_dbus_object_get_path (NM_DBUS_OBJECT (self))); +} + void nm_checkpoint_set_timeout_callback (NMCheckpoint *self, NMCheckpointTimeoutCallback callback, @@ -114,12 +120,33 @@ nm_checkpoint_set_timeout_callback (NMCheckpoint *self, priv->timeout_data = user_data; } -gboolean -nm_checkpoint_includes_device (NMCheckpoint *self, NMDevice *device) +NMDevice * +nm_checkpoint_includes_devices (NMCheckpoint *self, NMDevice *const*devices, guint n_devices) { NMCheckpointPrivate *priv = NM_CHECKPOINT_GET_PRIVATE (self); + guint i; - return g_hash_table_contains (priv->devices, device); + for (i = 0; i < n_devices; i++) { + if (g_hash_table_contains (priv->devices, devices[i])) + return devices[i]; + } + return NULL; +} + +NMDevice * +nm_checkpoint_includes_devices_of (NMCheckpoint *self, NMCheckpoint *cp_for_devices) +{ + NMCheckpointPrivate *priv = NM_CHECKPOINT_GET_PRIVATE (self); + NMCheckpointPrivate *priv2 = NM_CHECKPOINT_GET_PRIVATE (cp_for_devices); + GHashTableIter iter; + NMDevice *device; + + g_hash_table_iter_init (&iter, priv2->devices); + while (g_hash_table_iter_next (&iter, (gpointer *) &device, NULL)) { + if (g_hash_table_contains (priv->devices, device)) + return device; + } + return NULL; } static NMSettingsConnection * diff --git a/src/nm-checkpoint.h b/src/nm-checkpoint.h index 49783ca533..e342650c23 100644 --- a/src/nm-checkpoint.h +++ b/src/nm-checkpoint.h @@ -53,11 +53,15 @@ NMCheckpoint *nm_checkpoint_new (NMManager *manager, GPtrArray *devices, guint32 typedef void (*NMCheckpointTimeoutCallback) (NMCheckpoint *self, gpointer user_data); +void nm_checkpoint_log_destroy (NMCheckpoint *self); + void nm_checkpoint_set_timeout_callback (NMCheckpoint *self, NMCheckpointTimeoutCallback callback, gpointer user_data); -gboolean nm_checkpoint_includes_device (NMCheckpoint *checkpoint, NMDevice *device); GVariant *nm_checkpoint_rollback (NMCheckpoint *self); +NMDevice *nm_checkpoint_includes_devices (NMCheckpoint *self, NMDevice *const*devices, guint n_devices); +NMDevice *nm_checkpoint_includes_devices_of (NMCheckpoint *self, NMCheckpoint *cp_for_devices); + #endif /* __NETWORKMANAGER_CHECKPOINT_H__ */ From 56500e596483c1e9547a0b92cf995b5c46ca6e24 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 28 Mar 2018 09:05:23 +0200 Subject: [PATCH 19/26] checkpoint: refactor setting error for lookup checkpoint failure This changes the error reason in nm_checkpoint_manager_rollback() from NM_MANAGER_ERROR_FAILED to NM_MANAGER_ERROR_INVALID_ARGUMENTS. --- src/nm-checkpoint-manager.c | 23 +++++++++-------------- src/nm-checkpoint-manager.h | 3 ++- 2 files changed, 11 insertions(+), 15 deletions(-) diff --git a/src/nm-checkpoint-manager.c b/src/nm-checkpoint-manager.c index 173eaf5a48..8d341f56b7 100644 --- a/src/nm-checkpoint-manager.c +++ b/src/nm-checkpoint-manager.c @@ -227,14 +227,9 @@ nm_checkpoint_manager_destroy (NMCheckpointManager *self, return TRUE; } - checkpoint = nm_checkpoint_manager_lookup_by_path (self, path); - if (!checkpoint) { - g_set_error (error, - NM_MANAGER_ERROR, - NM_MANAGER_ERROR_INVALID_ARGUMENTS, - "checkpoint %s does not exist", path); + checkpoint = nm_checkpoint_manager_lookup_by_path (self, path, error); + if (!checkpoint) return FALSE; - } destroy_checkpoint (self, checkpoint, TRUE); return TRUE; @@ -253,19 +248,16 @@ nm_checkpoint_manager_rollback (NMCheckpointManager *self, g_return_val_if_fail (results, FALSE); g_return_val_if_fail (!error || !*error, FALSE); - checkpoint = nm_checkpoint_manager_lookup_by_path (self, path); - if (!checkpoint) { - g_set_error (error, NM_MANAGER_ERROR, NM_MANAGER_ERROR_FAILED, - "checkpoint %s does not exist", path); + checkpoint = nm_checkpoint_manager_lookup_by_path (self, path, error); + if (!checkpoint) return FALSE; - } *results = rollback_checkpoint (self, checkpoint); return TRUE; } NMCheckpoint * -nm_checkpoint_manager_lookup_by_path (NMCheckpointManager *self, const char *path) +nm_checkpoint_manager_lookup_by_path (NMCheckpointManager *self, const char *path, GError **error) { NMCheckpoint *checkpoint; @@ -274,8 +266,11 @@ nm_checkpoint_manager_lookup_by_path (NMCheckpointManager *self, const char *pat checkpoint = (NMCheckpoint *) nm_dbus_manager_lookup_object (nm_dbus_object_get_manager (NM_DBUS_OBJECT (GET_MANAGER (self))), path); if ( !checkpoint - || !NM_IS_CHECKPOINT (checkpoint)) + || !NM_IS_CHECKPOINT (checkpoint)) { + g_set_error (error, NM_MANAGER_ERROR, NM_MANAGER_ERROR_INVALID_ARGUMENTS, + "checkpoint %s does not exist", path); return NULL; + } nm_assert (c_list_contains (&self->checkpoints_lst_head, &checkpoint->checkpoints_lst)); return checkpoint; diff --git a/src/nm-checkpoint-manager.h b/src/nm-checkpoint-manager.h index bcdf0d3602..158d670a12 100644 --- a/src/nm-checkpoint-manager.h +++ b/src/nm-checkpoint-manager.h @@ -32,7 +32,8 @@ NMCheckpointManager *nm_checkpoint_manager_new (NMManager *manager, GParamSpec * void nm_checkpoint_manager_free (NMCheckpointManager *self); NMCheckpoint *nm_checkpoint_manager_lookup_by_path (NMCheckpointManager *self, - const char *path); + const char *path, + GError **error); NMCheckpoint *nm_checkpoint_manager_create (NMCheckpointManager *self, const char *const*device_names, From ab8312a18e8b281a62f39ce59a077fda77943187 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 28 Mar 2018 17:24:30 +0200 Subject: [PATCH 20/26] checkpoint: generate GIR information for NMCheckpointCreateFlags Note that this changes API for checkpoint_create_async() in Python via GIR. Previously it would require an integer argument, now a flags argument. But this API is still unstable, it will be introduced with 1.12. --- examples/python/gi/checkpoint.py | 1 + libnm-core/nm-dbus-interface.h | 4 ++-- libnm/libnm.ver | 1 + 3 files changed, 4 insertions(+), 2 deletions(-) diff --git a/examples/python/gi/checkpoint.py b/examples/python/gi/checkpoint.py index 513e6c3a0a..de9468ea8f 100644 --- a/examples/python/gi/checkpoint.py +++ b/examples/python/gi/checkpoint.py @@ -48,6 +48,7 @@ def do_create(client): sys.exit("Unknown device %s" % arg) devices.append(d) + # FIXME: flags are not a plain integer. client.checkpoint_create_async(devices, timeout, 0, None, create_cb, None) def destroy_cb(client, result, data): diff --git a/libnm-core/nm-dbus-interface.h b/libnm-core/nm-dbus-interface.h index 0a9c41916d..ec9c7f1aee 100644 --- a/libnm-core/nm-dbus-interface.h +++ b/libnm-core/nm-dbus-interface.h @@ -857,9 +857,9 @@ typedef enum { * * The flags for CheckpointCreate call * - * Since: 1.4 + * Since: 1.4 (gi flags generated since 1.12) */ -typedef enum { /*< skip >*/ +typedef enum { /*< flags >*/ NM_CHECKPOINT_CREATE_FLAG_NONE = 0, NM_CHECKPOINT_CREATE_FLAG_DESTROY_ALL = 0x01, NM_CHECKPOINT_CREATE_FLAG_DELETE_NEW_CONNECTIONS = 0x02, diff --git a/libnm/libnm.ver b/libnm/libnm.ver index b475dcf7ee..b99d935807 100644 --- a/libnm/libnm.ver +++ b/libnm/libnm.ver @@ -1334,6 +1334,7 @@ global: libnm_1_12_0 { global: + nm_checkpoint_create_flags_get_type; nm_checkpoint_get_created; nm_checkpoint_get_devices; nm_checkpoint_get_rollback_timeout; From f67303221bdc8098cb3b6203ec777b294e412971 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 28 Mar 2018 08:09:56 +0200 Subject: [PATCH 21/26] checkpoint: allow resetting the rollback timeout via D-Bus This allows to adjust the timeout of an existing checkpoint. The main usecase of checkpoints, is to have a fail-safe when configuring the network remotely. By allowing to reset the timeout, the user can perform a series of actions, and keep bumping the timeout. That way, the entire series is still guarded by the same checkpoint, but the user can start with short timeout, and re-adjust the timeout as he goes along. The libnm API only implements the async form (at least for now). Sync methods are fundamentally wrong with D-Bus, and it's probably not needed. Also, follow glib convenction, where the async form doesn't have the _async name suffix. Also, accept a D-Bus path as argument, not a NMCheckpoint instance. The libnm API should not be more restricted than the underlying D-Bus API. It would be cumbersome to require the user to lookup the NMCheckpoint instance first, especially since libnm doesn't provide an efficient or convenient lookup-by-path method. On the other hand, retrieving the path from a NMCheckpoint instance is always possible. --- .../org.freedesktop.NetworkManager.xml | 21 +++++ libnm/libnm.ver | 2 + libnm/nm-client.c | 82 +++++++++++++++++++ libnm/nm-client.h | 13 +++ libnm/nm-manager.c | 60 ++++++++++++++ libnm/nm-manager.h | 9 ++ src/nm-audit-manager.h | 1 + src/nm-checkpoint-manager.c | 20 +++++ src/nm-checkpoint-manager.h | 5 ++ src/nm-checkpoint.c | 38 ++++++++- src/nm-checkpoint.h | 2 + src/nm-manager.c | 54 +++++++++++- 12 files changed, 303 insertions(+), 4 deletions(-) diff --git a/introspection/org.freedesktop.NetworkManager.xml b/introspection/org.freedesktop.NetworkManager.xml index 26a618c1bf..b8ad5911f2 100644 --- a/introspection/org.freedesktop.NetworkManager.xml +++ b/introspection/org.freedesktop.NetworkManager.xml @@ -251,6 +251,27 @@ + + + + + +