From bd0d471c8f127972b2a02dcd5a47b3634baaa1d7 Mon Sep 17 00:00:00 2001 From: Dan Streetman Date: Mon, 27 Apr 2020 06:38:40 -0400 Subject: [PATCH 1/4] network: Change IgnoreCarrierLoss default to value of ConfigureWithoutCarrier It doesn't make much sense to have ConfigureWithoutCarrier set, but not IgnoreCarrierLoss; all the configuration added during initial interface bring-up will be lost at the first carrier up/down. --- man/systemd.network.xml | 8 +++++--- src/network/networkd-network-gperf.gperf | 2 +- src/network/networkd-network.c | 5 +++++ src/network/networkd-network.h | 2 +- 4 files changed, 12 insertions(+), 5 deletions(-) diff --git a/man/systemd.network.xml b/man/systemd.network.xml index f9e8fa5c25..2de59f2d7d 100644 --- a/man/systemd.network.xml +++ b/man/systemd.network.xml @@ -828,15 +828,17 @@ ConfigureWithoutCarrier= Takes a boolean. Allows networkd to configure a specific link even if it has no carrier. - Defaults to false. + Defaults to false. If is not explicitly set, it will + default to this value. IgnoreCarrierLoss= - A boolean. Allows networkd to retain both the static and dynamic configuration of the - interface even if its carrier is lost. Defaults to false. + Takes a boolean. Allows networkd to retain both the static and dynamic configuration + of the interface even if its carrier is lost. When unset, the value specified with + is used. diff --git a/src/network/networkd-network-gperf.gperf b/src/network/networkd-network-gperf.gperf index a2dd5479b2..6f9d7dead7 100644 --- a/src/network/networkd-network-gperf.gperf +++ b/src/network/networkd-network-gperf.gperf @@ -102,7 +102,7 @@ Network.ProxyARP, config_parse_tristate, Network.IPv6ProxyNDPAddress, config_parse_ipv6_proxy_ndp_address, 0, 0 Network.BindCarrier, config_parse_strv, 0, offsetof(Network, bind_carrier) Network.ConfigureWithoutCarrier, config_parse_bool, 0, offsetof(Network, configure_without_carrier) -Network.IgnoreCarrierLoss, config_parse_bool, 0, offsetof(Network, ignore_carrier_loss) +Network.IgnoreCarrierLoss, config_parse_tristate, 0, offsetof(Network, ignore_carrier_loss) Network.KeepConfiguration, config_parse_keep_configuration, 0, offsetof(Network, keep_configuration) Address.Address, config_parse_address, 0, 0 Address.Peer, config_parse_address, 0, 0 diff --git a/src/network/networkd-network.c b/src/network/networkd-network.c index 1b4083febf..74e383b540 100644 --- a/src/network/networkd-network.c +++ b/src/network/networkd-network.c @@ -271,6 +271,9 @@ int network_verify(Network *network) { if (network->dhcp_use_gateway < 0) network->dhcp_use_gateway = network->dhcp_use_routes; + if (network->ignore_carrier_loss < 0) + network->ignore_carrier_loss = network->configure_without_carrier; + if (network->dhcp_critical >= 0) { if (network->keep_configuration >= 0) log_warning("%s: Both KeepConfiguration= and deprecated CriticalConnection= are set. " @@ -460,6 +463,8 @@ int network_load_one(Manager *manager, OrderedHashmap **networks, const char *fi .ipv6_accept_ra_route_table_set = false, .ipv6_accept_ra_start_dhcp6_client = true, + .configure_without_carrier = false, + .ignore_carrier_loss = -1, .keep_configuration = _KEEP_CONFIGURATION_INVALID, .ipv6_address_gen_mode = _LINK_IPV6_ADDRESS_GEN_MODE_INVALID, .can_triple_sampling = -1, diff --git a/src/network/networkd-network.h b/src/network/networkd-network.h index 362ef6508f..912ecd0a2e 100644 --- a/src/network/networkd-network.h +++ b/src/network/networkd-network.h @@ -257,7 +257,7 @@ struct Network { int allmulticast; bool unmanaged; bool configure_without_carrier; - bool ignore_carrier_loss; + int ignore_carrier_loss; KeepConfiguration keep_configuration; LinkIPv6AddressGenMode ipv6_address_gen_mode; uint32_t iaid; From 6b8a1aa6a37b59565e400a379a27ac60ce59f895 Mon Sep 17 00:00:00 2001 From: Dan Streetman Date: Sat, 2 May 2020 09:56:33 -0400 Subject: [PATCH 2/4] network: return 1 on start and 0 if ipv4ll is already started Instead of -EBUSY, return 0 from sd_ipv4ll_start() if it's already started, and change successful start return value to 1. This matches sd_ndisc_start() behavior; 1 indicates successful start, and 0 indicates already started. --- src/libsystemd-network/sd-ipv4ll.c | 6 ++++-- src/libsystemd-network/test-ipv4ll.c | 4 ++-- src/libsystemd-network/test-ndisc-rs.c | 1 + 3 files changed, 7 insertions(+), 4 deletions(-) diff --git a/src/libsystemd-network/sd-ipv4ll.c b/src/libsystemd-network/sd-ipv4ll.c index aa1ece41d2..4f4d9ebe87 100644 --- a/src/libsystemd-network/sd-ipv4ll.c +++ b/src/libsystemd-network/sd-ipv4ll.c @@ -252,12 +252,14 @@ static int ipv4ll_start_internal(sd_ipv4ll *ll, bool reset_generation) { return r; } - return 0; + return 1; } int sd_ipv4ll_start(sd_ipv4ll *ll) { assert_return(ll, -EINVAL); - assert_return(sd_ipv4ll_is_running(ll) == 0, -EBUSY); + + if (sd_ipv4ll_is_running(ll)) + return 0; return ipv4ll_start_internal(ll, true); } diff --git a/src/libsystemd-network/test-ipv4ll.c b/src/libsystemd-network/test-ipv4ll.c index 2f319bf7d9..6051e65820 100644 --- a/src/libsystemd-network/test-ipv4ll.c +++ b/src/libsystemd-network/test-ipv4ll.c @@ -159,10 +159,10 @@ static void test_basic_request(sd_event *e) { assert_se(sd_ipv4ll_start(ll) == -EINVAL); assert_se(sd_ipv4ll_set_ifindex(ll, 1) == 0); - assert_se(sd_ipv4ll_start(ll) == 0); + assert_se(sd_ipv4ll_start(ll) == 1); sd_event_run(e, (uint64_t) -1); - assert_se(sd_ipv4ll_start(ll) == -EBUSY); + assert_se(sd_ipv4ll_start(ll) == 0); assert_se(sd_ipv4ll_is_running(ll)); diff --git a/src/libsystemd-network/test-ndisc-rs.c b/src/libsystemd-network/test-ndisc-rs.c index c8ee1ec31d..9fc902645c 100644 --- a/src/libsystemd-network/test-ndisc-rs.c +++ b/src/libsystemd-network/test-ndisc-rs.c @@ -297,6 +297,7 @@ static void test_rs(void) { assert_se(sd_ndisc_stop(nd) >= 0); assert_se(sd_ndisc_start(nd) >= 0); + assert_se(sd_ndisc_start(nd) >= 0); assert_se(sd_ndisc_stop(nd) >= 0); assert_se(sd_ndisc_start(nd) >= 0); From 9bacf4313eb2f54fd6d9affce10b027e80ec50ef Mon Sep 17 00:00:00 2001 From: Dan Streetman Date: Mon, 27 Apr 2020 15:35:30 -0400 Subject: [PATCH 3/4] test: expand configure-without-carrier test The test currently doesn't actually test configure-without-carrier since it does have carrier for the entire test. It now forces carrier down before starting the network portion of the test. Also, it tests to verify the configuration is retained across future carrier losses/gains. --- ...thout-carrier.network => 25-test1.network} | 1 - .../configure-without-carrier.conf | 2 + .../ignore-carrier-loss-no.conf | 2 + test/test-network/systemd-networkd-tests.py | 57 ++++++++++++++++--- 4 files changed, 53 insertions(+), 9 deletions(-) rename test/test-network/conf/{configure-without-carrier.network => 25-test1.network} (71%) create mode 100644 test/test-network/conf/25-test1.network.d/configure-without-carrier.conf create mode 100644 test/test-network/conf/25-test1.network.d/ignore-carrier-loss-no.conf diff --git a/test/test-network/conf/configure-without-carrier.network b/test/test-network/conf/25-test1.network similarity index 71% rename from test/test-network/conf/configure-without-carrier.network rename to test/test-network/conf/25-test1.network index 5bd9d7e84a..965013f558 100644 --- a/test/test-network/conf/configure-without-carrier.network +++ b/test/test-network/conf/25-test1.network @@ -4,4 +4,3 @@ Name=test1 [Network] Address=192.168.0.15/24 Gateway=192.168.0.1 -ConfigureWithoutCarrier=true diff --git a/test/test-network/conf/25-test1.network.d/configure-without-carrier.conf b/test/test-network/conf/25-test1.network.d/configure-without-carrier.conf new file mode 100644 index 0000000000..fed3b0eef3 --- /dev/null +++ b/test/test-network/conf/25-test1.network.d/configure-without-carrier.conf @@ -0,0 +1,2 @@ +[Network] +ConfigureWithoutCarrier=true diff --git a/test/test-network/conf/25-test1.network.d/ignore-carrier-loss-no.conf b/test/test-network/conf/25-test1.network.d/ignore-carrier-loss-no.conf new file mode 100644 index 0000000000..b091e98fc4 --- /dev/null +++ b/test/test-network/conf/25-test1.network.d/ignore-carrier-loss-no.conf @@ -0,0 +1,2 @@ +[Network] +IgnoreCarrierLoss=false diff --git a/test/test-network/systemd-networkd-tests.py b/test/test-network/systemd-networkd-tests.py index 3fc93a725f..7f9737f809 100755 --- a/test/test-network/systemd-networkd-tests.py +++ b/test/test-network/systemd-networkd-tests.py @@ -1671,11 +1671,11 @@ class NetworkdNetworkTests(unittest.TestCase, Utilities): '25-gateway-next-static.network', '25-sysctl-disable-ipv6.network', '25-sysctl.network', + '25-test1.network', '25-veth-peer.network', '25-veth.netdev', '25-vrf.netdev', '26-link-local-addressing-ipv6.network', - 'configure-without-carrier.network', 'routing-policy-rule-dummy98.network', 'routing-policy-rule-test1.network'] @@ -1762,15 +1762,56 @@ class NetworkdNetworkTests(unittest.TestCase, Utilities): self.assertNotRegex(output, '192.168.100.10/24') def test_configure_without_carrier(self): - copy_unit_to_networkd_unit_path('configure-without-carrier.network', '11-dummy.netdev') + copy_unit_to_networkd_unit_path('11-dummy.netdev') start_networkd() - self.wait_online(['test1:routable']) + self.wait_operstate('test1', 'off', '') + check_output('ip link set dev test1 up carrier off') - output = check_output(*networkctl_cmd, '-n', '0', 'status', 'test1', env=env) - print(output) - self.assertRegex(output, '192.168.0.15') - self.assertRegex(output, '192.168.0.1') - self.assertRegex(output, 'routable') + copy_unit_to_networkd_unit_path('25-test1.network.d/configure-without-carrier.conf', dropins=False) + restart_networkd() + self.wait_online(['test1:no-carrier']) + + carrier_map = {'on': '1', 'off': '0'} + routable_map = {'on': 'routable', 'off': 'no-carrier'} + for carrier in ['off', 'on', 'off']: + with self.subTest(carrier=carrier): + if carrier_map[carrier] != read_link_attr('test1', 'carrier'): + check_output(f'ip link set dev test1 carrier {carrier}') + self.wait_online([f'test1:{routable_map[carrier]}']) + + output = check_output(*networkctl_cmd, '-n', '0', 'status', 'test1', env=env) + print(output) + self.assertRegex(output, '192.168.0.15') + self.assertRegex(output, '192.168.0.1') + self.assertRegex(output, routable_map[carrier]) + + def test_configure_without_carrier_yes_ignore_carrier_loss_no(self): + copy_unit_to_networkd_unit_path('11-dummy.netdev') + start_networkd() + self.wait_operstate('test1', 'off', '') + check_output('ip link set dev test1 up carrier off') + + copy_unit_to_networkd_unit_path('25-test1.network') + restart_networkd() + self.wait_online(['test1:no-carrier']) + + carrier_map = {'on': '1', 'off': '0'} + routable_map = {'on': 'routable', 'off': 'no-carrier'} + for (carrier, have_config) in [('off', True), ('on', True), ('off', False)]: + with self.subTest(carrier=carrier, have_config=have_config): + if carrier_map[carrier] != read_link_attr('test1', 'carrier'): + check_output(f'ip link set dev test1 carrier {carrier}') + self.wait_online([f'test1:{routable_map[carrier]}']) + + output = check_output(*networkctl_cmd, '-n', '0', 'status', 'test1', env=env) + print(output) + if have_config: + self.assertRegex(output, '192.168.0.15') + self.assertRegex(output, '192.168.0.1') + else: + self.assertNotRegex(output, '192.168.0.15') + self.assertNotRegex(output, '192.168.0.1') + self.assertRegex(output, routable_map[carrier]) def test_routing_policy_rule(self): copy_unit_to_networkd_unit_path('routing-policy-rule-test1.network', '11-dummy.netdev') From 0fc0d85faea8909e7af9d227a31c1f11546c5313 Mon Sep 17 00:00:00 2001 From: Dan Streetman Date: Mon, 27 Apr 2020 15:47:28 -0400 Subject: [PATCH 4/4] test: add bridge configure-without-carrier test Verify configure-without-carrier works, and retains configuration across carrier losses/gains. --- ...6-bridge-configure-without-carrier.network | 9 ++++ test/test-network/systemd-networkd-tests.py | 53 +++++++++++++++++++ 2 files changed, 62 insertions(+) create mode 100644 test/test-network/conf/26-bridge-configure-without-carrier.network diff --git a/test/test-network/conf/26-bridge-configure-without-carrier.network b/test/test-network/conf/26-bridge-configure-without-carrier.network new file mode 100644 index 0000000000..e1196b866b --- /dev/null +++ b/test/test-network/conf/26-bridge-configure-without-carrier.network @@ -0,0 +1,9 @@ +[Match] +Name=bridge99 + +[Network] +LinkLocalAddressing=yes +IPv6AcceptRA=no +ConfigureWithoutCarrier=yes +Address=10.1.2.3/24 +Gateway=10.1.2.1 diff --git a/test/test-network/systemd-networkd-tests.py b/test/test-network/systemd-networkd-tests.py index 7f9737f809..6dbc2b0c4f 100755 --- a/test/test-network/systemd-networkd-tests.py +++ b/test/test-network/systemd-networkd-tests.py @@ -445,6 +445,9 @@ class Utilities(): def check_link_exists(self, link): self.assertTrue(link_exists(link)) + def check_link_attr(self, *args): + self.assertEqual(read_link_attr(*args[:-1]), args[-1]); + def wait_operstate(self, link, operstate='degraded', setup_state='configured', setup_timeout=5, fail_assert=True): """Wait for the link to reach the specified operstate and/or setup state. @@ -2649,6 +2652,7 @@ class NetworkdBridgeTests(unittest.TestCase, Utilities): '11-dummy.netdev', '12-dummy.netdev', '26-bridge.netdev', + '26-bridge-configure-without-carrier.network', '26-bridge-slave-interface-1.network', '26-bridge-slave-interface-2.network', '26-bridge-vlan-master.network', @@ -2763,6 +2767,55 @@ class NetworkdBridgeTests(unittest.TestCase, Utilities): print(output) self.assertRegex(output, 'ff00::/8 table local metric 256 (linkdown )?pref medium') + def test_bridge_configure_without_carrier(self): + copy_unit_to_networkd_unit_path('26-bridge.netdev', '26-bridge-configure-without-carrier.network', + '11-dummy.netdev') + start_networkd() + + # With ConfigureWithoutCarrier=yes, the bridge should remain configured for all these situations + for test in ['no-slave', 'add-slave', 'slave-up', 'slave-no-carrier', 'slave-carrier', 'slave-down']: + with self.subTest(test=test): + if test == 'no-slave': + # bridge has no slaves; it's up but *might* not have carrier + self.wait_online(['bridge99:no-carrier']) + # due to a bug in the kernel, newly-created bridges are brought up + # *with* carrier, unless they have had any setting changed; e.g. + # their mac set, priority set, etc. Then, they will lose carrier + # as soon as a (down) slave interface is added, and regain carrier + # again once the slave interface is brought up. + #self.check_link_attr('bridge99', 'carrier', '0') + elif test == 'add-slave': + # add slave to bridge, but leave it down; bridge is definitely no-carrier + self.check_link_attr('test1', 'operstate', 'down') + check_output('ip link set dev test1 master bridge99') + self.wait_online(['bridge99:no-carrier:no-carrier']) + self.check_link_attr('bridge99', 'carrier', '0') + elif test == 'slave-up': + # bring up slave, which will have carrier; bridge gains carrier + check_output('ip link set dev test1 up') + self.wait_online(['bridge99:routable']) + self.check_link_attr('bridge99', 'carrier', '1') + elif test == 'slave-no-carrier': + # drop slave carrier; bridge loses carrier + check_output('ip link set dev test1 carrier off') + self.wait_online(['bridge99:no-carrier:no-carrier']) + self.check_link_attr('bridge99', 'carrier', '0') + elif test == 'slave-carrier': + # restore slave carrier; bridge gains carrier + check_output('ip link set dev test1 carrier on') + self.wait_online(['bridge99:routable']) + self.check_link_attr('bridge99', 'carrier', '1') + elif test == 'slave-down': + # bring down slave; bridge loses carrier + check_output('ip link set dev test1 down') + self.wait_online(['bridge99:no-carrier:no-carrier']) + self.check_link_attr('bridge99', 'carrier', '0') + + output = check_output(*networkctl_cmd, '-n', '0', 'status', 'bridge99', env=env) + print(output) + self.assertRegex(output, '10.1.2.3') + self.assertRegex(output, '10.1.2.1') + def test_bridge_ignore_carrier_loss(self): copy_unit_to_networkd_unit_path('11-dummy.netdev', '12-dummy.netdev', '26-bridge.netdev', '26-bridge-slave-interface-1.network', '26-bridge-slave-interface-2.network',