[PATCH 1/6] test-runner: remove special case for "root" namespace

[PATCH 1/6] test-runner: remove special case for "root" namespace

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 2827 bytes --]

This was a placeholder at one point but modules grew to depend on it
being a string. Fix these dependencies and set the root namespace
name to None so there is no more special case needed to handle both
a named namespace and the original 'root' namespace.
---
 autotests/util/hwsim.py |  8 ++++----
 autotests/util/iwd.py   |  2 +-
 tools/test-runner       | 10 ++--------
 3 files changed, 7 insertions(+), 13 deletions(-)

diff --git a/autotests/util/hwsim.py b/autotests/util/hwsim.py
index cc62495c..26092379 100755
--- a/autotests/util/hwsim.py
+++ b/autotests/util/hwsim.py
@@ -301,15 +301,15 @@ class Hwsim(iwd.AsyncOpAbstract):
     _instances = WeakValueDictionary()
 
     def __new__(cls, namespace=ctx):
-        name = namespace.name
+        key = id(namespace)
 
-        if name not in cls._instances.keys():
+        if key not in cls._instances.keys():
             obj = object.__new__(cls)
             obj._initialized = False
 
-            cls._instances[name] = obj
+            cls._instances[key] = obj
 
-        return cls._instances[name]
+        return cls._instances[key]
 
     def __init__(self, namespace=ctx):
         if self._initialized:
diff --git a/autotests/util/iwd.py b/autotests/util/iwd.py
index 730659ac..63b526fd 100755
--- a/autotests/util/iwd.py
+++ b/autotests/util/iwd.py
@@ -1053,7 +1053,7 @@ class IWD(AsyncOpAbstract):
         # reference so that __del__ gets called when it's released. This is only
         # done for the root namespace in order to allow testutil to function
         # correctly in non-namespace tests.
-        if self.namespace.name == "root":
+        if self.namespace.name is None:
             IWD._default_instance = weakref.ref(self)
 
     def __del__(self):
diff --git a/tools/test-runner b/tools/test-runner
index 9e4d0780..210e4f5f 100755
--- a/tools/test-runner
+++ b/tools/test-runner
@@ -599,12 +599,6 @@ class Namespace:
 		return self._bus
 
 	def start_process(self, args, env=None, **kwargs):
-		# Special case for 'root' namespace (aka TestContext)
-		if self.name == "root":
-			ns = None
-		else:
-			ns = self.name
-
 		if not env:
 			env = os.environ.copy()
 
@@ -612,7 +606,7 @@ class Namespace:
 			# In case this process needs DBus...
 			env['DBUS_SYSTEM_BUS_ADDRESS'] = self.dbus_address
 
-		p = Process(args, namespace=ns, env=env, **kwargs)
+		p = Process(args, namespace=self.name, env=env, **kwargs)
 
 		if not kwargs.get('wait', False):
 			self.processes.append(p)
