All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 1/4] test-runner: Support running hostapd in namespaces
@ 2022-07-07 20:03 Andrew Zaborowski
  2022-07-07 20:03 ` [PATCH v2 2/4] autotests: DHCPv4 renewal/resend test in testNetconfig Andrew Zaborowski
                   ` (3 more replies)
  0 siblings, 4 replies; 8+ messages in thread
From: Andrew Zaborowski @ 2022-07-07 20:03 UTC (permalink / raw)
  To: iwd

The kernel will not let us test some scenarios of communication between
two hwsim radios (e.g. STA and AP) if they're in the same net namespace.
For example, when connected, you can't add normal IPv4 subnet routes for
the same subnet on two different interfaces in one namespace (you'd
either get an EEXIST or you'd replace the other route), you can set
different metrics on the routes but that won't fix IP routing.  For
testNetconfig the result is that communication works for DHCP before we
get the inital lease but renewals won't work because they're unicast.
Allow hostapd to run on a radio that has been moved to a different
namespace in hw.conf so we don't have to work around these issues.
---
changes in v2:
 - ensure at least one hostapd session starts if the [HOSTAPD] section
   is present to mimic original behaviour and fix testEAD

 tools/run-tests | 82 ++++++++++++++++++++++++++++++++-----------------
 tools/utils.py  |  2 +-
 2 files changed, 54 insertions(+), 30 deletions(-)

diff --git a/tools/run-tests b/tools/run-tests
index 565847df..c0f230cb 100755
--- a/tools/run-tests
+++ b/tools/run-tests
@@ -58,16 +58,19 @@ def exit_vm():
 	runner.stop()
 
 class Interface:
-	def __init__(self, name, config):
+	def __init__(self, name, config, ns=None):
 		self.name = name
 		self.ctrl_interface = '/var/run/hostapd/' + name
 		self.config = config
+		self.ns = ns
 
 	def __del__(self):
-		Process(['iw', 'dev', self.name, 'del']).wait()
+		Process(['iw', 'dev', self.name, 'del'],
+				namespace=self.ns.name if self.ns else None).wait()
 
 	def set_interface_state(self, state):
-		Process(['ip', 'link', 'set', self.name, state]).wait()
+		Process(['ip', 'link', 'set', self.name, state],
+				namespace=self.ns.name if self.ns else None).wait()
 
 class Radio:
 	def __init__(self, name):
@@ -75,11 +78,16 @@ class Radio:
 		# hostapd will reset this if this radio is used by it
 		self.use = 'iwd'
 		self.interface = None
+		self.ns = None
 
 	def __del__(self):
 		print("Removing radio %s" % self.name)
 		self.interface = None
 
+	def set_namespace(self, ns):
+		self.ns = ns
+		Process(['iw', 'phy', self.name, 'set', 'netns', 'name', ns.name]).wait()
+
 	def create_interface(self, config, use):
 		global intf_id
 
@@ -87,19 +95,21 @@ class Radio:
 
 		intf_id += 1
 
-		self.interface = Interface(ifname, config)
+		self.interface = Interface(ifname, config, ns=self.ns)
 		self.use = use
 
 		Process(['iw', 'phy', self.name, 'interface', 'add', ifname,
-				'type', 'managed']).wait()
+				'type', 'managed'], namespace=self.ns.name if self.ns else None).wait()
 
 		return self.interface
 
 	def __str__(self):
 		ret = self.name + ':\n'
-		ret += '\tUsed By: %s ' % self.use
+		ret += '\tUsed By: %s' % self.use
 		if self.interface:
-			ret += '(%s)' % self.interface.name
+			ret += ' (%s)' % self.interface.name
+		if self.ns is not None:
+			ret += ' (ns=%s)' % self.ns.name
 
 		ret += '\n'
 
