2:54 a.m.
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
2:54 a.m.
New subject: [PATCH 2/6] test-runner: don't use start_process for transient processes
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
2:54 a.m.
New subject: [PATCH 3/6] test-runner: fix process cleanup
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
2:54 a.m.
New subject: [PATCH 4/6] test-runner: use Process to start hostapd
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
2:54 a.m.
New subject: [PATCH 5/6] test-runner: write out separators in log files
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
2:54 a.m.
New subject: [PATCH 6/6] test-runner: move process tracking out of Namespace
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
3:46 a.m.
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
376
days inactive
376
days old
6 comments
2 participants
participants (2)
-
Denis Kenzior -
James Prestwood