@@ -791,7 +785,7 @@ class TestContext(Namespace):
 		such as processes, radios, interfaces and test results.
 	'''
 	def __init__(self, args):
-		self.name = "root"
+		self.name = None
 		self.processes = []
 		self.args = args
 		self.hw_config = None
-- 
2.31.1

[PATCH 2/6] test-runner: don't use start_process for transient processes

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 743 bytes --]

Any process which is short lived and  waited for should just use
Process directly as to not add to the process queue.
---
 tools/test-runner | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/test-runner b/tools/test-runner
index 210e4f5f..34fdb74a 100755
--- a/tools/test-runner
+++ b/tools/test-runner
@@ -464,8 +464,8 @@ class Hostapd:
 
 		print("Initializing hostapd instances")
 
-		ctx.start_process(['ip', 'link', 'set', 'eth0', 'up']).wait()
-		ctx.start_process(['ip', 'link', 'set', 'eth1', 'up']).wait()
+		Process(['ip', 'link', 'set', 'eth0', 'up']).wait()
+		Process(['ip', 'link', 'set', 'eth1', 'up']).wait()
 
 		self.global_ctrl_iface = '/var/run/hostapd/ctrl'
 
-- 
2.31.1

[PATCH 3/6] test-runner: fix process cleanup

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 1651 bytes --]

Processes which were not explicitly killed ended up staying around
forever because they internally held references to other objects
such as GLib IO watches or write FDs.

This shuffles some code so these objects get cleaned up both when
explititly killed and after being waited for.
---
 tools/test-runner | 31 +++++++++++++++++--------------
 1 file changed, 17 insertions(+), 14 deletions(-)

diff --git a/tools/test-runner b/tools/test-runner
index 34fdb74a..fb52c57c 100755
--- a/tools/test-runner
+++ b/tools/test-runner
@@ -287,7 +287,23 @@ class Process(subprocess.Popen):
 	# Override wait() so it can do so non-blocking
 	def wait(self, timeout=10):
 		Namespace.non_block_wait(self.__wait, timeout, 1)
+		self._cleanup()
 
+	def _cleanup(self):
+		if self.cleanup:
+			self.cleanup()
+
+		self.write_fds = []
+
+		if self.io_watch:
+			GLib.source_remove(self.io_watch)
+			self.io_watch = None
+
+		self.cleanup = None
+		self.killed = True
+
+		if self in self.ctx.processes:
+			self.ctx.processes.remove(self)
 	# Override kill()
 	def kill(self, force=False):
 		if self.killed:
@@ -306,20 +322,7 @@ class Process(subprocess.Popen):
 			dbg("Process %s did not complete in 15 seconds!" % self.name)
 			super().kill()
 
-		if self.cleanup:
-			self.cleanup()
-
-		self.write_fds = []
-
-		if self.io_watch:
-			GLib.source_remove(self.io_watch)
-			self.io_watch = None
-
-		self.cleanup = None
-		self.killed = True
-
-		if self in self.ctx.processes:
-			self.ctx.processes.remove(self)
+		self._cleanup()
 
 	def __str__(self):
 		return str(self.args) + '\n'
-- 
2.31.1

[PATCH 4/6] test-runner: use Process to start hostapd

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 597 bytes --]

Since the hostapd process object is tracked by the Hostapd class there
is no sense of keeping it in the process list as well.
---
 tools/test-runner | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/test-runner b/tools/test-runner
index fb52c57c..1fe5e843 100755
--- a/tools/test-runner
+++ b/tools/test-runner
@@ -499,7 +499,7 @@ class Hostapd:
 		if ctx.is_verbose('hostapd'):
 			args.append('-d')
 
-		self.process = ctx.start_process(args)
+		self.process = Process(args)
 
 		self.process.wait_for_socket(self.global_ctrl_iface, 30)
 
-- 
2.31.1

[PATCH 5/6] test-runner: write out separators in log files

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 3600 bytes --]

The test-runner logging is very basic and just dumps everything into files
per-test. This means any subtests are just appended to existing log files
which can be difficult to parse after the fact. This is especially hard
when IWD/Hostapd runs once for the entirety of the test (as opposed to
killing between tests).

This patch writes out a separator between each subtests in the form:
===== <file>:<function> =====

To do this all processes are now kept as weak references inside the
Process class itself. Process.write_separators() can be called which
will iterate through all running processes and write the provided
separator.

This also paves the way to remove the ctx.processes array which is more
trouble than its worth due to reference issues.

Note: For tests which start IWD this will have no effect as the separator
is written prior to the test running. For these tests though, it is
much easier to read the log files because you can clearly see when
IWD starts and exits.
---
 tools/test-runner | 47 ++++++++++++++++++++++++++++++++++-------------
 1 file changed, 34 insertions(+), 13 deletions(-)

diff --git a/tools/test-runner b/tools/test-runner
index 1fe5e843..bd0034c2 100755
--- a/tools/test-runner
+++ b/tools/test-runner
@@ -27,6 +27,7 @@ from collections import namedtuple
 from time import sleep
 import dbus.mainloop.glib
 from gi.repository import GLib
+from weakref import WeakValueDictionary
 
 libc = ctypes.cdll['libc.so.6']
 libc.mount.argtypes = (ctypes.c_char_p, ctypes.c_char_p, ctypes.c_char_p, \
@@ -161,8 +162,14 @@ busconfig.dtd\">
 '''
 
 class Process(subprocess.Popen):