@@ -188,7 +198,7 @@ class Hostapd:
 		A set of running hostapd instances. This is really just a single
 		process since hostapd can be started with multiple config files.
 	'''
-	def __init__(self, radios, configs, radius):
+	def __init__(self, ns, radios, configs, radius):
 		if len(configs) != len(radios):
 			raise Exception("Config (%d) and radio (%d) list length not equal" % \
 						(len(configs), len(radios)))
@@ -198,8 +208,8 @@ class Hostapd:
 		Process(['ip', 'link', 'set', 'eth0', 'up']).wait()
 		Process(['ip', 'link', 'set', 'eth1', 'up']).wait()
 
-		self.global_ctrl_iface = '/var/run/hostapd/ctrl'
-
+		self.ns = ns
+		self.global_ctrl_iface = '/var/run/hostapd/ctrl' + (str(ns.name) if ns.name else 'main')
 		self.instances = [HostapdInstance(c, r) for c, r in zip(configs, radios)]
 
 		ifaces = [rad.interface.name for rad in radios]
@@ -227,7 +237,7 @@ class Hostapd:
 		if Process.is_verbose('hostapd'):
 			args.append('-d')
 
-		self.process = Process(args)
+		self.process = Process(args, namespace=ns.name)
 
 		self.process.wait_for_socket(self.global_ctrl_iface, 30)
 
@@ -397,32 +407,44 @@ class TestContext(Namespace):
 			#          tests. In this case you would not care what
 			#          was using each radio, just that there was
 			#          enough to run all tests.
-			nradios = 0
-			for k, _ in settings.items():
-				if k == 'radius_server':
-					continue
-				nradios += 1
-
-			hapd_radios = self.radios[:nradios]
-
+			hapd_configs = [conf for rad, conf in settings.items() if rad != 'radius_server']
+			hapd_processes = [(self, self.radios[:len(hapd_configs)], hapd_configs)]
 		else:
-			hapd_radios = [rad for rad in self.radios if rad.name in settings]
-
-		hapd_configs = [conf for rad, conf in settings.items() if rad != 'radius_server']
+			hapd_processes = []
+			for ns in [self] + self.namespaces:
+				ns_radios = [rad for rad in ns.radios if rad.name in settings]
+				if len(ns_radios):
+					ns_configs = [settings[rad.name] for rad in ns_radios]
+					hapd_processes.append((ns, ns_radios, ns_configs))
+			if not hapd_processes:
+				hapd_processes.append((self, [], []))
 
 		radius_config = settings.get('radius_server', None)
 
-		self.hostapd = Hostapd(hapd_radios, hapd_configs, radius_config)
-		self.hostapd.attach_cli()
+		self.hostapd = [Hostapd(ns, radios, configs, radius_config)
+				for ns, radios, configs in hapd_processes]
+
+		for hapd in self.hostapd:
+			hapd.attach_cli()
 
 	def get_frequencies(self):
 		frequencies = []
 
-		for hapd in self.hostapd.instances:
-			frequencies.append(hapd.cli.frequency)
+		for hapd in self.hostapd:
+			frequencies += [instance.cli.frequency for instance in hapd.instances]
 
 		return frequencies
 
+	def get_hapd_instance(self, config=None):
+		instances = [i for hapd in self.hostapd for i in hapd.instances]
+
+		if config is None:
+			return instances[0]
+
+		for hapd in instances:
+			if hapd.config == config:
+				return hapd
+
 	def start_wpas_interfaces(self):
 		if 'WPA_SUPPLICANT' not in self.hw_config:
 			return
@@ -543,11 +565,13 @@ class TestContext(Namespace):
 		for arg in vars(self.args):
 			ret += '\t --%s %s\n' % (arg, str(getattr(self.args, arg)))
 
-		ret += 'Hostapd:\n'
 		if self.hostapd:
-			for h in self.hostapd.instances:
-				ret += '\t%s\n' % str(h)
+			for hapd in self.hostapd:
+				ret += 'Hostapd (ns=%s):\n' % (hapd.ns.name,)
+				for h in hapd.instances:
+					ret += '\t%s\n' % (str(h),)
 		else:
+			ret += 'Hostapd:\n'
 			ret += '\tNo Hostapd instances\n'
 
 		info = self.meminfo_to_dict()
diff --git a/tools/utils.py b/tools/utils.py
index f3e12a85..bc030230 100644
--- a/tools/utils.py
+++ b/tools/utils.py
@@ -324,7 +324,7 @@ class Namespace:
 
 		Process(['ip', 'netns', 'add', name]).wait()
 		for r in radios:
-			Process(['iw', 'phy', r.name, 'set', 'netns', 'name', name]).wait()
+			r.set_namespace(self)
 
 		self.start_dbus()
 
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v2 2/4] autotests: DHCPv4 renewal/resend test in testNetconfig
  2022-07-07 20:03 [PATCH v2 1/4] test-runner: Support running hostapd in namespaces Andrew Zaborowski
@ 2022-07-07 20:03 ` Andrew Zaborowski
  2022-07-07 20:03 ` [PATCH v2 3/4] autotests: Also validate correct hostname sent over DHCPv4 Andrew Zaborowski
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 8+ messages in thread
From: Andrew Zaborowski @ 2022-07-07 20:03 UTC (permalink / raw)
  To: iwd

Test that the DHCPv4 lease got renewed after the T1 timer runs out.
Then also simulate the DHCPREQUEST during renew being lost and
retransmitted and the lease eventually getting renewed T1 + 60s later.

The main downside is that this test will inevitably take a while if
running in Qemu without the time travel ability.

Update the test and some utility code to run hostapd in an isolated net
namespace for connection_test.py.  We now need a second hostapd
instance though because in static_test.py we test ACD and we need to
produce an IP conflict.  Moving the hostapd instance unexpectedly fixes
dhcpd's internal mechanism to avoid IP conflicts and it would no longer
assign 192.168.1.10 to the second client, it'd notice that address was
already in use and assign the next free address, or fail if there was
none.  So add a second hostapd instance that runs in the main namespace
together with the statically-configured client, it turns out the test
relies on the kernel being unable to deliver IP traffic to interfaces on
the same system.
---
 .../{ssidTKIP.conf => ap-main.conf}           |   2 +-
 autotests/testNetconfig/ap-ns1.conf           |   7 +
 autotests/testNetconfig/connection_test.py    | 195 +++++++++++++++---
 autotests/testNetconfig/dhcpd.conf            |  14 +-
 autotests/testNetconfig/hw.conf               |   8 +-
 .../{ssidTKIP.psk => static.psk}              |   0
 autotests/testNetconfig/static_test.py        |   8 +-
 autotests/util/hostapd.py                     |   6 +-
 8 files changed, 202 insertions(+), 38 deletions(-)
 rename autotests/testNetconfig/{ssidTKIP.conf => ap-main.conf} (83%)
 create mode 100644 autotests/testNetconfig/ap-ns1.conf
 rename autotests/testNetconfig/{ssidTKIP.psk => static.psk} (100%)

diff --git a/autotests/testNetconfig/ssidTKIP.conf b/autotests/testNetconfig/ap-main.conf
similarity index 83%
rename from autotests/testNetconfig/ssidTKIP.conf
rename to autotests/testNetconfig/ap-main.conf
index 11ef15f0..0ec7fdfa 100644
--- a/autotests/testNetconfig/ssidTKIP.conf
+++ b/autotests/testNetconfig/ap-main.conf
@@ -1,6 +1,6 @@
 hw_mode=g
 channel=1
-ssid=ssidTKIP
+ssid=ap-main
 
 wpa=1
 wpa_pairwise=TKIP
diff --git a/autotests/testNetconfig/ap-ns1.conf b/autotests/testNetconfig/ap-ns1.conf
new file mode 100644
index 00000000..5459173f
--- /dev/null
+++ b/autotests/testNetconfig/ap-ns1.conf
@@ -0,0 +1,7 @@
+hw_mode=g
+channel=1
+ssid=ap-ns1
+
+wpa=1
+wpa_pairwise=TKIP
+wpa_passphrase=secret123
diff --git a/autotests/testNetconfig/connection_test.py b/autotests/testNetconfig/connection_test.py
index 5481e624..aebb0a2e 100644
--- a/autotests/testNetconfig/connection_test.py
+++ b/autotests/testNetconfig/connection_test.py
@@ -13,6 +13,7 @@ import testutil
 from config import ctx
 import os
 import socket
+import datetime, time
 
 class Test(unittest.TestCase):
 
@@ -27,6 +28,14 @@ class Test(unittest.TestCase):
 
             return True
 
+        def get_ll_addrs6(ns, ifname):
+            show_ip = ns.start_process(['ip', 'addr', 'show', ifname])
+            show_ip.wait()
+            for l in show_ip.out.split('\n'):
+                if 'inet6 fe80::' in l:
+                    return socket.inet_pton(socket.AF_INET6, l.split(None, 1)[1].split('/', 1)[0])
+            return None
+
         wd = IWD(True)
 
         psk_agent = PSKAgent("secret123")
@@ -35,7 +44,7 @@ class Test(unittest.TestCase):
         devices = wd.list_devices(1)
         device = devices[0]
 
-        ordered_network = device.get_ordered_network('ssidTKIP')
+        ordered_network = device.get_ordered_network('ap-ns1')
 
         self.assertEqual(ordered_network.type, NetworkType.psk)
 
@@ -46,18 +55,21 @@ class Test(unittest.TestCase):
 
         condition = 'obj.state == DeviceState.connected'
         wd.wait_for_object_condition(device, condition)
+        connect_time = time.time()
 
         testutil.test_iface_operstate()
-        testutil.test_ifaces_connected()
 
         testutil.test_ip_address_match(device.name, '192.168.1.10', 17, 24)
         ctx.non_block_wait(check_addr, 10, device,
                             exception=Exception("IPv6 address was not set"))
 
+        # Cannot use test_ifaces_connected() across namespaces (implementation details)
+        testutil.test_ip_connected(('192.168.1.10', ctx), ('192.168.1.1', self.ns1))
+
         ifname = str(device.name)
-        router_ll_addr = [addr for addr, _, _ in testutil.get_addrs6(self.hapd.ifname) if addr[0:2] == b'\xfe\x80'][0]
+        router_ll_addr = get_ll_addrs6(self.ns1, self.hapd.ifname)
         # Since we're in an isolated VM with freshly created interfaces we know any routes
-        # will have been created by IWD and don't have to allow for pre-existing routes
+        # will have been created by IWD and we don't have to allow for pre-existing routes
         # in the table.
         # Flags: 1=RTF_UP, 2=RTF_GATEWAY
         expected_routes4 = {
@@ -94,6 +106,67 @@ class Test(unittest.TestCase):
         # of our log since we care about the end result here.
         self.assertEqual(expected_rclog, entries[-3:])
 
+        leases_file = self.parse_lease_file('/tmp/dhcpd.leases', socket.AF_INET)
+        lease = leases_file['leases'][socket.inet_pton(socket.AF_INET, '192.168.1.10')]
+        self.assertEqual(lease['state'], 'active')
+        self.assertTrue(lease['starts'] < connect_time)
+        self.assertTrue(lease['ends'] > connect_time)
+        # The T1 is 15 seconds per dhcpd.conf.  This is the approximate interval between lease
+        # renewals we should see from the client (+/- 1 second + some jitter).  Wait a little
+        # less than twice that time (25s) so that we can expect the lease was renewed strictly
+        # once (not 0 or 2 times) by that time, check that the lease timestamps have changed by
+        # at least 10s so as to leave a lot of margin.
+        renew_time = lease['starts'] + 15
+        now = time.time()
+        ctx.non_block_wait(lambda: False, renew_time + 10 - now, exception=False)
+
+        leases_file = self.parse_lease_file('/tmp/dhcpd.leases', socket.AF_INET)
+        new_lease = leases_file['leases'][socket.inet_pton(socket.AF_INET, '192.168.1.10')]
+        self.assertEqual(new_lease['state'], 'active')
+        self.assertTrue(new_lease['starts'] > lease['starts'] + 10)
+        self.assertTrue(new_lease['starts'] < lease['starts'] + 25)
+        self.assertTrue(new_lease['ends'] > lease['ends'] + 10)
+        self.assertTrue(new_lease['ends'] < lease['ends'] + 25)
+
+        # Now wait another T1 seconds but don't let our DHCP client get its REQUEST out this
+        # time so as to test renew timeouts and resends.  The retry interval is 60 seconds
+        # since (T2 - T1) / 2 is shorter than 60s.  It is now about 10s since the last
+        # renewal or 5s before the next DHCPREQUEST frame that is going to be lost.  We'll
+        # wait T1 seconds, so until about 10s after the failed attempt, we'll check that
+        # there was no renewal by that time, just in case, and we'll reenable frame delivery.
+        # We'll then wait another 60s and we should see the lease has been successfully
+        # renewed some 10 seconds earlier on the 1st DHCPREQUEST retransmission.
+        #
+        # We can't use hswim to block the frames from reaching the AP because we'd lose
+        # beacons and get disconnected.  We also can't drop our subnet route or IP address
+        # because IWD's sendto() call would synchronously error out and the DHCP client
+        # would just give up.  Add a false route to break routing to 192.168.1.1 and delete
+        # it afterwards.
+        os.system('ip route add 192.168.1.1/32 dev ' + ifname  + ' via 192.168.1.100 preference 0')
+
+        lease = new_lease
+        renew_time = lease['starts'] + 15
+        now = time.time()
+        ctx.non_block_wait(lambda: False, renew_time + 10 - now, exception=False)
+
+        leases_file = self.parse_lease_file('/tmp/dhcpd.leases', socket.AF_INET)
+        new_lease = leases_file['leases'][socket.inet_pton(socket.AF_INET, '192.168.1.10')]
+        self.assertEqual(new_lease['starts'], lease['starts'])
+
+        os.system('ip route del 192.168.1.1/32 dev ' + ifname  + ' via 192.168.1.100 preference 0')
+
+        retry_time = lease['starts'] + 75
+        now = time.time()
+        ctx.non_block_wait(lambda: False, retry_time + 10 - now, exception=False)
+
+        leases_file = self.parse_lease_file('/tmp/dhcpd.leases', socket.AF_INET)
+        new_lease = leases_file['leases'][socket.inet_pton(socket.AF_INET, '192.168.1.10')]
+        self.assertEqual(new_lease['state'], 'active')
+        self.assertTrue(new_lease['starts'] > lease['starts'] + 70)
+        self.assertTrue(new_lease['starts'] < lease['starts'] + 85)
+        self.assertTrue(new_lease['ends'] > lease['ends'] + 70)
+        self.assertTrue(new_lease['ends'] < lease['ends'] + 85)
+
         device.disconnect()
 
         condition = 'not obj.connected'
@@ -116,25 +189,27 @@ class Test(unittest.TestCase):
             except:
                 pass
 
-        hapd = HostapdCLI()
-        cls.hapd = hapd
+        cls.ns1 = ctx.get_namespace('ns1')
+        cls.hapd = HostapdCLI('ap-ns1.conf')
         # TODO: This could be moved into test-runner itself if other tests ever
         #       require this functionality (p2p, FILS, etc.). Since its simple
         #       enough it can stay here for now.
-        ctx.start_process(['ip', 'addr','add', '192.168.1.1/255.255.128.0',
-                            'dev', hapd.ifname,]).wait()
-        ctx.start_process(['touch', '/tmp/dhcpd.leases']).wait()
-        cls.dhcpd_pid = ctx.start_process(['dhcpd', '-f', '-cf', '/tmp/dhcpd.conf',
-                                            '-lf', '/tmp/dhcpd.leases',
-                                            hapd.ifname], cleanup=remove_lease4)
-
-        ctx.start_process(['ip', 'addr', 'add', '3ffe:501:ffff:100::1/72',
-                            'dev', hapd.ifname]).wait()
-        ctx.start_process(['touch', '/tmp/dhcpd6.leases']).wait()
-        cls.dhcpd6_pid = ctx.start_process(['dhcpd', '-6', '-f', '-cf', '/tmp/dhcpd-v6.conf',
-                                            '-lf', '/tmp/dhcpd6.leases',
-                                            hapd.ifname], cleanup=remove_lease6)
-        ctx.start_process(['sysctl', 'net.ipv6.conf.' + hapd.ifname + '.forwarding=1']).wait()
+        cls.ns1.start_process(['ip', 'addr','add', '192.168.1.1/17',
+                            'dev', cls.hapd.ifname]).wait()
+        cls.ns1.start_process(['touch', '/tmp/dhcpd.leases']).wait()
+        cls.dhcpd_pid = cls.ns1.start_process(['dhcpd', '-f', '-d', '-cf', '/tmp/dhcpd.conf',
+                                                '-lf', '/tmp/dhcpd.leases',
+                                                cls.hapd.ifname], cleanup=remove_lease4)
+
+        cls.ns1.start_process(['ip', 'addr', 'add', '3ffe:501:ffff:100::1/72',
+                            'dev', cls.hapd.ifname]).wait()
+        cls.ns1.start_process(['touch', '/tmp/dhcpd6.leases']).wait()
+        cls.dhcpd6_pid = cls.ns1.start_process(['dhcpd', '-6', '-f', '-d',
+                                                '-cf', '/tmp/dhcpd-v6.conf',
+                                                '-lf', '/tmp/dhcpd6.leases',
+                                                cls.hapd.ifname], cleanup=remove_lease6)
+        cls.ns1.start_process(['sysctl',
+                                'net.ipv6.conf.' + cls.hapd.ifname + '.forwarding=1']).wait()
         # Send out Router Advertisements telling clients to use DHCPv6.
         # Note trying to send the RAs from the router's global IPv6 address by adding a
         # "AdvRASrcAddress { 3ffe:501:ffff:100::1; };" line will fail because the client
@@ -142,7 +217,7 @@ class Test(unittest.TestCase):
         # with a non-link-local gateway address that is present on another interface in the
         # same namespace.
         config = open('/tmp/radvd.conf', 'w')
-        config.write('interface ' + hapd.ifname + ''' {
+        config.write('interface ' + cls.hapd.ifname + ''' {
             AdvSendAdvert on;
             AdvManagedFlag on;
             prefix 3ffe:501:ffff:100::/72 { AdvAutonomous off; };
@@ -151,7 +226,8 @@ class Test(unittest.TestCase):
             route 3ffe:501:ffff:500::/66 { AdvRoutePreference high; };
             };''')
         config.close()
-        cls.radvd_pid = ctx.start_process(['radvd', '-n', '-d5', '-p', '/tmp/radvd.pid', '-C', '/tmp/radvd.conf'])
+        cls.radvd_pid = cls.ns1.start_process(['radvd', '-n', '-d5',
+                                                '-p', '/tmp/radvd.pid', '-C', '/tmp/radvd.conf'])
 
         cls.orig_path = os.environ['PATH']
         os.environ['PATH'] = '/tmp/test-bin:' + os.environ['PATH']
@@ -160,14 +236,83 @@ class Test(unittest.TestCase):
     @classmethod
     def tearDownClass(cls):
         IWD.clear_storage()
-        ctx.stop_process(cls.dhcpd_pid)
+        cls.ns1.stop_process(cls.dhcpd_pid)
         cls.dhcpd_pid = None
-        ctx.stop_process(cls.dhcpd6_pid)
+        cls.ns1.stop_process(cls.dhcpd6_pid)
         cls.dhcpd6_pid = None
-        ctx.stop_process(cls.radvd_pid)
+        cls.ns1.stop_process(cls.radvd_pid)
         cls.radvd_pid = None
         os.system('rm -rf /tmp/radvd.conf /tmp/resolvconf.log /tmp/test-bin')
         os.environ['PATH'] = cls.orig_path
 
+    @staticmethod
+    def parse_lease_file(path, family):
+        file = open(path, 'r')
+        lines = file.readlines()
+        file.close()
+
+        stack = [[]]
+        statement = []
+        token = ''
+        for line in lines:
+            whitespace = False
+            quote = False
+            for ch in line:
+                if not quote and ch in ' \t\r\n;{}=#':
+                    if len(token):
+                        statement.append(token)
+                        token = ''
+                if not quote and ch in ';{}':
+                    if len(statement):
+                        stack[-1].append(statement)
+                        statement = []
+                if ch == '"':
+                    quote = not quote
+                elif quote or ch not in ' \t\r\n;{}#':
+                    token += ch
+                if ch == '#':
+                    break
+                elif ch == '{':
+                    stack.append([])
+                elif ch == '}':
+                    statements = stack.pop()
+                    stack[-1][-1].append(statements)
+            if len(token):
+                statement.append(token)
+                token = ''
+        if len(statement):
+            stack[-1].append(statement)
+        statements = stack.pop(0)
+        if len(stack):
+            raise Exception('Unclosed block(s)')
+
+        contents = {'leases':{}}
+        for s in statements:
+            if s[0] == 'lease':
+                ip = socket.inet_pton(family, s[1])
+                lease = {}
+                for param in s[2]:
+                    if param[0] in ('starts', 'ends', 'tstp', 'tsfp', 'atsfp', 'cltt'):
+                        weekday = param[1]
+                        year, month, day = param[2].split('/')
+                        hour, minute, second = param[3].split(':')
+                        dt = datetime.datetime(
+                                int(year), int(month), int(day),
+                                int(hour), int(minute), int(second),
+                                tzinfo=datetime.timezone.utc)
+                        lease[param[0]] = dt.timestamp()
+                    elif param[0:2] == ['binding', 'state']:
+                        lease['state'] = param[2]
+                    elif param[0:2] == ['hardware', 'ethernet']:
+                        lease['hwaddr'] = bytes([int(v, 16) for v in param[2].split(':')])
+                    elif param[0] in ('preferred-life', 'max-life'):
+                        lease[param[0]] = int(param[1])
+                    elif param[0] in ('client-hostname'):
+                        lease[param[0]] = param[1]
+                contents['leases'][ip] = lease # New entries overwrite older ones
+            elif s[0] == 'server-duid':
+                contents[s[0]] = s[1]
+        return contents
+
 if __name__ == '__main__':
     unittest.main(exit=True)
diff --git a/autotests/testNetconfig/dhcpd.conf b/autotests/testNetconfig/dhcpd.conf
index d8a9d24c..0a154637 100644
--- a/autotests/testNetconfig/dhcpd.conf
+++ b/autotests/testNetconfig/dhcpd.conf
@@ -1,5 +1,15 @@
-default-lease-time 600;         # 10 minutes
-max-lease-time 7200;            # 2  hours
+default-lease-time 120;         # 2 minutes
+min-lease-time 120;             # 2 minutes
+max-lease-time 120;             # 2 minutes
+option dhcp-renewal-time 15;    # 15 secs for T1
+# We set a relatively low lease lifetime of 2 minutes but our renewal interval
+# (T1) is still unproportionally low to speed the test up -- 12% instead of the
+# default 50% lifetime value.  We need a lifetime in the order of minutes
+# because minimum lease renewal retry interval is 60s per spec.  However by
+# default dhcpd will not renew leases that are newer than 25% their lifetime.
+# Set that threshold to 1% so that we can verify that the lease is renewed
+# without waiting too long.
+dhcp-cache-threshold 1;
 
 option broadcast-address 192.168.127.255;
 option routers 192.168.1.254;
diff --git a/autotests/testNetconfig/hw.conf b/autotests/testNetconfig/hw.conf
index d5adc9ad..edf656d6 100644
--- a/autotests/testNetconfig/hw.conf
+++ b/autotests/testNetconfig/hw.conf
@@ -1,9 +1,11 @@
 [SETUP]
-num_radios=3
+num_radios=4
 start_iwd=0
 
 [HOSTAPD]
-rad0=ssidTKIP.conf
+rad2=ap-main.conf
+rad3=ap-ns1.conf
 
 [NameSpaces]
-ns0=rad2
+ns0=rad0
+ns1=rad3
diff --git a/autotests/testNetconfig/ssidTKIP.psk b/autotests/testNetconfig/static.psk
similarity index 100%
rename from autotests/testNetconfig/ssidTKIP.psk
rename to autotests/testNetconfig/static.psk
diff --git a/autotests/testNetconfig/static_test.py b/autotests/testNetconfig/static_test.py
index d9f0b9cb..01d694ca 100644
--- a/autotests/testNetconfig/static_test.py
+++ b/autotests/testNetconfig/static_test.py
@@ -32,7 +32,7 @@ class Test(unittest.TestCase):
         dev1 = wd.list_devices(1)[0]
         dev2 = wd_ns0.list_devices(1)[0]
 
-        ordered_network = dev1.get_ordered_network('ssidTKIP')
+        ordered_network = dev1.get_ordered_network('ap-main')
 
         self.assertEqual(ordered_network.type, NetworkType.psk)
 
@@ -80,7 +80,7 @@ class Test(unittest.TestCase):
         # of the log since we care about the end result here.
         self.assertEqual(expected_rclog, entries[-3:])
 
-        ordered_network = dev2.get_ordered_network('ssidTKIP')
+        ordered_network = dev2.get_ordered_network('ap-main')
 
         condition = 'not obj.connected'
         wd_ns0.wait_for_object_condition(ordered_network.network_object, condition)
@@ -117,7 +117,7 @@ class Test(unittest.TestCase):
             except:
                 pass
 
-        hapd = HostapdCLI()
+        hapd = HostapdCLI('ap-main.conf')
         # TODO: This could be moved into test-runner itself if other tests ever
         #       require this functionality (p2p, FILS, etc.). Since it's simple
         #       enough it can stay here for now.
@@ -127,7 +127,7 @@ class Test(unittest.TestCase):
         cls.dhcpd_pid = ctx.start_process(['dhcpd', '-f', '-cf', '/tmp/dhcpd.conf',
                                             '-lf', '/tmp/dhcpd.leases',
                                             hapd.ifname], cleanup=remove_lease)
-        IWD.copy_to_storage('ssidTKIP.psk', '/tmp/storage')
+        IWD.copy_to_storage('static.psk', '/tmp/storage', 'ap-main.psk')
 
         cls.orig_path = os.environ['PATH']
         os.environ['PATH'] = '/tmp/test-bin:' + os.environ['PATH']
diff --git a/autotests/util/hostapd.py b/autotests/util/hostapd.py
index 758427fe..3ae8ff89 100644
--- a/autotests/util/hostapd.py
+++ b/autotests/util/hostapd.py
@@ -33,7 +33,7 @@ class HostapdCLI(object):
     _instances = WeakValueDictionary()
 
     def __new__(cls, config=None, *args, **kwargs):
-        hapd = ctx.hostapd[config]
+        hapd = ctx.get_hapd_instance(config)
 
         if not config:
             config = hapd.config
@@ -58,10 +58,10 @@ class HostapdCLI(object):
         if not ctx.hostapd:
             raise Exception("No hostapd instances are configured")
 
-        if not config and len(ctx.hostapd.instances) > 1:
+        if not config and sum([len(hapd.instances) for hapd in ctx.hostapd]) > 1:
             raise Exception('config must be provided if more than one hostapd instance exists')
 
-        hapd = ctx.hostapd[config]
+        hapd = ctx.get_hapd_instance(config)
 
         self.interface = hapd.intf
         self.config = hapd.config
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v2 3/4] autotests: Also validate correct hostname sent over DHCPv4
  2022-07-07 20:03 [PATCH v2 1/4] test-runner: Support running hostapd in namespaces Andrew Zaborowski
  2022-07-07 20:03 ` [PATCH v2 2/4] autotests: DHCPv4 renewal/resend test in testNetconfig Andrew Zaborowski
@ 2022-07-07 20:03 ` Andrew Zaborowski
  2022-07-07 20:03 ` [PATCH v2 4/4] test-runner: Mark source directory as safe for git Andrew Zaborowski
  2022-07-07 22:37 ` [PATCH v2 1/4] test-runner: Support running hostapd in namespaces James Prestwood
  3 siblings, 0 replies; 8+ messages in thread
