From de61722efe8a43c282004d4dcb8293fed037368a Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 22 Feb 2022 21:13:58 +0100 Subject: [PATCH 1/7] core: add nm_dbus_manager_lookup_object_with_type() helper This makes the non-obvious fact clearer, that when you look up an object by an untrusted, user-provided path, it might not be the object type you are looking for. In basically all cases, you need to check that the result is of the expected type. This helper makes that clearer. --- src/core/nm-dbus-manager.c | 15 +++++++++++++++ src/core/nm-dbus-manager.h | 3 +++ 2 files changed, 18 insertions(+) diff --git a/src/core/nm-dbus-manager.c b/src/core/nm-dbus-manager.c index 2c5f7acee8..7fcbf6cad8 100644 --- a/src/core/nm-dbus-manager.c +++ b/src/core/nm-dbus-manager.c @@ -1082,6 +1082,21 @@ nm_dbus_manager_lookup_object(NMDBusManager *self, const char *path) return obj; } +gpointer +nm_dbus_manager_lookup_object_with_type(NMDBusManager *self, GType gtype, const char *path) +{ + gpointer ptr; + + nm_assert(g_type_is_a(gtype, NM_TYPE_DBUS_OBJECT)); + nm_assert(gtype != NM_TYPE_DBUS_OBJECT); + + ptr = nm_dbus_manager_lookup_object(self, path); + if (!ptr || !G_TYPE_CHECK_INSTANCE_TYPE(ptr, gtype)) + return NULL; + + return ptr; +} + void _nm_dbus_manager_obj_export(NMDBusObject *obj) { diff --git a/src/core/nm-dbus-manager.h b/src/core/nm-dbus-manager.h index d7ca78d174..d977f99ffd 100644 --- a/src/core/nm-dbus-manager.h +++ b/src/core/nm-dbus-manager.h @@ -52,6 +52,9 @@ gboolean nm_dbus_manager_is_stopping(NMDBusManager *self); gpointer nm_dbus_manager_lookup_object(NMDBusManager *self, const char *path); +gpointer +nm_dbus_manager_lookup_object_with_type(NMDBusManager *self, GType gtype, const char *path); + void _nm_dbus_manager_obj_export(NMDBusObject *obj); void _nm_dbus_manager_obj_unexport(NMDBusObject *obj); void From 6f948fcd2e651a3511f5d9d9f0e75581dcc33a6f Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 22 Feb 2022 21:19:18 +0100 Subject: [PATCH 2/7] core: use nm_dbus_manager_lookup_object_with_type() I think this makes it clearer that we should always look for a certain type, because NMDBusManager tracks all D-Bus objects. --- src/core/devices/wifi/nm-wifi-ap.c | 7 ++++--- src/core/devices/wifi/nm-wifi-p2p-peer.c | 9 +++++---- src/core/nm-checkpoint-manager.c | 9 +++++---- src/core/nm-manager.c | 13 +++++++++---- src/core/settings/nm-settings.c | 6 ++++-- 5 files changed, 27 insertions(+), 17 deletions(-) diff --git a/src/core/devices/wifi/nm-wifi-ap.c b/src/core/devices/wifi/nm-wifi-ap.c index ac0b748a51..825a5aa8f1 100644 --- a/src/core/devices/wifi/nm-wifi-ap.c +++ b/src/core/devices/wifi/nm-wifi-ap.c @@ -1037,9 +1037,10 @@ nm_wifi_ap_lookup_for_device(NMDevice *device, const char *exported_path) g_return_val_if_fail(NM_IS_DEVICE(device), NULL); - ap = nm_dbus_manager_lookup_object(nm_dbus_object_get_manager(NM_DBUS_OBJECT(device)), - exported_path); - if (!ap || !NM_IS_WIFI_AP(ap) || ap->wifi_device != device) + ap = nm_dbus_manager_lookup_object_with_type(nm_dbus_object_get_manager(NM_DBUS_OBJECT(device)), + NM_TYPE_WIFI_AP, + exported_path); + if (!ap || ap->wifi_device != device) return NULL; return ap; diff --git a/src/core/devices/wifi/nm-wifi-p2p-peer.c b/src/core/devices/wifi/nm-wifi-p2p-peer.c index 0a17427067..8ccb2a4d5e 100644 --- a/src/core/devices/wifi/nm-wifi-p2p-peer.c +++ b/src/core/devices/wifi/nm-wifi-p2p-peer.c @@ -139,10 +139,11 @@ nm_wifi_p2p_peer_lookup_for_device(NMDevice *device, const char *exported_path) g_return_val_if_fail(NM_IS_DEVICE(device), NULL); - peer = (NMWifiP2PPeer *) nm_dbus_manager_lookup_object( - nm_dbus_object_get_manager(NM_DBUS_OBJECT(device)), - exported_path); - if (!peer || !NM_IS_WIFI_P2P_PEER(peer) || peer->wifi_device != device) + peer = + nm_dbus_manager_lookup_object_with_type(nm_dbus_object_get_manager(NM_DBUS_OBJECT(device)), + NM_TYPE_WIFI_P2P_PEER, + exported_path); + if (!peer || peer->wifi_device != device) return NULL; return peer; diff --git a/src/core/nm-checkpoint-manager.c b/src/core/nm-checkpoint-manager.c index 4a3e7aafba..6c9ea2a634 100644 --- a/src/core/nm-checkpoint-manager.c +++ b/src/core/nm-checkpoint-manager.c @@ -259,10 +259,11 @@ nm_checkpoint_manager_lookup_by_path(NMCheckpointManager *self, const char *path g_return_val_if_fail(self, NULL); - checkpoint = - nm_dbus_manager_lookup_object(nm_dbus_object_get_manager(NM_DBUS_OBJECT(GET_MANAGER(self))), - path); - if (!checkpoint || !NM_IS_CHECKPOINT(checkpoint)) { + checkpoint = nm_dbus_manager_lookup_object_with_type( + nm_dbus_object_get_manager(NM_DBUS_OBJECT(GET_MANAGER(self))), + NM_TYPE_CHECKPOINT, + path); + if (!checkpoint) { g_set_error(error, NM_MANAGER_ERROR, NM_MANAGER_ERROR_INVALID_ARGUMENTS, diff --git a/src/core/nm-manager.c b/src/core/nm-manager.c index c2a8f61350..88488befa0 100644 --- a/src/core/nm-manager.c +++ b/src/core/nm-manager.c @@ -1159,8 +1159,10 @@ active_connection_get_by_path(NMManager *self, const char *path) NMManagerPrivate *priv = NM_MANAGER_GET_PRIVATE(self); NMActiveConnection *ac; - ac = nm_dbus_manager_lookup_object(nm_dbus_object_get_manager(NM_DBUS_OBJECT(self)), path); - if (!ac || !NM_IS_ACTIVE_CONNECTION(ac) || c_list_is_empty(&ac->active_connections_lst)) + ac = nm_dbus_manager_lookup_object_with_type(nm_dbus_object_get_manager(NM_DBUS_OBJECT(self)), + NM_TYPE_ACTIVE_CONNECTION, + path); + if (!ac || c_list_is_empty(&ac->active_connections_lst)) return NULL; nm_assert(c_list_contains(&priv->active_connections_lst_head, &ac->active_connections_lst)); @@ -1292,8 +1294,11 @@ nm_manager_get_device_by_path(NMManager *self, const char *path) g_return_val_if_fail(path, NULL); - device = nm_dbus_manager_lookup_object(nm_dbus_object_get_manager(NM_DBUS_OBJECT(self)), path); - if (!device || !NM_IS_DEVICE(device) || c_list_is_empty(&device->devices_lst)) + device = + nm_dbus_manager_lookup_object_with_type(nm_dbus_object_get_manager(NM_DBUS_OBJECT(self)), + NM_TYPE_DEVICE, + path); + if (!device || c_list_is_empty(&device->devices_lst)) return NULL; nm_assert(c_list_contains(&priv->devices_lst_head, &device->devices_lst)); diff --git a/src/core/settings/nm-settings.c b/src/core/settings/nm-settings.c index 6619d3e0a2..6589291131 100644 --- a/src/core/settings/nm-settings.c +++ b/src/core/settings/nm-settings.c @@ -3201,8 +3201,10 @@ nm_settings_get_connection_by_path(NMSettings *self, const char *path) priv = NM_SETTINGS_GET_PRIVATE(self); connection = - nm_dbus_manager_lookup_object(nm_dbus_object_get_manager(NM_DBUS_OBJECT(self)), path); - if (!connection || !NM_IS_SETTINGS_CONNECTION(connection)) + nm_dbus_manager_lookup_object_with_type(nm_dbus_object_get_manager(NM_DBUS_OBJECT(self)), + NM_TYPE_SETTINGS_CONNECTION, + path); + if (!connection) return NULL; nm_assert(c_list_contains(&priv->connections_lst_head, &connection->_connections_lst)); From 53406e721da77175a970534b0f6c28405a0602e9 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 1 Mar 2022 13:24:27 +0100 Subject: [PATCH 3/7] core/trivial: rename parameter to link_changed_cb() "info" is not a good name. Variables of this kind are usually called "plink" or "pllink". Rename. --- src/core/devices/nm-device.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/core/devices/nm-device.c b/src/core/devices/nm-device.c index aa4c99782c..b96f8e7294 100644 --- a/src/core/devices/nm-device.c +++ b/src/core/devices/nm-device.c @@ -6641,7 +6641,7 @@ static void link_changed_cb(NMPlatform *platform, int obj_type_i, int ifindex, - NMPlatformLink *info, + NMPlatformLink *pllink, int change_type_i, NMDevice *self) { @@ -6654,7 +6654,7 @@ link_changed_cb(NMPlatform *platform, priv = NM_DEVICE_GET_PRIVATE(self); if (ifindex == nm_device_get_ifindex(self)) { - if (!(info->n_ifi_flags & IFF_UP)) + if (!(pllink->n_ifi_flags & IFF_UP)) priv->device_link_changed_down = TRUE; if (!priv->device_link_changed_id) { priv->device_link_changed_id = g_idle_add(device_link_changed, self); From a566fd8cf40fb3c6ef18bdf929a9983d64020363 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 22 Feb 2022 10:08:30 +0100 Subject: [PATCH 4/7] core/device: use c_list_for_each_entry() for interating of slaves list This convenience macro is just shorter, resulting in more(?) readable code and less clutter. --- src/core/devices/nm-device.c | 26 +++++++++----------------- 1 file changed, 9 insertions(+), 17 deletions(-) diff --git a/src/core/devices/nm-device.c b/src/core/devices/nm-device.c index b96f8e7294..17acfabb3e 100644 --- a/src/core/devices/nm-device.c +++ b/src/core/devices/nm-device.c @@ -5878,11 +5878,9 @@ static SlaveInfo * find_slave_info(NMDevice *self, NMDevice *slave) { NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE(self); - CList *iter; SlaveInfo *info; - c_list_for_each (iter, &priv->slaves) { - info = c_list_entry(iter, SlaveInfo, lst_slave); + c_list_for_each_entry (info, &priv->slaves, lst_slave) { if (info->slave == slave) return info; } @@ -7627,14 +7625,12 @@ nm_device_master_check_slave_physical_port(NMDevice *self, NMDevice *slave, NMLo NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE(self); const char *slave_physical_port_id, *existing_physical_port_id; SlaveInfo *info; - CList *iter; slave_physical_port_id = nm_device_get_physical_port_id(slave); if (!slave_physical_port_id) return; - c_list_for_each (iter, &priv->slaves) { - info = c_list_entry(iter, SlaveInfo, lst_slave); + c_list_for_each_entry (info, &priv->slaves, lst_slave) { if (info->slave == slave) continue; @@ -7660,7 +7656,8 @@ nm_device_master_release_slaves(NMDevice *self) { NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE(self); NMDeviceStateReason reason; - CList *iter, *safe; + SlaveInfo *info; + SlaveInfo *safe; /* Don't release the slaves if this connection doesn't belong to NM. */ if (nm_device_sys_iface_state_is_external(self)) @@ -7670,9 +7667,7 @@ nm_device_master_release_slaves(NMDevice *self) if (priv->state == NM_DEVICE_STATE_FAILED) reason = NM_DEVICE_STATE_REASON_DEPENDENCY_FAILED; - c_list_for_each_safe (iter, safe, &priv->slaves) { - SlaveInfo *info = c_list_entry(iter, SlaveInfo, lst_slave); - + c_list_for_each_entry_safe (info, safe, &priv->slaves, lst_slave) { if (priv->activation_state_preserve_external_ports && nm_device_sys_iface_state_is_external(info->slave)) { _LOGT(LOGD_DEVICE, @@ -9437,7 +9432,7 @@ activate_stage2_device_config(NMDevice *self) NMActStageReturn ret; NMSettingWired *s_wired; gboolean no_firmware = FALSE; - CList *iter; + SlaveInfo *info; NMTernary accept_all_mac_addresses; nm_device_state_changed(self, NM_DEVICE_STATE_CONFIG, NM_DEVICE_STATE_REASON_NONE); @@ -9484,8 +9479,7 @@ activate_stage2_device_config(NMDevice *self) } /* If we have slaves that aren't yet enslaved, do that now */ - c_list_for_each (iter, &priv->slaves) { - SlaveInfo *info = c_list_entry(iter, SlaveInfo, lst_slave); + c_list_for_each_entry (info, &priv->slaves, lst_slave) { NMDeviceState slave_state = nm_device_get_state(info->slave); if (slave_state == NM_DEVICE_STATE_IP_CONFIG) @@ -10413,14 +10407,12 @@ have_any_ready_slaves(NMDevice *self) { NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE(self); SlaveInfo *info; - CList *iter; /* Any enslaved slave is "ready" in the generic case as it's - * at least >= NM_DEVCIE_STATE_IP_CONFIG and has had Layer 2 + * at least >= NM_DEVICE_STATE_IP_CONFIG and has had Layer 2 * properties set up. */ - c_list_for_each (iter, &priv->slaves) { - info = c_list_entry(iter, SlaveInfo, lst_slave); + c_list_for_each_entry (info, &priv->slaves, lst_slave) { if (NM_DEVICE_GET_PRIVATE(info->slave)->is_enslaved) return TRUE; } From 17ac71cd58305d31dfc19611e20907b6cf4a7ba5 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 22 Feb 2022 20:32:49 +0100 Subject: [PATCH 5/7] core/device: add ReleaseSlaveType argument for nm_device_master_release_one_slave() I find the two (dependent) booleans "configure" and "force" confusing. nm_device_master_release_one_slave() has many callers, it's interesting to be able to grep for the release-type. Add an enum to make this more readable. --- src/core/devices/nm-device.c | 55 ++++++++++++++++++++++-------------- 1 file changed, 34 insertions(+), 21 deletions(-) diff --git a/src/core/devices/nm-device.c b/src/core/devices/nm-device.c index 17acfabb3e..f6dda826e7 100644 --- a/src/core/devices/nm-device.c +++ b/src/core/devices/nm-device.c @@ -100,6 +100,12 @@ typedef void (*ActivationHandleFunc)(NMDevice *self); +typedef enum { + RELEASE_SLAVE_TYPE_NO_CONFIG, + RELEASE_SLAVE_TYPE_CONFIG, + RELEASE_SLAVE_TYPE_CONFIG_FORCE, +} ReleaseSlaveType; + typedef enum { CLEANUP_TYPE_KEEP, CLEANUP_TYPE_REMOVED, @@ -5952,8 +5958,8 @@ nm_device_master_enslave_slave(NMDevice *self, NMDevice *slave, NMConnection *co * @self: the master device * @slave: the slave device to release * @configure: whether @self needs to actually release @slave - * @force: force the release of @slave even if it wasn't added - * to @master by NetworkManager + * @release_type: whether @self needs to actually release slave + * and whether that is forced. * @reason: the state change reason for the @slave * * If @self is capable of enslaving other devices (ie it's a bridge, bond, team, @@ -5963,8 +5969,7 @@ nm_device_master_enslave_slave(NMDevice *self, NMDevice *slave, NMConnection *co static void nm_device_master_release_one_slave(NMDevice *self, NMDevice *slave, - gboolean configure, - gboolean force, + ReleaseSlaveType release_type, NMDeviceStateReason reason) { NMDevicePrivate *priv; @@ -5974,7 +5979,10 @@ nm_device_master_release_one_slave(NMDevice *self, g_return_if_fail(NM_DEVICE(self)); g_return_if_fail(NM_DEVICE(slave)); - g_return_if_fail(!force || configure); + nm_assert(NM_IN_SET(release_type, + RELEASE_SLAVE_TYPE_NO_CONFIG, + RELEASE_SLAVE_TYPE_CONFIG, + RELEASE_SLAVE_TYPE_CONFIG_FORCE)); g_return_if_fail(NM_DEVICE_GET_CLASS(self)->release_slave != NULL); info = find_slave_info(self, slave); @@ -5984,7 +5992,9 @@ nm_device_master_release_one_slave(NMDevice *self, slave, nm_device_get_iface(slave), !info ? "(not registered)" : (info->slave_is_enslaved ? "(enslaved)" : "(not enslaved)"), - force ? " (force-configure)" : (configure ? " (configure)" : "")); + release_type == RELEASE_SLAVE_TYPE_CONFIG_FORCE + ? " (force-configure)" + : (release_type == RELEASE_SLAVE_TYPE_CONFIG ? " (configure)" : "(no-config)")); if (!info) g_return_if_reached(); @@ -5996,8 +6006,11 @@ nm_device_master_release_one_slave(NMDevice *self, nm_assert(slave == info->slave); /* first, let subclasses handle the release ... */ - if (info->slave_is_enslaved || nm_device_sys_iface_state_is_external(slave) || force) - NM_DEVICE_GET_CLASS(self)->release_slave(self, slave, configure); + if (info->slave_is_enslaved || nm_device_sys_iface_state_is_external(slave) + || release_type >= RELEASE_SLAVE_TYPE_CONFIG_FORCE) + NM_DEVICE_GET_CLASS(self)->release_slave(self, + slave, + release_type >= RELEASE_SLAVE_TYPE_CONFIG); /* raise notifications about the release, including clearing is_enslaved. */ nm_device_slave_notify_release(slave, reason); @@ -6333,8 +6346,7 @@ device_recheck_slave_status(NMDevice *self, const NMPlatformLink *plink) nm_device_master_release_one_slave(priv->master, self, - FALSE, - FALSE, + RELEASE_SLAVE_TYPE_NO_CONFIG, NM_DEVICE_STATE_REASON_CONNECTION_ASSUMED); } @@ -7528,7 +7540,11 @@ slave_state_changed(NMDevice *slave, configure = priv->sys_iface_state == NM_DEVICE_SYS_IFACE_STATE_MANAGED && nm_device_sys_iface_state_get(slave) != NM_DEVICE_SYS_IFACE_STATE_EXTERNAL; - nm_device_master_release_one_slave(self, slave, configure, FALSE, reason); + nm_device_master_release_one_slave(self, + slave, + configure ? RELEASE_SLAVE_TYPE_CONFIG + : RELEASE_SLAVE_TYPE_NO_CONFIG, + reason); /* Bridge/bond/team interfaces are left up until manually deactivated */ if (c_list_is_empty(&priv->slaves) && priv->state == NM_DEVICE_STATE_ACTIVATED) _LOGD(LOGD_DEVICE, "last slave removed; remaining activated"); @@ -7675,7 +7691,7 @@ nm_device_master_release_slaves(NMDevice *self) nm_device_get_iface(info->slave)); continue; } - nm_device_master_release_one_slave(self, info->slave, TRUE, FALSE, reason); + nm_device_master_release_one_slave(self, info->slave, RELEASE_SLAVE_TYPE_CONFIG, reason); } /* We only need this flag for a short time. It served its purpose. Clear @@ -7852,8 +7868,7 @@ nm_device_removed(NMDevice *self, gboolean unconfigure_ip_config) * Release the slave from master, but don't touch the device. */ nm_device_master_release_one_slave(priv->master, self, - FALSE, - FALSE, + RELEASE_SLAVE_TYPE_NO_CONFIG, NM_DEVICE_STATE_REASON_CONNECTION_ASSUMED); } @@ -8928,8 +8943,7 @@ master_ready(NMDevice *self, NMActiveConnection *active) if (priv->master && priv->master != master) nm_device_master_release_one_slave(priv->master, self, - FALSE, - FALSE, + RELEASE_SLAVE_TYPE_NO_CONFIG, NM_DEVICE_STATE_REASON_CONNECTION_ASSUMED); /* If the master didn't change, add-slave only rechecks whether to assume a connection. */ @@ -9214,8 +9228,7 @@ activate_stage1_device_prepare(NMDevice *self) else if (priv->master) { nm_device_master_release_one_slave(priv->master, self, - TRUE, - TRUE, + RELEASE_SLAVE_TYPE_CONFIG_FORCE, NM_DEVICE_STATE_REASON_CONNECTION_ASSUMED); } @@ -15104,12 +15117,12 @@ nm_device_cleanup(NMDevice *self, NMDeviceStateReason reason, CleanupType cleanu /* slave: mark no longer enslaved */ if (priv->master && priv->ifindex > 0 - && nm_platform_link_get_master(nm_device_get_platform(self), priv->ifindex) <= 0) + && nm_platform_link_get_master(nm_device_get_platform(self), priv->ifindex) <= 0) { nm_device_master_release_one_slave(priv->master, self, - FALSE, - FALSE, + RELEASE_SLAVE_TYPE_NO_CONFIG, NM_DEVICE_STATE_REASON_CONNECTION_ASSUMED); + } lldp_setup(self, NM_TERNARY_FALSE); From 4629506b808fca3a7561e7fbd341c06e36477ac6 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 23 Feb 2022 21:11:04 +0100 Subject: [PATCH 6/7] core/device: rename nm_device_master_release_one_slave() We have nm_device_master_add_slave(). This should be mirrored by nm_device_master_release_slave() (not release-one-slave). Thereby, also rename nm_device_master_release_slaves() to nm_device_master_release_slaves_all() to make it clearer. --- src/core/devices/nm-device-bond.c | 2 +- src/core/devices/nm-device-private.h | 2 +- src/core/devices/nm-device.c | 69 ++++++++++++++-------------- 3 files changed, 36 insertions(+), 37 deletions(-) diff --git a/src/core/devices/nm-device-bond.c b/src/core/devices/nm-device-bond.c index f19f70b676..16896d5753 100644 --- a/src/core/devices/nm-device-bond.c +++ b/src/core/devices/nm-device-bond.c @@ -373,7 +373,7 @@ apply_bonding_config(NMDeviceBond *self) NM_SETTING_BOND_OPTION_MODE); /* Need to release all slaves before we can change bond mode */ if (!nm_streq0(device_bond_mode, mode_str)) - nm_device_master_release_slaves(device); + nm_device_master_release_slaves_all(device); set_bond_attr_or_default(device, s_bond, NM_SETTING_BOND_OPTION_MODE); diff --git a/src/core/devices/nm-device-private.h b/src/core/devices/nm-device-private.h index 790bb8238b..b54aed6a75 100644 --- a/src/core/devices/nm-device-private.h +++ b/src/core/devices/nm-device-private.h @@ -61,7 +61,7 @@ void nm_device_recheck_available_connections(NMDevice *device); void nm_device_master_check_slave_physical_port(NMDevice *self, NMDevice *slave, NMLogDomain log_domain); -void nm_device_master_release_slaves(NMDevice *self); +void nm_device_master_release_slaves_all(NMDevice *self); void nm_device_set_carrier(NMDevice *self, gboolean carrier); diff --git a/src/core/devices/nm-device.c b/src/core/devices/nm-device.c index f6dda826e7..a2e513d7e5 100644 --- a/src/core/devices/nm-device.c +++ b/src/core/devices/nm-device.c @@ -5954,7 +5954,7 @@ nm_device_master_enslave_slave(NMDevice *self, NMDevice *slave, NMConnection *co } /** - * nm_device_master_release_one_slave: + * nm_device_master_release_slave: * @self: the master device * @slave: the slave device to release * @configure: whether @self needs to actually release @slave @@ -5967,10 +5967,10 @@ nm_device_master_enslave_slave(NMDevice *self, NMDevice *slave, NMConnection *co * updates the state of @self and @slave to reflect its release. */ static void -nm_device_master_release_one_slave(NMDevice *self, - NMDevice *slave, - ReleaseSlaveType release_type, - NMDeviceStateReason reason) +nm_device_master_release_slave(NMDevice *self, + NMDevice *slave, + ReleaseSlaveType release_type, + NMDeviceStateReason reason) { NMDevicePrivate *priv; NMDevicePrivate *slave_priv; @@ -6344,10 +6344,10 @@ device_recheck_slave_status(NMDevice *self, const NMPlatformLink *plink) goto out; } - nm_device_master_release_one_slave(priv->master, - self, - RELEASE_SLAVE_TYPE_NO_CONFIG, - NM_DEVICE_STATE_REASON_CONNECTION_ASSUMED); + nm_device_master_release_slave(priv->master, + self, + RELEASE_SLAVE_TYPE_NO_CONFIG, + NM_DEVICE_STATE_REASON_CONNECTION_ASSUMED); } if (master && NM_DEVICE_GET_CLASS(master)->enslave_slave) { @@ -7540,11 +7540,11 @@ slave_state_changed(NMDevice *slave, configure = priv->sys_iface_state == NM_DEVICE_SYS_IFACE_STATE_MANAGED && nm_device_sys_iface_state_get(slave) != NM_DEVICE_SYS_IFACE_STATE_EXTERNAL; - nm_device_master_release_one_slave(self, - slave, - configure ? RELEASE_SLAVE_TYPE_CONFIG - : RELEASE_SLAVE_TYPE_NO_CONFIG, - reason); + nm_device_master_release_slave(self, + slave, + configure ? RELEASE_SLAVE_TYPE_CONFIG + : RELEASE_SLAVE_TYPE_NO_CONFIG, + reason); /* Bridge/bond/team interfaces are left up until manually deactivated */ if (c_list_is_empty(&priv->slaves) && priv->state == NM_DEVICE_STATE_ACTIVATED) _LOGD(LOGD_DEVICE, "last slave removed; remaining activated"); @@ -7666,9 +7666,8 @@ nm_device_master_check_slave_physical_port(NMDevice *self, NMDevice *slave, NMLo } } -/* release all slaves */ void -nm_device_master_release_slaves(NMDevice *self) +nm_device_master_release_slaves_all(NMDevice *self) { NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE(self); NMDeviceStateReason reason; @@ -7691,7 +7690,7 @@ nm_device_master_release_slaves(NMDevice *self) nm_device_get_iface(info->slave)); continue; } - nm_device_master_release_one_slave(self, info->slave, RELEASE_SLAVE_TYPE_CONFIG, reason); + nm_device_master_release_slave(self, info->slave, RELEASE_SLAVE_TYPE_CONFIG, reason); } /* We only need this flag for a short time. It served its purpose. Clear @@ -7866,10 +7865,10 @@ nm_device_removed(NMDevice *self, gboolean unconfigure_ip_config) if (priv->master) { /* this is called when something externally messes with the slave or during shut-down. * Release the slave from master, but don't touch the device. */ - nm_device_master_release_one_slave(priv->master, - self, - RELEASE_SLAVE_TYPE_NO_CONFIG, - NM_DEVICE_STATE_REASON_CONNECTION_ASSUMED); + nm_device_master_release_slave(priv->master, + self, + RELEASE_SLAVE_TYPE_NO_CONFIG, + NM_DEVICE_STATE_REASON_CONNECTION_ASSUMED); } _dev_l3_register_l3cds(self, priv->l3cfg, FALSE, unconfigure_ip_config); @@ -8941,10 +8940,10 @@ master_ready(NMDevice *self, NMActiveConnection *active) _LOGD(LOGD_DEVICE, "master connection ready; master device %s", nm_device_get_iface(master)); if (priv->master && priv->master != master) - nm_device_master_release_one_slave(priv->master, - self, - RELEASE_SLAVE_TYPE_NO_CONFIG, - NM_DEVICE_STATE_REASON_CONNECTION_ASSUMED); + nm_device_master_release_slave(priv->master, + self, + RELEASE_SLAVE_TYPE_NO_CONFIG, + NM_DEVICE_STATE_REASON_CONNECTION_ASSUMED); /* If the master didn't change, add-slave only rechecks whether to assume a connection. */ nm_device_master_add_slave(master, @@ -9226,10 +9225,10 @@ activate_stage1_device_prepare(NMDevice *self) if (master) master_ready(self, active); else if (priv->master) { - nm_device_master_release_one_slave(priv->master, - self, - RELEASE_SLAVE_TYPE_CONFIG_FORCE, - NM_DEVICE_STATE_REASON_CONNECTION_ASSUMED); + nm_device_master_release_slave(priv->master, + self, + RELEASE_SLAVE_TYPE_CONFIG_FORCE, + NM_DEVICE_STATE_REASON_CONNECTION_ASSUMED); } nm_device_activate_schedule_stage2_device_config(self, TRUE); @@ -15093,7 +15092,7 @@ nm_device_cleanup(NMDevice *self, NMDeviceStateReason reason, CleanupType cleanu if (cleanup_type == CLEANUP_TYPE_DECONFIGURE) { /* master: release slaves */ - nm_device_master_release_slaves(self); + nm_device_master_release_slaves_all(self); /* Take out any entries in the routing table and any IP address the device had. */ if (ifindex > 0) { @@ -15118,10 +15117,10 @@ nm_device_cleanup(NMDevice *self, NMDeviceStateReason reason, CleanupType cleanu /* slave: mark no longer enslaved */ if (priv->master && priv->ifindex > 0 && nm_platform_link_get_master(nm_device_get_platform(self), priv->ifindex) <= 0) { - nm_device_master_release_one_slave(priv->master, - self, - RELEASE_SLAVE_TYPE_NO_CONFIG, - NM_DEVICE_STATE_REASON_CONNECTION_ASSUMED); + nm_device_master_release_slave(priv->master, + self, + RELEASE_SLAVE_TYPE_NO_CONFIG, + NM_DEVICE_STATE_REASON_CONNECTION_ASSUMED); } lldp_setup(self, NM_TERNARY_FALSE); @@ -15622,7 +15621,7 @@ _set_state_full(NMDevice *self, NMDeviceState state, NMDeviceStateReason reason, sett_conn ? nm_settings_connection_get_id(sett_conn) : ""); /* Notify any slaves of the unexpected failure */ - nm_device_master_release_slaves(self); + nm_device_master_release_slaves_all(self); /* If the connection doesn't yet have a timestamp, set it to zero so that * we can distinguish between connections we've tried to activate and have From 8aaee8c50c0631653abcabaad291f04c9441facf Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 1 Mar 2022 12:11:11 +0100 Subject: [PATCH 7/7] core/device: avoid logging "%p" format and use obfuscated ptr --- src/core/devices/nm-device.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/core/devices/nm-device.c b/src/core/devices/nm-device.c index a2e513d7e5..47daa92e81 100644 --- a/src/core/devices/nm-device.c +++ b/src/core/devices/nm-device.c @@ -4459,9 +4459,9 @@ _parent_set_ifindex(NMDevice *self, int parent_ifindex, gboolean force_check) _LOGD(LOGD_DEVICE, "parent: ifindex %d, no device", priv->parent_ifindex); else { _LOGD(LOGD_DEVICE, - "parent: ifindex %d, device %p, %s", + "parent: ifindex %d, device " NM_HASH_OBFUSCATE_PTR_FMT ", %s", priv->parent_ifindex, - priv->parent_device.obj, + NM_HASH_OBFUSCATE_PTR(priv->parent_device.obj), nm_device_get_iface(priv->parent_device.obj)); } @@ -5988,8 +5988,8 @@ nm_device_master_release_slave(NMDevice *self, info = find_slave_info(self, slave); _LOGT(LOGD_CORE, - "master: release one slave %p/%s %s%s", - slave, + "master: release one slave " NM_HASH_OBFUSCATE_PTR_FMT "/%s %s%s", + NM_HASH_OBFUSCATE_PTR(slave), nm_device_get_iface(slave), !info ? "(not registered)" : (info->slave_is_enslaved ? "(enslaved)" : "(not enslaved)"), release_type == RELEASE_SLAVE_TYPE_CONFIG_FORCE @@ -7582,8 +7582,8 @@ nm_device_master_add_slave(NMDevice *self, NMDevice *slave, gboolean configure) info = find_slave_info(self, slave); _LOGT(LOGD_CORE, - "master: add one slave %p/%s%s", - slave, + "master: add one slave " NM_HASH_OBFUSCATE_PTR_FMT "/%s%s", + NM_HASH_OBFUSCATE_PTR(slave), nm_device_get_iface(slave), info ? " (already registered)" : "");