+	processes = WeakValueDictionary()
 	ctx = None
 
+	def __new__(cls, *args, **kwargs):
+		obj = super().__new__(cls)
+		cls.processes[id(obj)] = obj
+		return obj
+
 	def __init__(self, args, namespace=None, outfile=None, env=None, check=False, cleanup=None):
 		self.write_fds = []
 		self.io_watch = None
@@ -212,6 +219,30 @@ class Process(subprocess.Popen):
 				raise subprocess.CalledProcessError(returncode=self.returncode,
 									cmd=args)
 
+	@staticmethod
+	def _write_io(instance, data, stdout=True):
+		for f in instance.write_fds:
+			f.write(data)
+
+			# Write out a separator so multiple process calls per
+			# test are easer to read.
+			if instance.hup:
+				f.write("Terminated: {}\n\n".format(instance.args))
+
+			f.flush()
+
+		if instance.verbose and stdout:
+			sys.__stdout__.write(data)
+			sys.__stdout__.flush()
+
+	@classmethod
+	def write_separators(cls, sep):
+		for proc in cls.processes.values():
+			if proc.killed:
+				continue
+
+			cls._write_io(proc, sep, stdout=False)
+
 	def process_io(self, source, condition):
 		if condition & GLib.IO_HUP:
 			self.hup = True
@@ -226,19 +257,7 @@ class Process(subprocess.Popen):
 		# Save data away in case the caller needs it (e.g. list_sta)
 		self.out += data
 
-		for f in self.write_fds:
-			f.write(data)
-
-			# Write out a separator so multiple process calls per
-			# test are easer to read.
-			if self.hup:
-				f.write("Terminated: {}\n\n".format(self.args))
-
-			f.flush()
-
-		if self.verbose:
-			sys.__stdout__.write(data)
-			sys.__stdout__.flush()
+		self._write_io(self, data)
 
 		return True
 
@@ -1179,6 +1198,8 @@ def start_test(ctx, subtests, rqueue):
 
 					sys.__stdout__.flush()
 
+					Process.write_separators("\n====== %s:%s ======\n\n" % (file, func))
+
 					if not skip:
 						# Run test (setUp/tearDown run automatically)
 						result = t()
-- 
2.31.1

[PATCH 6/6] test-runner: move process tracking out of Namespace

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 3729 bytes --]

Since Process.processes is a weak reference dictionary any process
put in this dict will disappear if all references are lost. This
is much better than keeping a list in the Namespace which will hold
the references forever until test-runner manually kills them all at
the end of the test. This does still need to be done for daemon
processes but everything else can just go away when it is no longer
needed.
---
 tools/test-runner | 40 +++++++++++++++++-----------------------
 1 file changed, 17 insertions(+), 23 deletions(-)

diff --git a/tools/test-runner b/tools/test-runner
index bd0034c2..b0d6554d 100755
--- a/tools/test-runner
+++ b/tools/test-runner
@@ -57,12 +57,9 @@ def dbg(*s, **kwargs):
 
 def exit_vm():
 	if config:
-		for p in config.ctx.processes:
+		for p in Process.get_all():
 			print("Process %s still running!" % p.args[0])
 			p.kill()
-			p = None
-
-		config.ctx.processes = []
 
 	if config.ctx and config.ctx.results:
 		print_results(config.ctx.results)