From: Andrew Zaborowski @ 2022-07-07 20:03 UTC (permalink / raw)
  To: iwd

---
 autotests/testNetconfig/auto.psk           | 5 +++++
 autotests/testNetconfig/connection_test.py | 5 +++++
 2 files changed, 10 insertions(+)
 create mode 100644 autotests/testNetconfig/auto.psk

diff --git a/autotests/testNetconfig/auto.psk b/autotests/testNetconfig/auto.psk
new file mode 100644
index 00000000..4077d65f
--- /dev/null
+++ b/autotests/testNetconfig/auto.psk
@@ -0,0 +1,5 @@
+[IPv4]
+SendHostname=true
+
+[Settings]
+AutoConnect=false
diff --git a/autotests/testNetconfig/connection_test.py b/autotests/testNetconfig/connection_test.py
index aebb0a2e..cf90993a 100644
--- a/autotests/testNetconfig/connection_test.py
+++ b/autotests/testNetconfig/connection_test.py
@@ -36,6 +36,8 @@ class Test(unittest.TestCase):
                     return socket.inet_pton(socket.AF_INET6, l.split(None, 1)[1].split('/', 1)[0])
             return None
 
+        os.system("hostname test-sta")
+        IWD.copy_to_storage('auto.psk', name='ap-ns1.psk')
         wd = IWD(True)
 
         psk_agent = PSKAgent("secret123")
