From 042f2b2e7e57b5630d823f6abdc77075e9c0a394 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 12 Sep 2016 12:29:34 +0200 Subject: [PATCH 1/6] core: use defines for signal names --- src/devices/nm-device.c | 2 +- src/devices/nm-device.h | 1 + src/devices/wifi/nm-device-olpc-mesh.c | 10 +++++----- src/devices/wifi/nm-device-wifi.c | 6 +++--- src/devices/wifi/nm-device-wifi.h | 7 +++++++ 5 files changed, 17 insertions(+), 9 deletions(-) diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index b3f1fa85f..db8a23421 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -12921,7 +12921,7 @@ nm_device_class_init (NMDeviceClass *klass) G_TYPE_UINT, G_TYPE_UINT, G_TYPE_UINT); signals[AUTOCONNECT_ALLOWED] = - g_signal_new ("autoconnect-allowed", + g_signal_new (NM_DEVICE_AUTOCONNECT_ALLOWED, G_OBJECT_CLASS_TYPE (object_class), G_SIGNAL_RUN_LAST, 0, diff --git a/src/devices/nm-device.h b/src/devices/nm-device.h index a757a37eb..c2863140d 100644 --- a/src/devices/nm-device.h +++ b/src/devices/nm-device.h @@ -87,6 +87,7 @@ #define NM_DEVICE_RECHECK_ASSUME "recheck-assume" #define NM_DEVICE_STATE_CHANGED "state-changed" #define NM_DEVICE_LINK_INITIALIZED "link-initialized" +#define NM_DEVICE_AUTOCONNECT_ALLOWED "autoconnect-allowed" #define NM_DEVICE_STATISTICS_REFRESH_RATE_MS "refresh-rate-ms" #define NM_DEVICE_STATISTICS_TX_BYTES "tx-bytes" diff --git a/src/devices/wifi/nm-device-olpc-mesh.c b/src/devices/wifi/nm-device-olpc-mesh.c index 6fbc63a1b..77a963756 100644 --- a/src/devices/wifi/nm-device-olpc-mesh.c +++ b/src/devices/wifi/nm-device-olpc-mesh.c @@ -178,7 +178,7 @@ act_stage1_prepare (NMDevice *device, NMDeviceStateReason *reason) /* wait with continuing configuration untill the companion device is done scanning */ - g_object_get (priv->companion, "scanning", &scanning, NULL); + g_object_get (priv->companion, NM_DEVICE_WIFI_SCANNING, &scanning, NULL); if (scanning) { priv->stage1_waiting = TRUE; return NM_ACT_STAGE_RETURN_POSTPONE; @@ -267,7 +267,7 @@ companion_notify_cb (NMDeviceWifi *companion, GParamSpec *pspec, gpointer user_d if (!priv->stage1_waiting) return; - g_object_get (companion, "scanning", &scanning, NULL); + g_object_get (companion, NM_DEVICE_WIFI_SCANNING, &scanning, NULL); if (!scanning) { priv->stage1_waiting = FALSE; @@ -343,13 +343,13 @@ check_companion (NMDeviceOlpcMesh *self, NMDevice *other) g_signal_connect (G_OBJECT (other), NM_DEVICE_STATE_CHANGED, G_CALLBACK (companion_state_changed_cb), self); - g_signal_connect (G_OBJECT (other), "notify::scanning", + g_signal_connect (G_OBJECT (other), "notify::" NM_DEVICE_WIFI_SCANNING, G_CALLBACK (companion_notify_cb), self); - g_signal_connect (G_OBJECT (other), "scanning-allowed", + g_signal_connect (G_OBJECT (other), NM_DEVICE_WIFI_SCANNING_ALLOWED, G_CALLBACK (companion_scan_allowed_cb), self); - g_signal_connect (G_OBJECT (other), "autoconnect-allowed", + g_signal_connect (G_OBJECT (other), NM_DEVICE_AUTOCONNECT_ALLOWED, G_CALLBACK (companion_autoconnect_allowed_cb), self); g_object_notify (G_OBJECT (self), NM_DEVICE_OLPC_MESH_COMPANION); diff --git a/src/devices/wifi/nm-device-wifi.c b/src/devices/wifi/nm-device-wifi.c index f97e66893..14d4c84a0 100644 --- a/src/devices/wifi/nm-device-wifi.c +++ b/src/devices/wifi/nm-device-wifi.c @@ -3199,7 +3199,7 @@ nm_device_wifi_class_init (NMDeviceWifiClass *klass) /* Signals */ signals[ACCESS_POINT_ADDED] = - g_signal_new ("access-point-added", + g_signal_new (NM_DEVICE_WIFI_ACCESS_POINT_ADDED, G_OBJECT_CLASS_TYPE (object_class), G_SIGNAL_RUN_FIRST, 0, @@ -3208,7 +3208,7 @@ nm_device_wifi_class_init (NMDeviceWifiClass *klass) NM_TYPE_AP); signals[ACCESS_POINT_REMOVED] = - g_signal_new ("access-point-removed", + g_signal_new (NM_DEVICE_WIFI_ACCESS_POINT_REMOVED, G_OBJECT_CLASS_TYPE (object_class), G_SIGNAL_RUN_FIRST, 0, @@ -3217,7 +3217,7 @@ nm_device_wifi_class_init (NMDeviceWifiClass *klass) NM_TYPE_AP); signals[SCANNING_ALLOWED] = - g_signal_new ("scanning-allowed", + g_signal_new (NM_DEVICE_WIFI_SCANNING_ALLOWED, G_OBJECT_CLASS_TYPE (object_class), G_SIGNAL_RUN_LAST, G_STRUCT_OFFSET (NMDeviceWifiClass, scanning_allowed), diff --git a/src/devices/wifi/nm-device-wifi.h b/src/devices/wifi/nm-device-wifi.h index ba9d60e2b..d2914fa21 100644 --- a/src/devices/wifi/nm-device-wifi.h +++ b/src/devices/wifi/nm-device-wifi.h @@ -39,6 +39,13 @@ #define NM_DEVICE_WIFI_CAPABILITIES "wireless-capabilities" #define NM_DEVICE_WIFI_SCANNING "scanning" +/* signals */ +#define NM_DEVICE_WIFI_ACCESS_POINT_ADDED "access-point-added" +#define NM_DEVICE_WIFI_ACCESS_POINT_REMOVED "access-point-removed" + +/* internal signals */ +#define NM_DEVICE_WIFI_SCANNING_ALLOWED "scanning-allowed" + typedef struct _NMDeviceWifi NMDeviceWifi; typedef struct _NMDeviceWifiClass NMDeviceWifiClass; From b122337353fb31b3837b5c252bac807a38a59aeb Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 12 Sep 2016 12:17:17 +0200 Subject: [PATCH 2/6] device: implement get_enabled() for NMDeviceWifi The virtual function NMDevice:set_enabled() has two implementations: NMDeviceModem and NMDeviceWifi. Likewise, the get_enabled() function should also be implemented by those types. The only caller of nm_device_get_enabled() is NMPolicy:schedule_activate_check(). It is correct to skip Wi-Fi devices based on their enabled state. --- src/devices/wifi/nm-device-wifi.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/devices/wifi/nm-device-wifi.c b/src/devices/wifi/nm-device-wifi.c index 14d4c84a0..c7771addf 100644 --- a/src/devices/wifi/nm-device-wifi.c +++ b/src/devices/wifi/nm-device-wifi.c @@ -2945,6 +2945,12 @@ device_state_changed (NMDevice *device, remove_all_aps (self); } +static gboolean +get_enabled (NMDevice *device) +{ + return NM_DEVICE_WIFI_GET_PRIVATE ((NMDeviceWifi *) device)->enabled; +} + static void set_enabled (NMDevice *device, gboolean enabled) { @@ -3138,6 +3144,7 @@ nm_device_wifi_class_init (NMDeviceWifiClass *klass) parent_class->check_connection_compatible = check_connection_compatible; parent_class->check_connection_available = check_connection_available; parent_class->complete_connection = complete_connection; + parent_class->get_enabled = get_enabled; parent_class->set_enabled = set_enabled; parent_class->act_stage1_prepare = act_stage1_prepare; From 9deb6ede73d4cd26a1e197265067f94ddb9ecb69 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 12 Sep 2016 12:32:27 +0200 Subject: [PATCH 3/6] device: drop NMDeviceWifi:bring_up() implementation Instead of letting the sub-class check the "enabled" state, let it be handled by nm_device_bring_up(). Note that nm_device_get_enabled() only has two implementations: NMDeviceModem:bring_up() and NMDeviceWifi:bring_up(). --- src/devices/nm-device.c | 5 +++++ src/devices/wifi/nm-device-wifi.c | 10 ---------- 2 files changed, 5 insertions(+), 10 deletions(-) diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index db8a23421..eee93e37a 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -9120,6 +9120,11 @@ nm_device_bring_up (NMDevice *self, gboolean block, gboolean *no_firmware) _LOGD (LOGD_HW, "bringing up device"); + NM_SET_OUT (no_firmware, FALSE); + + if (!nm_device_get_enabled (self)) + return FALSE; + if (NM_DEVICE_GET_CLASS (self)->bring_up) { if (!NM_DEVICE_GET_CLASS (self)->bring_up (self, no_firmware)) return FALSE; diff --git a/src/devices/wifi/nm-device-wifi.c b/src/devices/wifi/nm-device-wifi.c index c7771addf..e106c88a8 100644 --- a/src/devices/wifi/nm-device-wifi.c +++ b/src/devices/wifi/nm-device-wifi.c @@ -452,15 +452,6 @@ periodic_update_cb (gpointer user_data) return TRUE; } -static gboolean -bring_up (NMDevice *device, gboolean *no_firmware) -{ - if (!NM_DEVICE_WIFI_GET_PRIVATE ((NMDeviceWifi *) device)->enabled) - return FALSE; - - return NM_DEVICE_CLASS (nm_device_wifi_parent_class)->bring_up (device, no_firmware); -} - static void ap_add_remove (NMDeviceWifi *self, guint signum, @@ -3138,7 +3129,6 @@ nm_device_wifi_class_init (NMDeviceWifiClass *klass) object_class->dispose = dispose; object_class->finalize = finalize; - parent_class->bring_up = bring_up; parent_class->can_auto_connect = can_auto_connect; parent_class->is_available = is_available; parent_class->check_connection_compatible = check_connection_compatible; From 2c8cb145c2adb177b0e318ca13f28b49838e2cb8 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 12 Sep 2016 12:40:32 +0200 Subject: [PATCH 4/6] device: drop NMDeviceVlan:bring_up() implementation This retry loop was added by commit dc6341acec2d12856265d6976420d56f0560410b. But I suspect, that the main-point there was not to retry the netlink request to set the interface up. Why would that fail, and why would a failure to set the interface up require a retry? I think it was added to wait for carrier. But waiting for carrier was later dropped with commit 5074898591ae99f79b0cc4872c3c7cf018abee82 and it is not clear why we would wait for carrier at all -- we don't do that for other device types either. --- src/devices/nm-device-vlan.c | 15 --------------- 1 file changed, 15 deletions(-) diff --git a/src/devices/nm-device-vlan.c b/src/devices/nm-device-vlan.c index 16edc0d2b..ad9d2486a 100644 --- a/src/devices/nm-device-vlan.c +++ b/src/devices/nm-device-vlan.c @@ -282,20 +282,6 @@ get_generic_capabilities (NMDevice *dev) return NM_DEVICE_CAP_CARRIER_DETECT | NM_DEVICE_CAP_IS_SOFTWARE; } -static gboolean -bring_up (NMDevice *dev, gboolean *no_firmware) -{ - gboolean success = FALSE; - guint i = 20; - - while (i-- > 0 && !success) { - success = NM_DEVICE_CLASS (nm_device_vlan_parent_class)->bring_up (dev, no_firmware); - g_usleep (50); - } - - return success; -} - /******************************************************************/ static gboolean @@ -673,7 +659,6 @@ nm_device_vlan_class_init (NMDeviceVlanClass *klass) parent_class->realize_start_notify = realize_start_notify; parent_class->unrealize_notify = unrealize_notify; parent_class->get_generic_capabilities = get_generic_capabilities; - parent_class->bring_up = bring_up; parent_class->act_stage1_prepare = act_stage1_prepare; parent_class->ip4_config_pre_commit = ip4_config_pre_commit; parent_class->is_available = is_available; From 14ae46021b7c130f856499f21725c7297697b230 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 12 Sep 2016 12:35:21 +0200 Subject: [PATCH 5/6] device: drop NMDeviceMacvlan:bring_up() implementation This was added by commit 4de8851eca06d797f1a3c89f5710d51018ca2bff, probably by copying from NMDeviceVlan. It's not clear why a netlink request to set the device IFF_UP would fail, or why that warrants a retry. --- src/devices/nm-device-macvlan.c | 15 --------------- 1 file changed, 15 deletions(-) diff --git a/src/devices/nm-device-macvlan.c b/src/devices/nm-device-macvlan.c index 2bfc65cb2..e9841a9d9 100644 --- a/src/devices/nm-device-macvlan.c +++ b/src/devices/nm-device-macvlan.c @@ -267,20 +267,6 @@ get_generic_capabilities (NMDevice *dev) return NM_DEVICE_CAP_CARRIER_DETECT | NM_DEVICE_CAP_IS_SOFTWARE; } -static gboolean -bring_up (NMDevice *dev, gboolean *no_firmware) -{ - gboolean success = FALSE; - guint i = 20; - - while (i-- > 0 && !success) { - success = NM_DEVICE_CLASS (nm_device_macvlan_parent_class)->bring_up (dev, no_firmware); - g_usleep (50); - } - - return success; -} - /******************************************************************/ static gboolean @@ -616,7 +602,6 @@ nm_device_macvlan_class_init (NMDeviceMacvlanClass *klass) object_class->set_property = set_property; device_class->act_stage1_prepare = act_stage1_prepare; - device_class->bring_up = bring_up; device_class->check_connection_compatible = check_connection_compatible; device_class->complete_connection = complete_connection; device_class->connection_type = NM_SETTING_MACVLAN_SETTING_NAME; From d461eb6894b0e9aa43ebcda7dc6546d128cd068e Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 12 Sep 2016 12:01:56 +0200 Subject: [PATCH 6/6] device: drop virtual methods for bring_up(), take_down() and is_up() They have no more implementations in derived classes. --- src/devices/nm-device.c | 78 ++++++++++++----------------------------- src/devices/nm-device.h | 3 -- 2 files changed, 22 insertions(+), 59 deletions(-) diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index eee93e37a..114630169 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -9093,19 +9093,11 @@ carrier_wait_timeout (gpointer user_data) static gboolean nm_device_is_up (NMDevice *self) { + int ifindex; + g_return_val_if_fail (NM_IS_DEVICE (self), FALSE); - if (NM_DEVICE_GET_CLASS (self)->is_up) - return NM_DEVICE_GET_CLASS (self)->is_up (self); - - return TRUE; -} - -static gboolean -is_up (NMDevice *self) -{ - int ifindex = nm_device_get_ip_ifindex (self); - + ifindex = nm_device_get_ip_ifindex (self); return ifindex > 0 ? nm_platform_link_is_up (NM_PLATFORM_GET, ifindex) : TRUE; } @@ -9115,18 +9107,23 @@ nm_device_bring_up (NMDevice *self, gboolean block, gboolean *no_firmware) NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE (self); gboolean device_is_up = FALSE; NMDeviceCapabilities capabilities; + int ifindex; g_return_val_if_fail (NM_IS_DEVICE (self), FALSE); - _LOGD (LOGD_HW, "bringing up device"); - NM_SET_OUT (no_firmware, FALSE); - if (!nm_device_get_enabled (self)) + if (!nm_device_get_enabled (self)) { + _LOGD (LOGD_HW, "bringing up device ignored due to disabled"); return FALSE; + } - if (NM_DEVICE_GET_CLASS (self)->bring_up) { - if (!NM_DEVICE_GET_CLASS (self)->bring_up (self, no_firmware)) + ifindex = nm_device_get_ip_ifindex (self); + _LOGD (LOGD_HW, "bringing up device %d", ifindex); + if (ifindex <= 0) { + /* assume success. */ + } else { + if (!nm_platform_link_set_up (NM_PLATFORM_GET, ifindex, no_firmware)) return FALSE; } @@ -9136,7 +9133,6 @@ nm_device_bring_up (NMDevice *self, gboolean block, gboolean *no_firmware) device_is_up = nm_device_is_up (self); if (block && !device_is_up) { - int ifindex = nm_device_get_ip_ifindex (self); gint64 wait_until = nm_utils_get_monotonic_timestamp_us () + 10000 /* microseconds */; do { @@ -9191,40 +9187,26 @@ nm_device_bring_up (NMDevice *self, gboolean block, gboolean *no_firmware) return TRUE; } -static gboolean -bring_up (NMDevice *self, gboolean *no_firmware) -{ - int ifindex = nm_device_get_ip_ifindex (self); - gboolean result; - - if (ifindex <= 0) { - if (no_firmware) - *no_firmware = FALSE; - return TRUE; - } - - result = nm_platform_link_set_up (NM_PLATFORM_GET, ifindex, no_firmware); - - return result; -} - void nm_device_take_down (NMDevice *self, gboolean block) { + int ifindex; gboolean device_is_up; g_return_if_fail (NM_IS_DEVICE (self)); - _LOGD (LOGD_HW, "taking down device"); - - if (NM_DEVICE_GET_CLASS (self)->take_down) { - if (!NM_DEVICE_GET_CLASS (self)->take_down (self)) - return; + ifindex = nm_device_get_ip_ifindex (self); + _LOGD (LOGD_HW, "taking down device %d", ifindex); + if (ifindex <= 0) { + /* devices without ifindex are always up. */ + return; } + if (!nm_platform_link_set_down (NM_PLATFORM_GET, ifindex)) + return; + device_is_up = nm_device_is_up (self); if (block && device_is_up) { - int ifindex = nm_device_get_ip_ifindex (self); gint64 wait_until = nm_utils_get_monotonic_timestamp_us () + 10000 /* microseconds */; do { @@ -9243,19 +9225,6 @@ nm_device_take_down (NMDevice *self, gboolean block) } } -static gboolean -take_down (NMDevice *self) -{ - int ifindex = nm_device_get_ip_ifindex (self); - - if (ifindex > 0) - return nm_platform_link_set_down (NM_PLATFORM_GET, ifindex); - - /* devices without ifindex are always up. */ - _LOGD (LOGD_HW, "cannot take down device without ifindex"); - return FALSE; -} - void nm_device_set_firmware_missing (NMDevice *self, gboolean new_missing) { @@ -12689,9 +12658,6 @@ 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->is_up = is_up; - klass->bring_up = bring_up; - klass->take_down = take_down; klass->carrier_changed = carrier_changed; klass->get_ip_iface_identifier = get_ip_iface_identifier; klass->unmanaged_on_quit = unmanaged_on_quit; diff --git a/src/devices/nm-device.h b/src/devices/nm-device.h index c2863140d..4e4d24e1f 100644 --- a/src/devices/nm-device.h +++ b/src/devices/nm-device.h @@ -205,9 +205,6 @@ typedef struct { /* Hardware state (IFF_UP) */ gboolean (*can_unmanaged_external_down) (NMDevice *self); - gboolean (*is_up) (NMDevice *self); - gboolean (*bring_up) (NMDevice *self, gboolean *no_firmware); - gboolean (*take_down) (NMDevice *self); /* Carrier state (IFF_LOWER_UP) */ void (*carrier_changed) (NMDevice *, gboolean carrier);