/bin/conman: Run ifplugd script for eth0.
ifplugd reacts to changes in ethernet state, but doesn't run the
ifplugd.action script for the link's initial state when the system
boots. This CL changes conman to trigger this when it starts up, if
it hasn't already run, and includes some associated cleanup and test
changes.
Also adds some debug logging, fixes a small bug that would have
prevented temporary connection check routes from being removed, and
fixes another small bug in the test implementation of 'ip route' which
could have masked this first bug.
Finally, some confusing code in Interface.update_routes was slightly
refactored for readability.
Change-Id: Ib18e2de1121f5382af5c6d2de9cbb46ac5f1be25
diff --git a/conman/connection_manager.py b/conman/connection_manager.py
index 9bbfb78..c484d4a 100755
--- a/conman/connection_manager.py
+++ b/conman/connection_manager.py
@@ -214,11 +214,11 @@
# explicitly.
if os.path.exists(os.path.join(self._interface_status_dir,
self.ETHERNET_STATUS_FILE)):
- self._ethernet_file_exists = True
self._process_file(self._interface_status_dir, self.ETHERNET_STATUS_FILE)
else:
- self._ethernet_file_exists = False
- self.bridge.ethernet = self.is_ethernet_up()
+ ethernet_up = self.is_ethernet_up()
+ self.bridge.ethernet = ethernet_up
+ self.ifplugd_action('eth0', ethernet_up)
for path, prefix in ((self._moca_status_dir, self.MOCA_NODE_FILE_PREFIX),
(self._status_dir, self.COMMAND_FILE_PREFIX),
@@ -402,11 +402,6 @@
return self.bridge.internet() or any(wifi.internet() for wifi in self.wifi)
def _update_interfaces_and_routes(self):
- # If ifplugd has never written an ethernet status file, poll manually before
- # updating routes.
- if not self._ethernet_file_exists:
- self.bridge.ethernet = self.is_ethernet_up()
-
self.bridge.update_routes()
for wifi in self.wifi:
wifi.update_routes()
@@ -457,7 +452,6 @@
if filename == self.ETHERNET_STATUS_FILE:
try:
self.bridge.ethernet = bool(int(contents))
- self._ethernet_file_exists = True
except ValueError:
logging.error('Status file contents should be 0 or 1, not %s',
contents)
@@ -502,15 +496,16 @@
self.bridge.add_moca_station(node)
has_moca = self.bridge.moca
if had_moca != has_moca:
- self.tell_ifplugd_about_moca(has_moca)
+ self.ifplugd_action('moca0', has_moca)
def interface_by_name(self, interface_name):
for ifc in [self.bridge] + self.wifi:
if ifc.name == interface_name:
return ifc
- def tell_ifplugd_about_moca(self, up):
- subprocess.call(self.IFPLUGD_ACTION + ['moca0', 'up' if up else 'down'])
+ def ifplugd_action(self, interface_name, up):
+ subprocess.call(self.IFPLUGD_ACTION + [interface_name,
+ 'up' if up else 'down'])
def _wifi_scan(self, wifi):
"""Perform a wifi scan and update wifi.cycler."""
diff --git a/conman/connection_manager_test.py b/conman/connection_manager_test.py
index 5388b22..afc5c9a 100755
--- a/conman/connection_manager_test.py
+++ b/conman/connection_manager_test.py
@@ -178,6 +178,8 @@
WIFI_SETCLIENT = ['echo', 'setclient']
IFUP = ['echo', 'ifup']
IFPLUGD_ACTION = ['echo', 'ifplugd.action']
+ # This simulates the output of 'ip link' when eth0 is up.
+ IP_LINK = ['echo', 'eth0 LOWER_UP']
def __init__(self, *args, **kwargs):
super(ConnectionManager, self).__init__(*args, **kwargs)
@@ -216,8 +218,7 @@
else:
open(socket, 'w')
wifi.set_connection_check_result('restricted')
- self.write_interface_status_file(wifi.name, '1')
- self.write_gateway_file(wifi.name)
+ self.ifplugd_action(wifi.name, True)
return True
return False
@@ -251,10 +252,16 @@
super(ConnectionManager, self)._wifi_scan(wifi)
wifi.wifi_scan_counter += 1
- def tell_ifplugd_about_moca(self, up):
+ def ifplugd_action(self, interface_name, up):
# Typically, when moca comes up, conman calls ifplugd.action, which writes
- # this file.
- self.write_interface_status_file('moca0', '1' if up else '0')
+ # this file. Also, when conman starts, it calls ifplugd.action for eth0.
+ self.write_interface_status_file(interface_name, '1' if up else '0')
+
+ # ifplugd calls run-dhclient, which results in a gateway file if the link is
+ # up (and working).
+ if up:
+ self.write_gateway_file('br0' if interface_name in ('eth0', 'moca0')
+ else interface_name)
# Non-overrides
@@ -317,12 +324,7 @@
f.write(value)
def set_ethernet(self, up):
- self.write_interface_status_file('eth0', '1' if up else '0')
-
- if up:
- # On the real system, ifplugd would run dhclient, which would write a
- # gateway file.
- self.write_gateway_file('br0')
+ self.ifplugd_action('eth0', up)
def set_moca(self, up):
moca_node1_file = os.path.join(self._moca_status_dir,
@@ -331,11 +333,6 @@
f.write(FAKE_MOCA_NODE1_FILE if up else
FAKE_MOCA_NODE1_FILE_DISCONNECTED)
- if up:
- # On the real system, ifplugd would run dhclient, which would write a
- # gateway file.
- self.write_gateway_file('br0')
-
def run_until_interface_update(self):
while self._interface_update_counter == 0:
self.run_once()
@@ -407,15 +404,12 @@
c: A ConnectionManager set up by @connection_manager_test.
"""
- # Initially, no access.
- wvtest.WVFAIL(c.acs())
- wvtest.WVFAIL(c.internet())
+ # Initially, there is ethernet access (via explicit check of ethernet status,
+ # rather than the interface status file).
+ wvtest.WVPASS(c.acs())
+ wvtest.WVPASS(c.internet())
c.run_once()
-
- # Bring up ethernet, access.
- c.set_ethernet(True)
- c.run_once()
wvtest.WVPASS(c.acs())
wvtest.WVPASS(c.internet())
wvtest.WVPASS(c.bridge.current_route())
diff --git a/conman/interface.py b/conman/interface.py
index c2de318..d814950 100755
--- a/conman/interface.py
+++ b/conman/interface.py
@@ -48,10 +48,13 @@
Whether the connection is working.
"""
if not self.links:
+ logging.debug('Connection check for %s failed due to no links', self.name)
return False
- logging.debug('gateway ip for %s is %s', self.name, self._gateway_ip)
+ logging.debug('Gateway ip for %s is %s', self.name, self._gateway_ip)
if self._gateway_ip is None:
+ logging.debug('Connection check for %s failed due to no gateway IP',
+ self.name)
return False
# Temporarily add a route to make sure the connection check can be run.
@@ -65,6 +68,7 @@
'via', self._gateway_ip,
'dev', self.name,
'metric', str(METRIC_TEMPORARY_CONNECTION_CHECK))
+ added_temporary_route = True
cmd = [self.CONNECTION_CHECK, '-I', self.name]
if check_acs:
@@ -72,6 +76,10 @@
with open(os.devnull, 'w') as devnull:
result = subprocess.call(cmd, stdout=devnull, stderr=devnull) == 0
+ logging.debug('Connection check%s for %s %s',
+ ' (ACS)' if check_acs else '',
+ self.name,
+ 'passed' if result else 'failed')
# Delete the temporary route.
if added_temporary_route:
@@ -205,22 +213,27 @@
deciding whether to add or remove routes.
"""
logging.debug('Updating routes for %s', self.name)
- # We care about the distinction between None (unknown) and False (known
- # inaccessible) here.
- # pylint: disable=g-explicit-bool-comparison
- maybe_had_acs = self._has_acs != False
- maybe_had_internet = self._has_internet != False
+ maybe_had_acs = self._has_acs
+ maybe_had_internet = self._has_internet
if expire_cache:
self.expire_connection_status_cache()
has_acs = self.acs()
has_internet = self.internet()
- if ((not maybe_had_acs and has_acs) or
- (not maybe_had_internet and has_internet)):
+
+ # This is a little confusing: We want to try adding a route if we _may_
+ # have gone from no access to some access, and we want to try deleting the
+ # route if we _may_ have lost *all* access. So the first condition checks
+ # for truthiness but the elif checks for explicit Falsity (i.e. excluding
+ # the None/unknown case).
+ had_access = maybe_had_acs or maybe_had_internet
+ # pylint: disable=g-explicit-bool-comparison
+ maybe_had_access = maybe_had_acs != False or maybe_had_internet != False
+ has_access = has_acs or has_internet
+ if not had_access and has_access:
self.add_route()
- elif ((maybe_had_acs or maybe_had_internet) and not
- (has_acs or has_internet)):
+ elif maybe_had_access and not has_access:
self.delete_route()
diff --git a/conman/interface_test.py b/conman/interface_test.py
index edc3541..9905fea 100755
--- a/conman/interface_test.py
+++ b/conman/interface_test.py
@@ -54,6 +54,7 @@
if k[0] == key[0]:
logging.debug('Deleting route for %r (generalized from %s)', k, key)
del self.routing_table[k]
+ break
class Bridge(FakeInterfaceMixin, interface.Bridge):