@@ -109,6 +111,7 @@ class Test(unittest.TestCase):
         leases_file = self.parse_lease_file('/tmp/dhcpd.leases', socket.AF_INET)
         lease = leases_file['leases'][socket.inet_pton(socket.AF_INET, '192.168.1.10')]
         self.assertEqual(lease['state'], 'active')
+        self.assertEqual(lease['client-hostname'], 'test-sta')
         self.assertTrue(lease['starts'] < connect_time)
         self.assertTrue(lease['ends'] > connect_time)
         # The T1 is 15 seconds per dhcpd.conf.  This is the approximate interval between lease
@@ -123,6 +126,7 @@ class Test(unittest.TestCase):
         leases_file = self.parse_lease_file('/tmp/dhcpd.leases', socket.AF_INET)
         new_lease = leases_file['leases'][socket.inet_pton(socket.AF_INET, '192.168.1.10')]
         self.assertEqual(new_lease['state'], 'active')
+        self.assertEqual(new_lease['client-hostname'], 'test-sta')
         self.assertTrue(new_lease['starts'] > lease['starts'] + 10)
         self.assertTrue(new_lease['starts'] < lease['starts'] + 25)
         self.assertTrue(new_lease['ends'] > lease['ends'] + 10)
@@ -162,6 +166,7 @@ class Test(unittest.TestCase):
         leases_file = self.parse_lease_file('/tmp/dhcpd.leases', socket.AF_INET)
         new_lease = leases_file['leases'][socket.inet_pton(socket.AF_INET, '192.168.1.10')]
         self.assertEqual(new_lease['state'], 'active')
