From 98651b90a10d71b560ccd3d236ac3c9d5bd36131 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 1 Jun 2017 22:38:51 +0200 Subject: [PATCH 1/4] device: handle default for unset ignore-carrier option depending on device Currently, device types like Bond hack around ignore-carrier setting, as they always want to ignore-carrier. Prepare so that also for such master types, we rely and honor the ignore-carrier setting better. In the next commit, bond, bridge and team devices they will get ignore-carrier turned on by default. --- src/devices/nm-device.c | 8 ++++++++ src/devices/nm-device.h | 2 ++ src/nm-config-data.c | 11 +++++++++-- 3 files changed, 19 insertions(+), 2 deletions(-) diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index b27e1ff180..cc2e39f68b 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -3829,6 +3829,14 @@ nm_device_is_available (NMDevice *self, NMDeviceCheckDevAvailableFlags flags) return NM_DEVICE_GET_CLASS (self)->is_available (self, flags); } +gboolean +nm_device_ignore_carrier_by_default (NMDevice *self) +{ + g_return_val_if_fail (NM_IS_DEVICE (self), FALSE); + + return FALSE; +} + gboolean nm_device_get_enabled (NMDevice *self) { diff --git a/src/devices/nm-device.h b/src/devices/nm-device.h index 0323a78291..fb87de896a 100644 --- a/src/devices/nm-device.h +++ b/src/devices/nm-device.h @@ -493,6 +493,8 @@ NMSetting * nm_device_get_applied_setting (NMDevice *dev, GType setting_ty void nm_device_removed (NMDevice *self, gboolean unconfigure_ip_config); +gboolean nm_device_ignore_carrier_by_default (NMDevice *self); + gboolean nm_device_is_available (NMDevice *dev, NMDeviceCheckDevAvailableFlags flags); gboolean nm_device_has_carrier (NMDevice *dev); diff --git a/src/nm-config-data.c b/src/nm-config-data.c index 8f4d11217d..21ad1a7346 100644 --- a/src/nm-config-data.c +++ b/src/nm-config-data.c @@ -304,15 +304,22 @@ nm_config_data_get_ignore_carrier (const NMConfigData *self, NMDevice *device) { gs_free char *value = NULL; gboolean has_match; + int m; g_return_val_if_fail (NM_IS_CONFIG_DATA (self), FALSE); g_return_val_if_fail (NM_IS_DEVICE (device), FALSE); value = nm_config_data_get_device_config (self, NM_CONFIG_KEYFILE_KEY_DEVICE_IGNORE_CARRIER, device, &has_match); if (has_match) - return nm_config_parse_boolean (value, FALSE); + m = nm_config_parse_boolean (value, -1); + else + m = nm_device_spec_match_list_full (device, NM_CONFIG_DATA_GET_PRIVATE (self)->ignore_carrier, -1); - return nm_device_spec_match_list (device, NM_CONFIG_DATA_GET_PRIVATE (self)->ignore_carrier); + if (NM_IN_SET (m, TRUE, FALSE)) + return m; + + /* if ignore-carrier is not explicitly configed, then it depends on the device (type). */ + return nm_device_ignore_carrier_by_default (device); } gboolean From e9a917d61922a6d98ab4ae97070292fbdb33a90b Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 2 Jun 2017 00:01:26 +0200 Subject: [PATCH 2/4] device: refactor how master device's set is_available() Previously, master device types like bridge, bond, and team would overwrite is_available() and check_connection_available() and always return TRUE. The device already expresses via nm_device_is_master() that it is of a master kind. Refactor the code, so, instead of having these device types overwrite is_available() and check_connection_available(), let the parents implementation react on nm_device_is_master(). There is no change in behavior at all. Instead, the knowledge how to treat a master device moves from the device implementation to the parent class. --- src/devices/nm-device-bond.c | 20 -------------------- src/devices/nm-device-bridge.c | 13 +++---------- src/devices/nm-device.c | 13 ++++++++++++- src/devices/team/nm-device-team.c | 20 -------------------- 4 files changed, 15 insertions(+), 51 deletions(-) diff --git a/src/devices/nm-device-bond.c b/src/devices/nm-device-bond.c index 73ce460a2e..f1db8eeacc 100644 --- a/src/devices/nm-device-bond.c +++ b/src/devices/nm-device-bond.c @@ -57,24 +57,6 @@ get_generic_capabilities (NMDevice *dev) return NM_DEVICE_CAP_CARRIER_DETECT | NM_DEVICE_CAP_IS_SOFTWARE; } -static gboolean -is_available (NMDevice *dev, NMDeviceCheckDevAvailableFlags flags) -{ - return TRUE; -} - -static gboolean -check_connection_available (NMDevice *device, - NMConnection *connection, - NMDeviceCheckConAvailableFlags flags, - const char *specific_object) -{ - /* Connections are always available because the carrier state is determined - * by the slave carrier states, not the bonds's state. - */ - return TRUE; -} - static gboolean check_connection_compatible (NMDevice *device, NMConnection *connection) { @@ -631,9 +613,7 @@ nm_device_bond_class_init (NMDeviceBondClass *klass) parent_class->is_master = TRUE; parent_class->get_generic_capabilities = get_generic_capabilities; - parent_class->is_available = is_available; parent_class->check_connection_compatible = check_connection_compatible; - parent_class->check_connection_available = check_connection_available; parent_class->complete_connection = complete_connection; parent_class->update_connection = update_connection; diff --git a/src/devices/nm-device-bridge.c b/src/devices/nm-device-bridge.c index 58400211b2..90950fdfcb 100644 --- a/src/devices/nm-device-bridge.c +++ b/src/devices/nm-device-bridge.c @@ -59,12 +59,6 @@ get_generic_capabilities (NMDevice *dev) return NM_DEVICE_CAP_CARRIER_DETECT | NM_DEVICE_CAP_IS_SOFTWARE; } -static gboolean -is_available (NMDevice *dev, NMDeviceCheckDevAvailableFlags flags) -{ - return TRUE; -} - static gboolean check_connection_available (NMDevice *device, NMConnection *connection, @@ -73,6 +67,9 @@ check_connection_available (NMDevice *device, { NMSettingBluetooth *s_bt; + if (!NM_DEVICE_CLASS (nm_device_bridge_parent_class)->check_connection_available (device, connection, flags, specific_object)) + return FALSE; + s_bt = _nm_connection_get_setting_bluetooth_for_nap (connection); if (s_bt) { return nm_bt_vtable_network_server @@ -80,9 +77,6 @@ check_connection_available (NMDevice *device, nm_setting_bluetooth_get_bdaddr (s_bt)); } - /* Connections are always available because the carrier state is determined - * by the bridge port carrier states, not the bridge's state. - */ return TRUE; } @@ -490,7 +484,6 @@ nm_device_bridge_class_init (NMDeviceBridgeClass *klass) parent_class->is_master = TRUE; parent_class->get_generic_capabilities = get_generic_capabilities; - parent_class->is_available = is_available; parent_class->check_connection_compatible = check_connection_compatible; parent_class->check_connection_available = check_connection_available; parent_class->complete_connection = complete_connection; diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index cc2e39f68b..c3469a8895 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -3790,12 +3790,17 @@ is_available (NMDevice *self, NMDeviceCheckDevAvailableFlags flags) { NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE (self); - if (priv->carrier || priv->ignore_carrier) + if ( priv->carrier + || priv->ignore_carrier) return TRUE; if (NM_FLAGS_HAS (flags, _NM_DEVICE_CHECK_DEV_AVAILABLE_IGNORE_CARRIER)) return TRUE; + /* master types are always available even without carrier. */ + if (nm_device_is_master (self)) + return TRUE; + return FALSE; } @@ -11681,6 +11686,12 @@ check_connection_available (NMDevice *self, return TRUE; } + /* master types are always available even without carrier. + * Making connection non-available would un-enslave slaves which + * is not desired. */ + if (nm_device_is_master (self)) + return TRUE; + return FALSE; } diff --git a/src/devices/team/nm-device-team.c b/src/devices/team/nm-device-team.c index 2986664105..aeb39ebf9e 100644 --- a/src/devices/team/nm-device-team.c +++ b/src/devices/team/nm-device-team.c @@ -83,24 +83,6 @@ get_generic_capabilities (NMDevice *device) return NM_DEVICE_CAP_CARRIER_DETECT | NM_DEVICE_CAP_IS_SOFTWARE; } -static gboolean -is_available (NMDevice *device, NMDeviceCheckDevAvailableFlags flags) -{ - return TRUE; -} - -static gboolean -check_connection_available (NMDevice *device, - NMConnection *connection, - NMDeviceCheckConAvailableFlags flags, - const char *specific_object) -{ - /* Connections are always available because the carrier state is determined - * by the team port carrier states, not the team's state. - */ - return TRUE; -} - static gboolean check_connection_compatible (NMDevice *device, NMConnection *connection) { @@ -890,9 +872,7 @@ nm_device_team_class_init (NMDeviceTeamClass *klass) parent_class->is_master = TRUE; parent_class->create_and_realize = create_and_realize; parent_class->get_generic_capabilities = get_generic_capabilities; - parent_class->is_available = is_available; parent_class->check_connection_compatible = check_connection_compatible; - parent_class->check_connection_available = check_connection_available; parent_class->complete_connection = complete_connection; parent_class->update_connection = update_connection; parent_class->master_update_slave_connection = master_update_slave_connection; From b0f6baad90a3d8b571a56cc255ad49d9fa26d874 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 2 Jun 2017 11:11:50 +0200 Subject: [PATCH 3/4] device: renew IP addressing on carrier change also for master devices Commit 348452f1e06e9bde9f84b90db4f5620ee672389a (device: renew DHCP lease for active "ignore-carrier" devices on carrier-on (bgo #743368)) added this behavior for non-master devices. The same reasoning applies here too. https://github.com/NetworkManager/NetworkManager/pull/18 Based-on-patch-by: Nikolay Martynov --- src/devices/nm-device.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index c3469a8895..866879c2fe 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -2253,10 +2253,16 @@ carrier_changed (NMDevice *self, gboolean carrier) if (!carrier) return; - if (nm_device_activate_ip4_state_in_wait (self)) - nm_device_activate_stage3_ip4_start (self); - if (nm_device_activate_ip6_state_in_wait (self)) - nm_device_activate_stage3_ip6_start (self); + /* Force master to retry getting ip addresses when carrier + * is restored. */ + if (priv->state == NM_DEVICE_STATE_ACTIVATED) + nm_device_update_dynamic_ip_setup (self); + else { + if (nm_device_activate_ip4_state_in_wait (self)) + nm_device_activate_stage3_ip4_start (self); + if (nm_device_activate_ip6_state_in_wait (self)) + nm_device_activate_stage3_ip6_start (self); + } return; } else if (priv->is_enslaved && !carrier) { From 8c91422954eceb96b0cba9f1e2b0e1d3ad8e53cd Mon Sep 17 00:00:00 2001 From: Nikolay Martynov Date: Thu, 1 Jun 2017 23:57:03 +0200 Subject: [PATCH 4/4] device: handle carrier changes for master device differently For master devices, instead of ignoring loss of carrier entirely, handle it. First of all, master devices are now by default ignore-carrier=yes. That means, without explict user configuration in NetworkManager.conf, the previous behavior in carrier_changed() does not change. If the user decides to configure the master device like [device-with-carrier] match-device=type:bond,type:bridge,type:team ignore-carrier=no then, master device will disconnect on carrier loss like regular devices. https://github.com/NetworkManager/NetworkManager/pull/18 Co-authored-by: Thomas Haller --- man/NetworkManager.conf.xml | 9 ++++++++- src/devices/nm-device.c | 34 ++++++++++++++-------------------- 2 files changed, 22 insertions(+), 21 deletions(-) diff --git a/man/NetworkManager.conf.xml b/man/NetworkManager.conf.xml index 0dc1b700d9..d8fa1b3b95 100644 --- a/man/NetworkManager.conf.xml +++ b/man/NetworkManager.conf.xml @@ -233,7 +233,10 @@ no-auto-default=* if specified (See ). Otherwise, it is a list of matches to specify for which device carrier should be ignored. See for the - syntax how to specify a device. + syntax how to specify a device. Note that master types like + bond, bridge, and team ignore carrier by default. You can however + revert that default using the "except:" specifier (or better, + use the per-device setting instead of the deprecated setting). @@ -839,6 +842,10 @@ unmanaged=1 interfaces will still reflect the actual device state; it's just that NetworkManager will not make use of that information. + + Master types like bond, bridge and team ignore carrier by default, + while other device types react on carrier changes by default. + This setting overwrites the deprecated main.ignore-carrier setting above. diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index 866879c2fe..3abf46cb10 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -2246,25 +2246,20 @@ carrier_changed (NMDevice *self, gboolean carrier) return; if (nm_device_is_master (self)) { - /* Bridge/bond/team carrier does not affect its own activation, - * but when carrier comes on, if there are slaves waiting, - * it will restart them. - */ - if (!carrier) + if (carrier) { + /* Force master to retry getting ip addresses when carrier + * is restored. */ + if (priv->state == NM_DEVICE_STATE_ACTIVATED) + nm_device_update_dynamic_ip_setup (self); + else { + if (nm_device_activate_ip4_state_in_wait (self)) + nm_device_activate_stage3_ip4_start (self); + if (nm_device_activate_ip6_state_in_wait (self)) + nm_device_activate_stage3_ip6_start (self); + } return; - - /* Force master to retry getting ip addresses when carrier - * is restored. */ - if (priv->state == NM_DEVICE_STATE_ACTIVATED) - nm_device_update_dynamic_ip_setup (self); - else { - if (nm_device_activate_ip4_state_in_wait (self)) - nm_device_activate_stage3_ip4_start (self); - if (nm_device_activate_ip6_state_in_wait (self)) - nm_device_activate_stage3_ip6_start (self); } - - return; + /* fall-through and change state of device */ } else if (priv->is_enslaved && !carrier) { /* Slaves don't deactivate when they lose carrier; for * bonds/teams in particular that would be actively @@ -3843,9 +3838,8 @@ nm_device_is_available (NMDevice *self, NMDeviceCheckDevAvailableFlags flags) gboolean nm_device_ignore_carrier_by_default (NMDevice *self) { - g_return_val_if_fail (NM_IS_DEVICE (self), FALSE); - - return FALSE; + /* master types ignore-carrier by default. */ + return nm_device_is_master (self); } gboolean