From 82d0fa2a87c5621c9b0412cf1914bb02130e4de5 Mon Sep 17 00:00:00 2001 From: Beniamino Galvani Date: Mon, 6 Mar 2023 15:49:43 +0100 Subject: [PATCH 1/3] device: make detach_port() method asynchronous This changes the signature of detach_port() to be asynchronous, similarly to attach_port(). The implementation can return TRUE/FALSE on immediate completion. Current implementations return immediately and so there is no change in behavior for now. --- src/core/devices/nm-device-bond.c | 11 +++++++++-- src/core/devices/nm-device-bridge.c | 13 ++++++++++--- src/core/devices/nm-device-vrf.c | 11 +++++++++-- src/core/devices/nm-device.c | 15 +++++++++++---- src/core/devices/nm-device.h | 10 +++++++++- src/core/devices/ovs/nm-device-ovs-bridge.c | 13 ++++++++++--- src/core/devices/ovs/nm-device-ovs-port.c | 11 +++++++++-- src/core/devices/team/nm-device-team.c | 11 +++++++++-- 8 files changed, 76 insertions(+), 19 deletions(-) diff --git a/src/core/devices/nm-device-bond.c b/src/core/devices/nm-device-bond.c index cb32331921..8e0cf33fe6 100644 --- a/src/core/devices/nm-device-bond.c +++ b/src/core/devices/nm-device-bond.c @@ -695,8 +695,13 @@ attach_port(NMDevice *device, return TRUE; } -static void -detach_port(NMDevice *device, NMDevice *port, gboolean configure) +static NMTernary +detach_port(NMDevice *device, + NMDevice *port, + gboolean configure, + GCancellable *cancellable, + NMDeviceAttachPortCallback callback, + gpointer user_data) { NMDeviceBond *self = NM_DEVICE_BOND(device); gboolean success; @@ -758,6 +763,8 @@ detach_port(NMDevice *device, NMDevice *port, gboolean configure) _LOGI(LOGD_BOND, "bond port %s was detached", nm_device_get_ip_iface(port)); } } + + return TRUE; } static gboolean diff --git a/src/core/devices/nm-device-bridge.c b/src/core/devices/nm-device-bridge.c index a2d0db6192..c07b154979 100644 --- a/src/core/devices/nm-device-bridge.c +++ b/src/core/devices/nm-device-bridge.c @@ -1052,8 +1052,13 @@ attach_port(NMDevice *device, return TRUE; } -static void -detach_port(NMDevice *device, NMDevice *port, gboolean configure) +static NMTernary +detach_port(NMDevice *device, + NMDevice *port, + gboolean configure, + GCancellable *cancellable, + NMDeviceAttachPortCallback callback, + gpointer user_data) { NMDeviceBridge *self = NM_DEVICE_BRIDGE(device); gboolean success; @@ -1070,7 +1075,7 @@ detach_port(NMDevice *device, NMDevice *port, gboolean configure) if (ifindex_slave <= 0) { _LOGD(LOGD_TEAM, "bridge port %s is already detached", nm_device_get_ip_iface(port)); - return; + return TRUE; } if (configure) { @@ -1086,6 +1091,8 @@ detach_port(NMDevice *device, NMDevice *port, gboolean configure) } else { _LOGI(LOGD_BRIDGE, "bridge port %s was detached", nm_device_get_ip_iface(port)); } + + return TRUE; } static gboolean diff --git a/src/core/devices/nm-device-vrf.c b/src/core/devices/nm-device-vrf.c index b037e8e2a6..a13de1cb66 100644 --- a/src/core/devices/nm-device-vrf.c +++ b/src/core/devices/nm-device-vrf.c @@ -241,8 +241,13 @@ attach_port(NMDevice *device, return TRUE; } -static void -detach_port(NMDevice *device, NMDevice *port, gboolean configure) +static NMTernary +detach_port(NMDevice *device, + NMDevice *port, + gboolean configure, + GCancellable *cancellable, + NMDeviceAttachPortCallback callback, + gpointer user_data) { NMDeviceVrf *self = NM_DEVICE_VRF(device); gboolean success; @@ -277,6 +282,8 @@ detach_port(NMDevice *device, NMDevice *port, gboolean configure) _LOGI(LOGD_DEVICE, "VRF port %s was detached", nm_device_get_ip_iface(port)); } } + + return TRUE; } /*****************************************************************************/ diff --git a/src/core/devices/nm-device.c b/src/core/devices/nm-device.c index 6117c6889a..8f697d5731 100644 --- a/src/core/devices/nm-device.c +++ b/src/core/devices/nm-device.c @@ -6402,10 +6402,17 @@ nm_device_master_release_slave(NMDevice *self, /* first, let subclasses handle the release ... */ 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)->detach_port(self, - slave, - release_type >= RELEASE_SLAVE_TYPE_CONFIG); + || release_type >= RELEASE_SLAVE_TYPE_CONFIG_FORCE) { + NMTernary ret; + + ret = NM_DEVICE_GET_CLASS(self)->detach_port(self, + slave, + release_type >= RELEASE_SLAVE_TYPE_CONFIG, + NULL, + NULL, + NULL); + nm_assert(NM_IN_SET(ret, TRUE, FALSE)); + } /* raise notifications about the release, including clearing is_enslaved. */ nm_device_slave_notify_release(slave, reason, release_type); diff --git a/src/core/devices/nm-device.h b/src/core/devices/nm-device.h index 581df9a358..4794d9af87 100644 --- a/src/core/devices/nm-device.h +++ b/src/core/devices/nm-device.h @@ -389,7 +389,15 @@ typedef struct _NMDeviceClass { GCancellable *cancellable, NMDeviceAttachPortCallback callback, gpointer user_data); - void (*detach_port)(NMDevice *self, NMDevice *port, gboolean configure); + /* This works similarly to attach_port(). However, current + * implementations don't report errors and so the only possible + * return values are TRUE and DEFAULT. */ + NMTernary (*detach_port)(NMDevice *self, + NMDevice *port, + gboolean configure, + GCancellable *cancellable, + NMDeviceAttachPortCallback callback, + gpointer user_data); void (*parent_changed_notify)(NMDevice *self, int old_ifindex, diff --git a/src/core/devices/ovs/nm-device-ovs-bridge.c b/src/core/devices/ovs/nm-device-ovs-bridge.c index 7b319af344..ff1917b133 100644 --- a/src/core/devices/ovs/nm-device-ovs-bridge.c +++ b/src/core/devices/ovs/nm-device-ovs-bridge.c @@ -97,9 +97,16 @@ attach_port(NMDevice *device, return TRUE; } -static void -detach_port(NMDevice *device, NMDevice *port, gboolean configure) -{} +static NMTernary +detach_port(NMDevice *device, + NMDevice *port, + gboolean configure, + GCancellable *cancellable, + NMDeviceAttachPortCallback callback, + gpointer user_data) +{ + return TRUE; +} void nm_device_ovs_reapply_connection(NMDevice *self, NMConnection *con_old, NMConnection *con_new) diff --git a/src/core/devices/ovs/nm-device-ovs-port.c b/src/core/devices/ovs/nm-device-ovs-port.c index 5510e39fbd..f9b24ee5ac 100644 --- a/src/core/devices/ovs/nm-device-ovs-port.c +++ b/src/core/devices/ovs/nm-device-ovs-port.c @@ -221,8 +221,13 @@ del_iface_cb(GError *error, gpointer user_data) g_object_unref(slave); } -static void -detach_port(NMDevice *device, NMDevice *port, gboolean configure) +static NMTernary +detach_port(NMDevice *device, + NMDevice *port, + gboolean configure, + GCancellable *cancellable, + NMDeviceAttachPortCallback callback, + gpointer user_data) { NMDeviceOvsPort *self = NM_DEVICE_OVS_PORT(device); bool port_not_managed = !NM_IN_SET(nm_device_sys_iface_state_get(port), @@ -248,6 +253,8 @@ detach_port(NMDevice *device, NMDevice *port, gboolean configure) if (NM_IS_DEVICE_OVS_INTERFACE(port)) nm_device_update_from_platform_link(port, NULL); } + + return TRUE; } /*****************************************************************************/ diff --git a/src/core/devices/team/nm-device-team.c b/src/core/devices/team/nm-device-team.c index 3e3fe52b67..60b15049a0 100644 --- a/src/core/devices/team/nm-device-team.c +++ b/src/core/devices/team/nm-device-team.c @@ -913,8 +913,13 @@ attach_port(NMDevice *device, return TRUE; } -static void -detach_port(NMDevice *device, NMDevice *port, gboolean configure) +static NMTernary +detach_port(NMDevice *device, + NMDevice *port, + gboolean configure, + GCancellable *cancellable, + NMDeviceAttachPortCallback callback, + gpointer user_data) { NMDeviceTeam *self = NM_DEVICE_TEAM(device); NMDeviceTeamPrivate *priv = NM_DEVICE_TEAM_GET_PRIVATE(self); @@ -964,6 +969,8 @@ detach_port(NMDevice *device, NMDevice *port, gboolean configure) _update_port_config(self, port_iface, "{}"); g_hash_table_remove(priv->port_configs, port_iface); } + + return TRUE; } static gboolean From 07dc237e5ccb4bbe51a5c1e3fd232446a3b98ade Mon Sep 17 00:00:00 2001 From: Beniamino Galvani Date: Mon, 3 Apr 2023 10:26:03 +0200 Subject: [PATCH 2/3] device: wait port detach before leaving the DEACTIVATING state The device shouldn't change state from DEACTIVATING to DISCONNECTED until its detached from its controller; otherwise, the port detach that is in progress can conflict with the following activation. --- src/core/devices/nm-device.c | 31 ++++++++++++++++++++++++++++--- 1 file changed, 28 insertions(+), 3 deletions(-) diff --git a/src/core/devices/nm-device.c b/src/core/devices/nm-device.c index 8f697d5731..304bacaf3e 100644 --- a/src/core/devices/nm-device.c +++ b/src/core/devices/nm-device.c @@ -749,6 +749,9 @@ typedef struct _NMDevicePrivate { guint check_delete_unrealized_id; guint32 interface_flags; + guint32 port_detach_count; + NMDeviceStateReason port_detach_reason; + struct { SriovOp *pending; /* SR-IOV operation currently running */ SriovOp *next; /* next SR-IOV operation scheduled */ @@ -869,6 +872,7 @@ static void sriov_op_cb(GError *error, gpointer user_data); static void device_ifindex_changed_cb(NMManager *manager, NMDevice *device_changed, NMDevice *self); static gboolean device_link_changed(gpointer user_data); static gboolean _get_maybe_ipv6_disabled(NMDevice *self); +static void deactivate_ready(NMDevice *self, NMDeviceStateReason reason); /*****************************************************************************/ @@ -6346,6 +6350,21 @@ nm_device_master_enslave_slave(NMDevice *self, NMDevice *slave, NMConnection *co attach_port_done(self, slave, success); } +static void +detach_port_cb(NMDevice *self, GError *error, gpointer user_data) +{ + nm_auto_unref_object NMDevice *slave = user_data; + NMDevicePrivate *slave_priv = NM_DEVICE_GET_PRIVATE(slave); + + nm_assert(slave_priv->port_detach_count > 0); + + if (--slave_priv->port_detach_count == 0) { + if (slave_priv->state == NM_DEVICE_STATE_DEACTIVATING) { + deactivate_ready(slave, slave_priv->port_detach_reason); + } + } +} + /** * nm_device_master_release_slave: * @self: the master device @@ -6409,9 +6428,12 @@ nm_device_master_release_slave(NMDevice *self, slave, release_type >= RELEASE_SLAVE_TYPE_CONFIG, NULL, - NULL, - NULL); - nm_assert(NM_IN_SET(ret, TRUE, FALSE)); + detach_port_cb, + g_object_ref(slave)); + if (ret == NM_TERNARY_DEFAULT) { + slave_priv->port_detach_count++; + slave_priv->port_detach_reason = reason; + } } /* raise notifications about the release, including clearing is_enslaved. */ @@ -15818,6 +15840,9 @@ deactivate_ready(NMDevice *self, NMDeviceStateReason reason) { NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE(self); + if (priv->port_detach_count > 0) + return; + if (priv->dispatcher.call_id) return; From fc132158267e1b7f6d78684ac61b5bc3db3dbf6d Mon Sep 17 00:00:00 2001 From: Beniamino Galvani Date: Wed, 8 Mar 2023 11:29:17 +0100 Subject: [PATCH 3/3] ovs: implement asynchronous detach_port() Make detach_port() return only after ovsdb reports that the operation finished. --- src/core/devices/ovs/nm-device-ovs-port.c | 49 +++++++++++------------ 1 file changed, 24 insertions(+), 25 deletions(-) diff --git a/src/core/devices/ovs/nm-device-ovs-port.c b/src/core/devices/ovs/nm-device-ovs-port.c index f9b24ee5ac..5ede46e99f 100644 --- a/src/core/devices/ovs/nm-device-ovs-port.c +++ b/src/core/devices/ovs/nm-device-ovs-port.c @@ -78,10 +78,11 @@ typedef struct { GCancellable *cancellable; NMDeviceAttachPortCallback callback; gpointer callback_user_data; + gboolean add; } AttachPortData; static void -add_iface_cb(GError *error, gpointer user_data) +add_del_iface_cb(GError *error, gpointer user_data) { AttachPortData *data = user_data; NMDeviceOvsPort *self; @@ -93,15 +94,17 @@ add_iface_cb(GError *error, gpointer user_data) } else if (error && !nm_utils_error_is_cancelled_or_disposing(error)) { self = NM_DEVICE_OVS_PORT(data->device); _LOGW(LOGD_DEVICE, - "device %s could not be added to a ovs port: %s", + "device %s could not be %s a ovs port: %s", nm_device_get_iface(data->port), + data->add ? "added to" : "removed from", error->message); nm_device_state_changed(data->port, NM_DEVICE_STATE_FAILED, NM_DEVICE_STATE_REASON_OVSDB_FAILED); } - data->callback(data->device, error, data->callback_user_data); + if (data->callback) + data->callback(data->device, error, data->callback_user_data); g_object_unref(data->device); g_object_unref(data->port); @@ -178,6 +181,7 @@ attach_port(NMDevice *device, .cancellable = g_object_ref(cancellable), .callback = callback, .callback_user_data = user_data, + .add = TRUE, }; nm_ovsdb_add_interface(nm_ovsdb_get(), @@ -186,7 +190,7 @@ attach_port(NMDevice *device, nm_device_get_applied_connection(port), bridge_device, port, - add_iface_cb, + add_del_iface_cb, data); /* DPDK ports does not have a link after the devbind, so the MTU must be @@ -205,22 +209,6 @@ attach_port(NMDevice *device, return NM_TERNARY_DEFAULT; } -static void -del_iface_cb(GError *error, gpointer user_data) -{ - NMDevice *slave = user_data; - - if (error && !g_error_matches(error, NM_UTILS_ERROR, NM_UTILS_ERROR_CANCELLED_DISPOSING)) { - nm_log_warn(LOGD_DEVICE, - "device %s could not be removed from a ovs port: %s", - nm_device_get_iface(slave), - error->message); - nm_device_state_changed(slave, NM_DEVICE_STATE_FAILED, NM_DEVICE_STATE_REASON_OVSDB_FAILED); - } - - g_object_unref(slave); -} - static NMTernary detach_port(NMDevice *device, NMDevice *port, @@ -233,6 +221,7 @@ detach_port(NMDevice *device, bool port_not_managed = !NM_IN_SET(nm_device_sys_iface_state_get(port), NM_DEVICE_SYS_IFACE_STATE_MANAGED, NM_DEVICE_SYS_IFACE_STATE_ASSUME); + NMTernary ret = TRUE; _LOGI(LOGD_DEVICE, "detaching ovs interface %s", nm_device_get_ip_iface(port)); @@ -241,10 +230,20 @@ detach_port(NMDevice *device, * to make sure its OVSDB entry is gone. */ if (configure || port_not_managed) { - nm_ovsdb_del_interface(nm_ovsdb_get(), - nm_device_get_iface(port), - del_iface_cb, - g_object_ref(port)); + AttachPortData *data; + + data = g_slice_new(AttachPortData); + *data = (AttachPortData){ + .device = g_object_ref(device), + .port = g_object_ref(port), + .cancellable = nm_g_object_ref(cancellable), + .callback = callback, + .callback_user_data = user_data, + .add = FALSE, + }; + + nm_ovsdb_del_interface(nm_ovsdb_get(), nm_device_get_iface(port), add_del_iface_cb, data); + ret = NM_TERNARY_DEFAULT; } if (configure) { @@ -254,7 +253,7 @@ detach_port(NMDevice *device, nm_device_update_from_platform_link(port, NULL); } - return TRUE; + return ret; } /*****************************************************************************/