+        self.assertEqual(lease['client-hostname'], 'test-sta')
         self.assertTrue(new_lease['starts'] > lease['starts'] + 70)
         self.assertTrue(new_lease['starts'] < lease['starts'] + 85)
         self.assertTrue(new_lease['ends'] > lease['ends'] + 70)
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v2 4/4] test-runner: Mark source directory as safe for git
  2022-07-07 20:03 [PATCH v2 1/4] test-runner: Support running hostapd in namespaces Andrew Zaborowski
  2022-07-07 20:03 ` [PATCH v2 2/4] autotests: DHCPv4 renewal/resend test in testNetconfig Andrew Zaborowski
  2022-07-07 20:03 ` [PATCH v2 3/4] autotests: Also validate correct hostname sent over DHCPv4 Andrew Zaborowski
@ 2022-07-07 20:03 ` Andrew Zaborowski
  2022-07-07 22:37 ` [PATCH v2 1/4] test-runner: Support running hostapd in namespaces James Prestwood
  3 siblings, 0 replies; 8+ messages in thread
From: Andrew Zaborowski @ 2022-07-07 20:03 UTC (permalink / raw)
  To: iwd

Since we use git ls-files to produce the list of all tests for -A, if
the source directory is owned by somebody other than root one might get:

fatal: unsafe repository ('/home/balrog/repos/iwd' is owned by someone else)
To add an exception for this directory, call:

	git config --global --add safe.directory /home/balrog/repos/iwd

