From 5a7374d8be33086a5c00a450a472069595ba1734 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). --- 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 68d6f48d40..e445a80d10 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -2170,11 +2170,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; @@ -2248,7 +2256,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; } @@ -2269,7 +2277,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) @@ -2281,7 +2288,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); @@ -2290,7 +2297,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); @@ -14109,7 +14116,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 805952ad09..c91ec2d5ec 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 a07c6255a02e098dae934ee0e6765e1ce5b927ae 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" --- 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 e445a80d10..49becc994e 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -2170,6 +2170,8 @@ nm_device_update_dynamic_ip_setup (NMDevice *self) } } +/*****************************************************************************/ + static void carrier_changed_notify (NMDevice *self, gboolean carrier) { @@ -2247,7 +2249,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); @@ -2262,7 +2264,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); @@ -2287,7 +2289,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)) { @@ -2300,12 +2302,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) { @@ -13720,7 +13724,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 6c5d883a4bd9ef167ee4fe8ec2b0187c7bc77142 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() --- 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 49becc994e..028808cb44 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -2257,9 +2257,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; } @@ -2267,11 +2265,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 eaba285375248a691aaa896fecdd991ad695c1b1 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 . --- 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 028808cb44..f26d434e9b 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -11765,7 +11765,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; @@ -11826,7 +11826,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 7e472b4eb36347684e81e1c3a2bd7348e19eb628 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. --- 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 f26d434e9b..65bcd83d74 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -2306,6 +2306,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 @@ -2883,15 +2893,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) @@ -3023,7 +3024,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", @@ -10287,7 +10288,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 02bb4ce7eb518bf955ed802511f1efde921bc919 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. --- 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 65bcd83d74..1ea051c918 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -2309,10 +2309,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); + } } } @@ -3023,16 +3033,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); @@ -10287,8 +10288,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 9f874d166d260bb4b9af32cb8d12d287341a9a8b 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. --- 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 1ea051c918..4f70d75d66 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -2290,19 +2290,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); + } } } @@ -10241,12 +10245,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; } @@ -10323,8 +10327,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); } @@ -13738,7 +13748,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);