From e9aa3cc3575b8456eb712c0e06dc815940b49cbc Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 12 May 2017 16:17:18 +0200 Subject: [PATCH 1/7] device: don't call virtual function carrier_changed() directly Don't give the subclass the ability to override the parents behavior. The parent implementation is not intended to allow for that. Instead, restrict the flexibility of how the virtual function integrates with the larger picture. That means, the virtual function is only called at one place, and there is only one implementation in NMDeviceEthernet (and it doesn't really matter whether the implementation chains up the parent implementation or not). (cherry picked from commit 5a7374d8be33086a5c00a450a472069595ba1734) --- src/devices/nm-device-ethernet.c | 7 +++---- src/devices/nm-device.c | 17 ++++++++++++----- src/devices/nm-device.h | 2 +- 3 files changed, 16 insertions(+), 10 deletions(-) diff --git a/src/devices/nm-device-ethernet.c b/src/devices/nm-device-ethernet.c index 4c5aeb5e1b..8a04d401ca 100644 --- a/src/devices/nm-device-ethernet.c +++ b/src/devices/nm-device-ethernet.c @@ -1597,12 +1597,11 @@ get_link_speed (NMDevice *device) } static void -carrier_changed (NMDevice *device, gboolean carrier) +carrier_changed_notify (NMDevice *device, gboolean carrier) { if (carrier) get_link_speed (device); - - NM_DEVICE_CLASS (nm_device_ethernet_parent_class)->carrier_changed (device, carrier); + NM_DEVICE_CLASS (nm_device_ethernet_parent_class)->carrier_changed_notify (device, carrier); } static void @@ -1764,7 +1763,7 @@ nm_device_ethernet_class_init (NMDeviceEthernetClass *klass) parent_class->deactivate = deactivate; parent_class->get_s390_subchannels = get_s390_subchannels; parent_class->update_connection = update_connection; - parent_class->carrier_changed = carrier_changed; + parent_class->carrier_changed_notify = carrier_changed_notify; parent_class->link_changed = link_changed; parent_class->is_available = is_available; parent_class->can_reapply_change = can_reapply_change; diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index da581a0d4e..ab5770d93f 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -2167,11 +2167,19 @@ nm_device_update_dynamic_ip_setup (NMDevice *self) } } +static void +carrier_changed_notify (NMDevice *self, gboolean carrier) +{ + /* stub */ +} + static void carrier_changed (NMDevice *self, gboolean carrier) { NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE (self); + NM_DEVICE_GET_CLASS (self)->carrier_changed_notify (self, carrier); + if (priv->state <= NM_DEVICE_STATE_UNMANAGED) return; @@ -2245,7 +2253,7 @@ link_disconnect_action_cb (gpointer user_data) priv->carrier_defer_id = 0; - NM_DEVICE_GET_CLASS (self)->carrier_changed (self, FALSE); + carrier_changed (self, FALSE); return FALSE; } @@ -2266,7 +2274,6 @@ void nm_device_set_carrier (NMDevice *self, gboolean carrier) { NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE (self); - NMDeviceClass *klass = NM_DEVICE_GET_CLASS (self); NMDeviceState state = nm_device_get_state (self); if (priv->carrier == carrier) @@ -2278,7 +2285,7 @@ nm_device_set_carrier (NMDevice *self, gboolean carrier) if (priv->carrier) { _LOGI (LOGD_DEVICE, "link connected"); link_disconnect_action_cancel (self); - klass->carrier_changed (self, TRUE); + carrier_changed (self, TRUE); if (nm_clear_g_source (&priv->carrier_wait_id)) { nm_device_remove_pending_action (self, NM_PENDING_ACTION_CARRIER_WAIT, TRUE); @@ -2287,7 +2294,7 @@ nm_device_set_carrier (NMDevice *self, gboolean carrier) } else if ( state <= NM_DEVICE_STATE_DISCONNECTED && !priv->queued_act_request) { _LOGD (LOGD_DEVICE, "link disconnected"); - klass->carrier_changed (self, FALSE); + carrier_changed (self, FALSE); } else { priv->carrier_defer_id = g_timeout_add_seconds (LINK_DISCONNECT_DELAY, link_disconnect_action_cb, self); @@ -14161,7 +14168,7 @@ nm_device_class_init (NMDeviceClass *klass) klass->can_unmanaged_external_down = can_unmanaged_external_down; klass->realize_start_notify = realize_start_notify; klass->unrealize_notify = unrealize_notify; - klass->carrier_changed = carrier_changed; + klass->carrier_changed_notify = carrier_changed_notify; klass->get_ip_iface_identifier = get_ip_iface_identifier; klass->unmanaged_on_quit = unmanaged_on_quit; klass->deactivate_reset_hw_addr = deactivate_reset_hw_addr; diff --git a/src/devices/nm-device.h b/src/devices/nm-device.h index be328eb6bf..5e6abb4a8f 100644 --- a/src/devices/nm-device.h +++ b/src/devices/nm-device.h @@ -261,7 +261,7 @@ typedef struct { gboolean (*can_unmanaged_external_down) (NMDevice *self); /* Carrier state (IFF_LOWER_UP) */ - void (*carrier_changed) (NMDevice *, gboolean carrier); + void (*carrier_changed_notify) (NMDevice *, gboolean carrier); gboolean (* get_ip_iface_identifier) (NMDevice *self, NMUtilsIPv6IfaceId *out_iid); From 0ed6b5bfff4d8a915e69866d15027d26e3785271 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 12 May 2017 16:21:55 +0200 Subject: [PATCH 2/7] device/trivial: rename functions related to "carrier" (cherry picked from commit a07c6255a02e098dae934ee0e6765e1ce5b927ae) --- src/devices/nm-device.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index ab5770d93f..c3423319c9 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -2167,6 +2167,8 @@ nm_device_update_dynamic_ip_setup (NMDevice *self) } } +/*****************************************************************************/ + static void carrier_changed_notify (NMDevice *self, gboolean carrier) { @@ -2244,7 +2246,7 @@ carrier_changed (NMDevice *self, gboolean carrier) #define LINK_DISCONNECT_DELAY 4 static gboolean -link_disconnect_action_cb (gpointer user_data) +carrier_disconnected_action_cb (gpointer user_data) { NMDevice *self = NM_DEVICE (user_data); NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE (self); @@ -2259,7 +2261,7 @@ link_disconnect_action_cb (gpointer user_data) } static void -link_disconnect_action_cancel (NMDevice *self) +carrier_disconnected_action_cancel (NMDevice *self) { NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE (self); @@ -2284,7 +2286,7 @@ nm_device_set_carrier (NMDevice *self, gboolean carrier) if (priv->carrier) { _LOGI (LOGD_DEVICE, "link connected"); - link_disconnect_action_cancel (self); + carrier_disconnected_action_cancel (self); carrier_changed (self, TRUE); if (nm_clear_g_source (&priv->carrier_wait_id)) { @@ -2297,12 +2299,14 @@ nm_device_set_carrier (NMDevice *self, gboolean carrier) carrier_changed (self, FALSE); } else { priv->carrier_defer_id = g_timeout_add_seconds (LINK_DISCONNECT_DELAY, - link_disconnect_action_cb, self); + carrier_disconnected_action_cb, self); _LOGD (LOGD_DEVICE, "link disconnected (deferring action for %d seconds) (id=%u)", LINK_DISCONNECT_DELAY, priv->carrier_defer_id); } } +/*****************************************************************************/ + static void device_recheck_slave_status (NMDevice *self, const NMPlatformLink *plink) { @@ -13774,7 +13778,7 @@ dispose (GObject *object) nm_clear_g_source (&priv->stats.timeout_id); - link_disconnect_action_cancel (self); + carrier_disconnected_action_cancel (self); if (priv->ifindex > 0) { priv->ifindex = 0; From 62f1875766a181528c36596b7bd16a78663879cc Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 12 May 2017 16:22:47 +0200 Subject: [PATCH 3/7] device: minor cleanup of carrier_disconnected_action_cancel() (cherry picked from commit 6c5d883a4bd9ef167ee4fe8ec2b0187c7bc77142) --- src/devices/nm-device.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index c3423319c9..a7916af58a 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -2254,9 +2254,7 @@ carrier_disconnected_action_cb (gpointer user_data) _LOGD (LOGD_DEVICE, "link disconnected (calling deferred action) (id=%u)", priv->carrier_defer_id); priv->carrier_defer_id = 0; - carrier_changed (self, FALSE); - return FALSE; } @@ -2264,11 +2262,11 @@ static void carrier_disconnected_action_cancel (NMDevice *self) { NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE (self); + guint id = priv->carrier_defer_id; - if (priv->carrier_defer_id) { - g_source_remove (priv->carrier_defer_id); - _LOGD (LOGD_DEVICE, "link disconnected (canceling deferred action) (id=%u)", priv->carrier_defer_id); - priv->carrier_defer_id = 0; + if (nm_clear_g_source (&priv->carrier_defer_id)) { + _LOGD (LOGD_DEVICE, "link disconnected (canceling deferred action) (id=%u)", + id); } } From f4600c7fa5afd960fb3657ca6d694e56f5dc5dac Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 12 May 2017 16:48:57 +0200 Subject: [PATCH 4/7] device: downgrade logging messages about (non) pending action Adding/Removing a pending action with assert_not_yet_pending/ assert_is_pending means that we expect that no action is taken. Downgrade the logging level in those cases to . (cherry picked from commit eaba285375248a691aaa896fecdd991ad695c1b1) --- src/devices/nm-device.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index a7916af58a..1d7ca574a2 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -11821,7 +11821,7 @@ nm_device_add_pending_action (NMDevice *self, const char *action, gboolean asser count + g_slist_length (iter), action); g_return_val_if_reached (FALSE); } else { - _LOGD (LOGD_DEVICE, "add_pending_action (%d): '%s' already pending (expected)", + _LOGT (LOGD_DEVICE, "add_pending_action (%d): '%s' already pending (expected)", count + g_slist_length (iter), action); } return FALSE; @@ -11882,7 +11882,7 @@ nm_device_remove_pending_action (NMDevice *self, const char *action, gboolean as _LOGW (LOGD_DEVICE, "remove_pending_action (%d): '%s' not pending", count, action); g_return_val_if_reached (FALSE); } else - _LOGD (LOGD_DEVICE, "remove_pending_action (%d): '%s' not pending (expected)", count, action); + _LOGT (LOGD_DEVICE, "remove_pending_action (%d): '%s' not pending (expected)", count, action); return FALSE; } From 83c2243d800fb20a651f787b65e6c5586a6f970d Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sun, 14 May 2017 22:08:26 +0200 Subject: [PATCH 5/7] device: rename and minor refactoring of check_carrier() The name should mirror what we already have: nm_device_set_carrier(). Also, move the code closer to nm_device_set_carrier() and refactor it a bit. (cherry picked from commit 7e472b4eb36347684e81e1c3a2bd7348e19eb628) --- src/devices/nm-device.c | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index 1d7ca574a2..e5f1e2d7fb 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -2303,6 +2303,16 @@ nm_device_set_carrier (NMDevice *self, gboolean carrier) } } +static void +nm_device_set_carrier_from_platform (NMDevice *self) +{ + if (!nm_device_has_capability (self, NM_DEVICE_CAP_NONSTANDARD_CARRIER)) { + nm_device_set_carrier (self, + nm_platform_link_is_connected (nm_device_get_platform (self), + nm_device_get_ip_ifindex (self))); + } +} + /*****************************************************************************/ static void @@ -2880,15 +2890,6 @@ config_changed (NMConfig *config, device_init_sriov_num_vfs (self); } -static void -check_carrier (NMDevice *self) -{ - int ifindex = nm_device_get_ip_ifindex (self); - - if (!nm_device_has_capability (self, NM_DEVICE_CAP_NONSTANDARD_CARRIER)) - nm_device_set_carrier (self, nm_platform_link_is_connected (nm_device_get_platform (self), ifindex)); -} - static void realize_start_notify (NMDevice *self, const NMPlatformLink *pllink) @@ -3020,7 +3021,7 @@ realize_start_setup (NMDevice *self, } if (nm_device_has_capability (self, NM_DEVICE_CAP_CARRIER_DETECT)) { - check_carrier (self); + nm_device_set_carrier_from_platform (self); _LOGD (LOGD_PLATFORM, "carrier is %s%s", priv->carrier ? "ON" : "OFF", @@ -10343,7 +10344,7 @@ nm_device_bring_up (NMDevice *self, gboolean block, gboolean *no_firmware) /* Store carrier immediately. */ if (nm_device_has_capability (self, NM_DEVICE_CAP_CARRIER_DETECT)) - check_carrier (self); + nm_device_set_carrier_from_platform (self); device_is_up = nm_device_is_up (self); if (block && !device_is_up) { From 3786e17c0f86561e23779490ec5032b432aa7178 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 15 May 2017 11:35:41 +0200 Subject: [PATCH 6/7] device: cleanup nm_device_set_carrier_from_platform() nm_device_set_carrier_from_platform() is only called from two places. - both check for NM_DEVICE_CAP_CARRIER_DETECT, so move that check inside the function. - drop the logging in realize_start_setup(). nm_device_set_carrier() already does logging. - always set the fake carrier in nm_device_set_carrier_from_platform(). For the fake carrer, we anyway expect it to be already TRUE in most case, so usually this should have no effect. Also emit a property changed signal. That is necessary to refresh the D-Bus property. (cherry picked from commit 02bb4ce7eb518bf955ed802511f1efde921bc919) --- src/devices/nm-device.c | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index e5f1e2d7fb..5b10e176dc 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -2306,10 +2306,20 @@ nm_device_set_carrier (NMDevice *self, gboolean carrier) static void nm_device_set_carrier_from_platform (NMDevice *self) { - if (!nm_device_has_capability (self, NM_DEVICE_CAP_NONSTANDARD_CARRIER)) { - nm_device_set_carrier (self, - nm_platform_link_is_connected (nm_device_get_platform (self), - nm_device_get_ip_ifindex (self))); + if (nm_device_has_capability (self, NM_DEVICE_CAP_CARRIER_DETECT)) { + if (!nm_device_has_capability (self, NM_DEVICE_CAP_NONSTANDARD_CARRIER)) { + nm_device_set_carrier (self, + nm_platform_link_is_connected (nm_device_get_platform (self), + nm_device_get_ip_ifindex (self))); + } + } else { + NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE (self); + + /* Fake online link when carrier detection is not available. */ + if (!priv->carrier) { + priv->carrier = TRUE; + _notify (self, PROP_CARRIER); + } } } @@ -3020,16 +3030,7 @@ realize_start_setup (NMDevice *self, self); } - if (nm_device_has_capability (self, NM_DEVICE_CAP_CARRIER_DETECT)) { - nm_device_set_carrier_from_platform (self); - _LOGD (LOGD_PLATFORM, - "carrier is %s%s", - priv->carrier ? "ON" : "OFF", - priv->ignore_carrier ? " (but ignored)" : ""); - } else { - /* Fake online link when carrier detection is not available. */ - priv->carrier = TRUE; - } + nm_device_set_carrier_from_platform (self); device_init_sriov_num_vfs (self); @@ -10343,8 +10344,7 @@ nm_device_bring_up (NMDevice *self, gboolean block, gboolean *no_firmware) } /* Store carrier immediately. */ - if (nm_device_has_capability (self, NM_DEVICE_CAP_CARRIER_DETECT)) - nm_device_set_carrier_from_platform (self); + nm_device_set_carrier_from_platform (self); device_is_up = nm_device_is_up (self); if (block && !device_is_up) { From 51a1fc3cd9f281f1348cf0ec1ea17d4d03ecd0b7 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 12 May 2017 16:32:15 +0200 Subject: [PATCH 7/7] device: fix delaying startup complete waiting for carrier platform: signal: link changed: 2: eth0 mtu ... ... device[0x7f90c29c64d0] (eth0): bringing up device ... platform: signal: link changed: 2: eth0 mtu ... ... device (eth0): link connected ... device[0x7f90c29c64d0] (eth0): add_pending_action (2): 'carrier wait' Note how we schedule the pending action 'carrier-wait', although the device already has carrier. That means, the pending action will not be removed until timeout, 5 seconds later. Avoid scheduling 'carrier-wait' if we already have carrier. However, don't just add the pending action 'carrier-wait' only during nm_device_bring_up(). Instead, always schedule the carrier_wait timeout. This gives a grace period during which we keep setting 'carrier-wait' whenever we have no carrier. This should prevent two cases: - during nm_device_bring_up() the platform state might not yet have caught up. If we don't add the pending action there, we will add it a moment later when carrier goes away. - bringing the interface up might cause carrier to get lost for a moment (flapping). If that happens within the timeout, also add the pending action. (cherry picked from commit 9f874d166d260bb4b9af32cb8d12d287341a9a8b) --- src/devices/nm-device.c | 47 +++++++++++++++++++++++++---------------- 1 file changed, 29 insertions(+), 18 deletions(-) diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index 5b10e176dc..4a2847b902 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -2287,19 +2287,23 @@ nm_device_set_carrier (NMDevice *self, gboolean carrier) carrier_disconnected_action_cancel (self); carrier_changed (self, TRUE); - if (nm_clear_g_source (&priv->carrier_wait_id)) { - nm_device_remove_pending_action (self, NM_PENDING_ACTION_CARRIER_WAIT, TRUE); + if (priv->carrier_wait_id) { + nm_device_remove_pending_action (self, NM_PENDING_ACTION_CARRIER_WAIT, FALSE); _carrier_wait_check_queued_act_request (self); } - } else if ( state <= NM_DEVICE_STATE_DISCONNECTED - && !priv->queued_act_request) { - _LOGD (LOGD_DEVICE, "link disconnected"); - carrier_changed (self, FALSE); } else { - priv->carrier_defer_id = g_timeout_add_seconds (LINK_DISCONNECT_DELAY, - carrier_disconnected_action_cb, self); - _LOGD (LOGD_DEVICE, "link disconnected (deferring action for %d seconds) (id=%u)", - LINK_DISCONNECT_DELAY, priv->carrier_defer_id); + if (priv->carrier_wait_id) + nm_device_add_pending_action (self, NM_PENDING_ACTION_CARRIER_WAIT, FALSE); + if ( state <= NM_DEVICE_STATE_DISCONNECTED + && !priv->queued_act_request) { + _LOGD (LOGD_DEVICE, "link disconnected"); + carrier_changed (self, FALSE); + } else { + priv->carrier_defer_id = g_timeout_add_seconds (LINK_DISCONNECT_DELAY, + carrier_disconnected_action_cb, self); + _LOGD (LOGD_DEVICE, "link disconnected (deferring action for %d seconds) (id=%u)", + LINK_DISCONNECT_DELAY, priv->carrier_defer_id); + } } } @@ -10297,12 +10301,12 @@ static gboolean carrier_wait_timeout (gpointer user_data) { NMDevice *self = NM_DEVICE (user_data); + NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE (self); - NM_DEVICE_GET_PRIVATE (self)->carrier_wait_id = 0; - nm_device_remove_pending_action (self, NM_PENDING_ACTION_CARRIER_WAIT, TRUE); - - _carrier_wait_check_queued_act_request (self); - + priv->carrier_wait_id = 0; + nm_device_remove_pending_action (self, NM_PENDING_ACTION_CARRIER_WAIT, FALSE); + if (!priv->carrier) + _carrier_wait_check_queued_act_request (self); return G_SOURCE_REMOVE; } @@ -10379,8 +10383,14 @@ nm_device_bring_up (NMDevice *self, gboolean block, gboolean *no_firmware) * a timeout is reached. */ if (nm_device_has_capability (self, NM_DEVICE_CAP_CARRIER_DETECT)) { - if (!nm_clear_g_source (&priv->carrier_wait_id)) - nm_device_add_pending_action (self, NM_PENDING_ACTION_CARRIER_WAIT, TRUE); + /* we start a grace period of 5 seconds during which we will schedule + * a pending action whenever we have no carrier. + * + * If during that time carrier goes away, we declare the interface + * as not ready. */ + nm_clear_g_source (&priv->carrier_wait_id); + if (!priv->carrier) + nm_device_add_pending_action (self, NM_PENDING_ACTION_CARRIER_WAIT, FALSE); priv->carrier_wait_id = g_timeout_add_seconds (5, carrier_wait_timeout, self); } @@ -13792,7 +13802,8 @@ dispose (GObject *object) available_connections_del_all (self); - nm_clear_g_source (&priv->carrier_wait_id); + if (nm_clear_g_source (&priv->carrier_wait_id)) + nm_device_remove_pending_action (self, NM_PENDING_ACTION_CARRIER_WAIT, FALSE); _clear_queued_act_request (priv);