Starting
/home/balrog/repos/iwd/tools/..//autotests/ threw an uncaught exception
Traceback (most recent call last):
  File "/home/balrog/repos/iwd/tools/run-tests", line 966, in run_auto_tests
    subtests = pre_test(ctx, test, copied)
  File "/home/balrog/repos/iwd/tools/run-tests", line 814, in pre_test
    raise Exception("No hw.conf found for %s" % test)
Exception: No hw.conf found for /home/balrog/repos/iwd/tools/..//autotests/

Mark args.testhome as a safe directory on every run.
---
 tools/run-tests | 4 +++-
 tools/runner.py | 1 +
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/tools/run-tests b/tools/run-tests
index c0f230cb..f409e134 100755
--- a/tools/run-tests
+++ b/tools/run-tests
@@ -630,8 +630,10 @@ def build_test_list(args):
 	# Run all tests
 	if not args.autotests:
 		# Get list of all autotests (committed in git)
+		Process(['git', 'config', '--system', '--add', 'safe.directory',
+					os.path.normpath(args.testhome)]).wait()
 		tests = os.popen('git -C %s ls-files autotests/ | cut -f2 -d"/" \
-					| grep "test*" | uniq' % args.testhome).read() \
+					| grep "^test" | uniq' % args.testhome).read() \
 					.strip().split('\n')
 		tests = [test_root + '/' + t for t in tests]
 	else:
diff --git a/tools/runner.py b/tools/runner.py
index d39b560f..164bc881 100644
--- a/tools/runner.py
+++ b/tools/runner.py
@@ -36,6 +36,7 @@ mounts_common = [
 	MountInfo('tmpfs', 'tmpfs', '/run', 'mode=0755',
 					MS_NOSUID|MS_NODEV|MS_STRICTATIME),
 	MountInfo('tmpfs', 'tmpfs', '/tmp', '', 0),
+	MountInfo('tmpfs', 'tmpfs', '/etc', '', 0),
 	MountInfo('tmpfs', 'tmpfs', '/usr/share/dbus-1', 'mode=0755',
 					MS_NOSUID|MS_NOEXEC|MS_NODEV|MS_STRICTATIME),
 ]
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 1/4] test-runner: Support running hostapd in namespaces
  2022-07-07 20:03 [PATCH v2 1/4] test-runner: Support running hostapd in namespaces Andrew Zaborowski
                   ` (2 preceding siblings ...)
  2022-07-07 20:03 ` [PATCH v2 4/4] test-runner: Mark source directory as safe for git Andrew Zaborowski
@ 2022-07-07 22:37 ` James Prestwood
  2022-07-07 23:11   ` Andrew Zaborowski
  3 siblings, 1 reply; 8+ messages in thread
From: James Prestwood @ 2022-07-07 22:37 UTC (permalink / raw)
  To: Andrew Zaborowski, iwd

Hi Andrew,

On Thu, 2022-07-07 at 22:03 +0200, Andrew Zaborowski wrote:
> The kernel will not let us test some scenarios of communication
> between
> two hwsim radios (e.g. STA and AP) if they're in the same net
> namespace.
> For example, when connected, you can't add normal IPv4 subnet routes
> for
> the same subnet on two different interfaces in one namespace (you'd
> either get an EEXIST or you'd replace the other route), you can set
> different metrics on the routes but that won't fix IP routing.  For
> testNetconfig the result is that communication works for DHCP before
> we
> get the inital lease but renewals won't work because they're unicast.
> Allow hostapd to run on a radio that has been moved to a different
> namespace in hw.conf so we don't have to work around these issues.
> ---
> changes in v2:
>  - ensure at least one hostapd session starts if the [HOSTAPD]
> section
>    is present to mimic original behaviour and fix testEAD
> 
>  tools/run-tests | 82 ++++++++++++++++++++++++++++++++---------------
> --
>  tools/utils.py  |  2 +-
>  2 files changed, 54 insertions(+), 30 deletions(-)
> 
> diff --git a/tools/run-tests b/tools/run-tests
> index 565847df..c0f230cb 100755
> --- a/tools/run-tests
> +++ b/tools/run-tests
> @@ -58,16 +58,19 @@ def exit_vm():
>         runner.stop()
>  
>  class Interface:
> -       def __init__(self, name, config):
> +       def __init__(self, name, config, ns=None):
>                 self.name = name
>                 self.ctrl_interface = '/var/run/hostapd/' + name
>                 self.config = config
> +               self.ns = ns
>  
>         def __del__(self):
> -               Process(['iw', 'dev', self.name, 'del']).wait()
> +               Process(['iw', 'dev', self.name, 'del'],
> +                               namespace=self.ns.name if self.ns
> else None).wait()
>  
>         def set_interface_state(self, state):
> -               Process(['ip', 'link', 'set', self.name,
> state]).wait()
> +               Process(['ip', 'link', 'set', self.name, state],
> +                               namespace=self.ns.name if self.ns
> else None).wait()
>  

So rather than this "self.ns.name if self.ns else None" thing, you
could either store the namespace name directly (since self.ns is only
used for the name), or store the namespace object like you do and
replace Process(...) with self.ns.start_process(...).

>  class Radio:
>         def __init__(self, name):
> @@ -75,11 +78,16 @@ class Radio:
>                 # hostapd will reset this if this radio is used by it
>                 self.use = 'iwd'
>                 self.interface = None
> +               self.ns = None
>  
>         def __del__(self):
>                 print("Removing radio %s" % self.name)
>                 self.interface = None
>  
> +       def set_namespace(self, ns):
> +               self.ns = ns
> +               Process(['iw', 'phy', self.name, 'set', 'netns',
> 'name', ns.name]).wait()
> +
>         def create_interface(self, config, use):
>                 global intf_id
>  
> @@ -87,19 +95,21 @@ class Radio:
>  
>                 intf_id += 1
>  
> -               self.interface = Interface(ifname, config)
> +               self.interface = Interface(ifname, config,
> ns=self.ns)
>                 self.use = use
>  
>                 Process(['iw', 'phy', self.name, 'interface', 'add',
> ifname,
> -                               'type', 'managed']).wait()
> +                               'type', 'managed'],
> namespace=self.ns.name if self.ns else None).wait()
>  
>                 return self.interface

Since create_interface() is the only function that needs the namespace
couldn't we remove set_namespace entirely and just pass the namespace
name into create_interface?