@@ -178,6 +175,7 @@ class Process(subprocess.Popen):
 		self.out = ''
 		self.hup = False
 		self.killed = False
+		self.namespace = namespace
 
 		if not self.ctx:
 			global config
@@ -219,6 +217,15 @@ class Process(subprocess.Popen):
 				raise subprocess.CalledProcessError(returncode=self.returncode,
 									cmd=args)
 
+	@classmethod
+	def get_all(cls):
+		return cls.processes.values()
+
+	@classmethod
+	def kill_all(cls):
+		for p in cls.processes.values():
+			p.kill()
+
 	@staticmethod
 	def _write_io(instance, data, stdout=True):
 		for f in instance.write_fds:
@@ -321,8 +328,6 @@ class Process(subprocess.Popen):
 		self.cleanup = None
 		self.killed = True
 
-		if self in self.ctx.processes:
-			self.ctx.processes.remove(self)
 	# Override kill()
 	def kill(self, force=False):
 		if self.killed:
@@ -588,7 +593,6 @@ dbus_count = 0
 class Namespace:
 	def __init__(self, args, name, radios):
 		self.dbus_address = None
-		self.processes = []
 		self.name = name
 		self.radios = radios
 		self.args = args
@@ -607,10 +611,7 @@ class Namespace:
 
 		self.radios = []
 
-		for p in list(self.processes):
-			p.kill()
-
-		self.processes = []
+		Process.kill_all()
 
 	def __del__(self):
 		print("Removing namespace %s" % self.name)
@@ -628,19 +629,14 @@ class Namespace:
 			# In case this process needs DBus...
 			env['DBUS_SYSTEM_BUS_ADDRESS'] = self.dbus_address
 
-		p = Process(args, namespace=self.name, env=env, **kwargs)
-
-		if not kwargs.get('wait', False):
-			self.processes.append(p)
-
-		return p
+		return Process(args, namespace=self.name, env=env, **kwargs)
 
 	def stop_process(self, p, force=False):
 		p.kill(force)
 
 	def is_process_running(self, process):
-		for p in self.processes:
-			if p.args[0] == process:
+		for p in Process.get_all():
+			if p.namespace == self.name and p.args[0] == process:
 				return True
 		return False
 
@@ -698,8 +694,7 @@ class Namespace:
 		if self.is_verbose('iwd-acd'):
 			env['IWD_ACD_DEBUG'] = '1'
 
-		pid = self.start_process(args, env=env)
-		return pid
+		return self.start_process(args, env=env)
 
 	def is_verbose(self, process, log=True):
 		process = os.path.basename(process)
@@ -786,7 +781,7 @@ class Namespace:
 	def __str__(self):
 		ret = 'Namespace: %s\n' % self.name
 		ret += 'Processes:\n'
-		for p in self.processes:
+		for p in Process.get_all():
 			ret += '\t%s' % str(p)
 
 		ret += 'Radios:\n'
@@ -808,7 +803,6 @@ class TestContext(Namespace):
 	'''
 	def __init__(self, args):
 		self.name = None
-		self.processes = []
 		self.args = args
 		self.hw_config = None
 		self.hostapd = None
-- 
2.31.1

Re: [PATCH 1/6] test-runner: remove special case for "root" namespace

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 560 bytes --]

Hi James,

On 9/7/21 11:54 AM, James Prestwood wrote:
> This was a placeholder at one point but modules grew to depend on it
> being a string. Fix these dependencies and set the root namespace
> name to None so there is no more special case needed to handle both
> a named namespace and the original 'root' namespace.
> ---
>   autotests/util/hwsim.py |  8 ++++----
>   autotests/util/iwd.py   |  2 +-
>   tools/test-runner       | 10 ++--------
>   3 files changed, 7 insertions(+), 13 deletions(-)
> 

All applied, thanks.

Regards,
-Denis