>  
>         def __str__(self):
>                 ret = self.name + ':\n'
> -               ret += '\tUsed By: %s ' % self.use
> +               ret += '\tUsed By: %s' % self.use
>                 if self.interface:
> -                       ret += '(%s)' % self.interface.name
> +                       ret += ' (%s)' % self.interface.name
> +               if self.ns is not None:
> +                       ret += ' (ns=%s)' % self.ns.name
>  
>                 ret += '\n'
>  
> @@ -188,7 +198,7 @@ class Hostapd:
>                 A set of running hostapd instances. This is really
> just a single
>                 process since hostapd can be started with multiple
> config files.
>         '''
> -       def __init__(self, radios, configs, radius):
> +       def __init__(self, ns, radios, configs, radius):
>                 if len(configs) != len(radios):
>                         raise Exception("Config (%d) and radio (%d)
> list length not equal" % \
>                                                 (len(configs),
> len(radios)))
> @@ -198,8 +208,8 @@ class Hostapd:
>                 Process(['ip', 'link', 'set', 'eth0', 'up']).wait()
>                 Process(['ip', 'link', 'set', 'eth1', 'up']).wait()
>  
> -               self.global_ctrl_iface = '/var/run/hostapd/ctrl'
> -
> +               self.ns = ns
> +               self.global_ctrl_iface = '/var/run/hostapd/ctrl' +
> (str(ns.name) if ns.name else 'main')
>                 self.instances = [HostapdInstance(c, r) for c, r in
> zip(configs, radios)]
>  
>                 ifaces = [rad.interface.name for rad in radios]
> @@ -227,7 +237,7 @@ class Hostapd:
>                 if Process.is_verbose('hostapd'):
>                         args.append('-d')
>  
> -               self.process = Process(args)
> +               self.process = Process(args, namespace=ns.name)
>  
>                 self.process.wait_for_socket(self.global_ctrl_iface,
> 30)
>  
> @@ -397,32 +407,44 @@ class TestContext(Namespace):
>                         #          tests. In this case you would not
> care what
>                         #          was using each radio, just that
> there was
>                         #          enough to run all tests.
> -                       nradios = 0
> -                       for k, _ in settings.items():
> -                               if k == 'radius_server':
> -                                       continue
> -                               nradios += 1
> -
> -                       hapd_radios = self.radios[:nradios]
> -
> +                       hapd_configs = [conf for rad, conf in
> settings.items() if rad != 'radius_server']
> +                       hapd_processes = [(self,
> self.radios[:len(hapd_configs)], hapd_configs)]
>                 else:
> -                       hapd_radios = [rad for rad in self.radios if
> rad.name in settings]
> -
> -               hapd_configs = [conf for rad, conf in
> settings.items() if rad != 'radius_server']
> +                       hapd_processes = []
> +                       for ns in [self] + self.namespaces:
> +                               ns_radios = [rad for rad in ns.radios
> if rad.name in settings]
> +                               if len(ns_radios):
> +                                       ns_configs =
> [settings[rad.name] for rad in ns_radios]
> +                                       hapd_processes.append((ns,
> ns_radios, ns_configs))
> +                       if not hapd_processes:
> +                               hapd_processes.append((self, [], []))
>  
>                 radius_config = settings.get('radius_server', None)
>  
> -               self.hostapd = Hostapd(hapd_radios, hapd_configs,
> radius_config)
> -               self.hostapd.attach_cli()
> +               self.hostapd = [Hostapd(ns, radios, configs,
> radius_config)
> +                               for ns, radios, configs in
> hapd_processes]
> +
> +               for hapd in self.hostapd:
> +                       hapd.attach_cli()
>  
>         def get_frequencies(self):
>                 frequencies = []
>  
> -               for hapd in self.hostapd.instances:
> -                       frequencies.append(hapd.cli.frequency)
> +               for hapd in self.hostapd:
> +                       frequencies += [instance.cli.frequency for
> instance in hapd.instances]
>  
>                 return frequencies
>  
> +       def get_hapd_instance(self, config=None):
> +               instances = [i for hapd in self.hostapd for i in
> hapd.instances]
> +
> +               if config is None:
> +                       return instances[0]
> +
> +               for hapd in instances:
> +                       if hapd.config == config:
> +                               return hapd
> +

Looks like this got carried over from that dropped patch?

>         def start_wpas_interfaces(self):
>                 if 'WPA_SUPPLICANT' not in self.hw_config:
>                         return
> @@ -543,11 +565,13 @@ class TestContext(Namespace):
>                 for arg in vars(self.args):
>                         ret += '\t --%s %s\n' % (arg,
> str(getattr(self.args, arg)))
>  
> -               ret += 'Hostapd:\n'
>                 if self.hostapd:
> -                       for h in self.hostapd.instances:
> -                               ret += '\t%s\n' % str(h)
> +                       for hapd in self.hostapd:
> +                               ret += 'Hostapd (ns=%s):\n' %
> (hapd.ns.name,)
> +                               for h in hapd.instances:
> +                                       ret += '\t%s\n' % (str(h),)
>                 else:
> +                       ret += 'Hostapd:\n'
>                         ret += '\tNo Hostapd instances\n'
>  
>                 info = self.meminfo_to_dict()
> diff --git a/tools/utils.py b/tools/utils.py
> index f3e12a85..bc030230 100644
> --- a/tools/utils.py
> +++ b/tools/utils.py
> @@ -324,7 +324,7 @@ class Namespace:
>  
>                 Process(['ip', 'netns', 'add', name]).wait()
>                 for r in radios:
> -                       Process(['iw', 'phy', r.name, 'set', 'netns',
> 'name', name]).wait()
> +                       r.set_namespace(self)
>  
>                 self.start_dbus()
>  



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 1/4] test-runner: Support running hostapd in namespaces
  2022-07-07 22:37 ` [PATCH v2 1/4] test-runner: Support running hostapd in namespaces James Prestwood
@ 2022-07-07 23:11   ` Andrew Zaborowski
  2022-07-07 23:19     ` James Prestwood
  0 siblings, 1 reply; 8+ messages in thread
From: Andrew Zaborowski @ 2022-07-07 23:11 UTC (permalink / raw)
  To: James Prestwood; +Cc: iwd

Hi James,

On Fri, 8 Jul 2022 at 00:37, James Prestwood <prestwoj@gmail.com> wrote:
> On Thu, 2022-07-07 at 22:03 +0200, Andrew Zaborowski wrote:
> >  class Interface:
> > -       def __init__(self, name, config):
> > +       def __init__(self, name, config, ns=None):
> >                 self.name = name
> >                 self.ctrl_interface = '/var/run/hostapd/' + name
> >                 self.config = config
> > +               self.ns = ns
> >
> >         def __del__(self):
> > -               Process(['iw', 'dev', self.name, 'del']).wait()
> > +               Process(['iw', 'dev', self.name, 'del'],
> > +                               namespace=self.ns.name if self.ns
> > else None).wait()
> >
> >         def set_interface_state(self, state):
> > -               Process(['ip', 'link', 'set', self.name,
> > state]).wait()
> > +               Process(['ip', 'link', 'set', self.name, state],
> > +                               namespace=self.ns.name if self.ns
> > else None).wait()
> >
>
> So rather than this "self.ns.name if self.ns else None" thing, you
> could either store the namespace name directly (since self.ns is only
> used for the name), or store the namespace object like you do and
> replace Process(...) with self.ns.start_process(...).

Ok, either of those should work, or maybe make sure Radios and
Interfaces are initalized with "ctx" as the namespace so self.ns is
never None.

>
> >  class Radio:
> >         def __init__(self, name):
> > @@ -75,11 +78,16 @@ class Radio:
> >                 # hostapd will reset this if this radio is used by it
> >                 self.use = 'iwd'
> >                 self.interface = None
> > +               self.ns = None
> >
> >         def __del__(self):
> >                 print("Removing radio %s" % self.name)
> >                 self.interface = None
> >
> > +       def set_namespace(self, ns):
> > +               self.ns = ns
> > +               Process(['iw', 'phy', self.name, 'set', 'netns',
> > 'name', ns.name]).wait()
> > +
> >         def create_interface(self, config, use):
> >                 global intf_id
> >
> > @@ -87,19 +95,21 @@ class Radio:
> >
> >                 intf_id += 1
> >
> > -               self.interface = Interface(ifname, config)
> > +               self.interface = Interface(ifname, config,
> > ns=self.ns)
> >                 self.use = use
> >
> >                 Process(['iw', 'phy', self.name, 'interface', 'add',
> > ifname,
> > -                               'type', 'managed']).wait()
> > +                               'type', 'managed'],
> > namespace=self.ns.name if self.ns else None).wait()
> >
> >                 return self.interface
>
> Since create_interface() is the only function that needs the namespace
> couldn't we remove set_namespace entirely and just pass the namespace
> name into create_interface?

So set_namespace() also moves the radio to the right namespace.  We
need this for radios used by both hostapd and IWD, where we don't use
create_interface().

I also think it's nice if you can obtain the namespace reference from
`radio.ns` or `interface.ns`, say, in autotests/util/*.py code or
elsewhere.

>
> > +       def get_hapd_instance(self, config=None):
> > +               instances = [i for hapd in self.hostapd for i in
> > hapd.instances]
> > +
> > +               if config is None:
> > +                       return instances[0]
> > +
> > +               for hapd in instances:
> > +                       if hapd.config == config:
> > +                               return hapd
> > +
>
> Looks like this got carried over from that dropped patch?

I use this in autotests/util/hostapd.py (patch 2/4) since ctx.hostapd
is now a list and to override the [] operator you'd need to subclass
the list and overall it's simpler this way.

Thanks for the review,
Best regards

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 1/4] test-runner: Support running hostapd in namespaces
  2022-07-07 23:11   ` Andrew Zaborowski
@ 2022-07-07 23:19     ` James Prestwood
  2022-07-07 23:42       ` Andrew Zaborowski
  0 siblings, 1 reply; 8+ messages in thread
From: James Prestwood @ 2022-07-07 23:19 UTC (permalink / raw)
  To: Andrew Zaborowski; +Cc: iwd

Hi Andrew,
> > >  class Radio:
> > >         def __init__(self, name):
> > > @@ -75,11 +78,16 @@ class Radio:
> > >                 # hostapd will reset this if this radio is used
> > > by it
> > >                 self.use = 'iwd'
> > >                 self.interface = None
> > > +               self.ns = None
> > > 
> > >         def __del__(self):
> > >                 print("Removing radio %s" % self.name)
> > >                 self.interface = None
> > > 
> > > +       def set_namespace(self, ns):
> > > +               self.ns = ns
> > > +               Process(['iw', 'phy', self.name, 'set', 'netns',
> > > 'name', ns.name]).wait()
> > > +
> > >         def create_interface(self, config, use):
> > >                 global intf_id
> > > 
> > > @@ -87,19 +95,21 @@ class Radio:
> > > 
> > >                 intf_id += 1
> > > 
> > > -               self.interface = Interface(ifname, config)
> > > +               self.interface = Interface(ifname, config,
> > > ns=self.ns)
> > >                 self.use = use
> > > 
> > >                 Process(['iw', 'phy', self.name, 'interface',
> > > 'add',
> > > ifname,
> > > -                               'type', 'managed']).wait()
> > > +                               'type', 'managed'],
> > > namespace=self.ns.name if self.ns else None).wait()
> > > 
> > >                 return self.interface
> > 
> > Since create_interface() is the only function that needs the
> > namespace
> > couldn't we remove set_namespace entirely and just pass the
> > namespace
> > name into create_interface?
> 
> So set_namespace() also moves the radio to the right namespace.  We
> need this for radios used by both hostapd and IWD, where we don't use
> create_interface().
> 
> I also think it's nice if you can obtain the namespace reference from
> `radio.ns` or `interface.ns`, say, in autotests/util/*.py code or
> elsewhere.


Are there situations where hostapd and IWD share a radio?

But I agree, its nice knowing what namespace a radio/interface belongs
to.

> 
> > 
> > > +       def get_hapd_instance(self, config=None):
> > > +               instances = [i for hapd in self.hostapd for i in
> > > hapd.instances]
> > > +
> > > +               if config is None:
> > > +                       return instances[0]
> > > +
> > > +               for hapd in instances:
> > > +                       if hapd.config == config:
> > > +                               return hapd
> > > +
> > 
> > Looks like this got carried over from that dropped patch?
> 
> I use this in autotests/util/hostapd.py (patch 2/4) since ctx.hostapd
> is now a list and to override the [] operator you'd need to subclass
> the list and overall it's simpler this way.


Sorry, I was confusing this with the HostapdCLI stuff. Nevermind.

> 
> Thanks for the review,
> Best regards




^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 1/4] test-runner: Support running hostapd in namespaces
  2022-07-07 23:19     ` James Prestwood
@ 2022-07-07 23:42       ` Andrew Zaborowski
  0 siblings, 0 replies; 8+ messages in thread
From: Andrew Zaborowski @ 2022-07-07 23:42 UTC (permalink / raw)
  To: James Prestwood; +Cc: iwd

On Fri, 8 Jul 2022 at 01:19, James Prestwood <prestwoj@gmail.com> wrote:
> > > Since create_interface() is the only function that needs the
> > > namespace
> > > couldn't we remove set_namespace entirely and just pass the
> > > namespace
> > > name into create_interface?
> >
> > So set_namespace() also moves the radio to the right namespace.  We
> > need this for radios used by both hostapd and IWD, where we don't use
> > create_interface().
> >
> > I also think it's nice if you can obtain the namespace reference from
> > `radio.ns` or `interface.ns`, say, in autotests/util/*.py code or
> > elsewhere.
>
>
> Are there situations where hostapd and IWD share a radio?

Sorry, I meant set_namespace() may be used by tools/utils.py whether
this is an IWD radio or a hostapd radio.  create_interface() on the
other hand is only needed for hostapd so we can't replace one with the
other.

>
> But I agree, its nice knowing what namespace a radio/interface belongs
> to.
>
> >
> > >
> > > > +       def get_hapd_instance(self, config=None):
> > > > +               instances = [i for hapd in self.hostapd for i in
> > > > hapd.instances]
> > > > +
> > > > +               if config is None:
> > > > +                       return instances[0]
> > > > +
> > > > +               for hapd in instances:
> > > > +                       if hapd.config == config:
> > > > +                               return hapd
> > > > +
> > >
> > > Looks like this got carried over from that dropped patch?
> >
> > I use this in autotests/util/hostapd.py (patch 2/4) since ctx.hostapd
> > is now a list and to override the [] operator you'd need to subclass
> > the list and overall it's simpler this way.
>
>
> Sorry, I was confusing this with the HostapdCLI stuff. Nevermind.

(That code also used this method, you weren't wrong)

Best regards

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2022-07-07 23:42 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-07-07 20:03 [PATCH v2 1/4] test-runner: Support running hostapd in namespaces Andrew Zaborowski
2022-07-07 20:03 ` [PATCH v2 2/4] autotests: DHCPv4 renewal/resend test in testNetconfig Andrew Zaborowski
2022-07-07 20:03 ` [PATCH v2 3/4] autotests: Also validate correct hostname sent over DHCPv4 Andrew Zaborowski
2022-07-07 20:03 ` [PATCH v2 4/4] test-runner: Mark source directory as safe for git Andrew Zaborowski
2022-07-07 22:37 ` [PATCH v2 1/4] test-runner: Support running hostapd in namespaces James Prestwood
2022-07-07 23:11   ` Andrew Zaborowski
2022-07-07 23:19     ` James Prestwood
2022-07-07 23:42       ` Andrew Zaborowski

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.