[PATCH 1/7] test-runner: start dmesg process with start_process

[PATCH 1/7] test-runner: start dmesg process with start_process

[James Prestwood]

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

This avoids the need to pass in the context explicitly.
---
 tools/test-runner | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/test-runner b/tools/test-runner
index 965b0a57..70694e47 100755
--- a/tools/test-runner
+++ b/tools/test-runner
@@ -1178,7 +1178,7 @@ def run_auto_tests(ctx, args):
 
 	# Write out kernel log
 	if ctx.args.log:
-		Process(["dmesg"], ctx=ctx, wait=True)
+		ctx.start_process(["dmesg"], wait=True)
 
 def run_unit_tests(ctx, args):
 	os.chdir(args.testhome + '/unit')
-- 
2.26.2

[PATCH 2/7] test-runner: clear log dir before new log test run

[James Prestwood]

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

The log dir was never being cleaned out prior to a new logging
test run. This could leave old stale files around. Note that this
will remove any past log files so if you need them, you want to
make a copy before running test-runner with --log again.
---
 tools/test-runner | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/tools/test-runner b/tools/test-runner
index 70694e47..a390014b 100755
--- a/tools/test-runner
+++ b/tools/test-runner
@@ -1249,6 +1249,14 @@ def run_tests():
 
 	if args.log:
 		mount('logdir', args.log, '9p', 0, 'trans=virtio,version=9p2000.L')
+		# Clear out any log files from other test runs
+		for f in glob('%s/*' % args.log):
+			print("removing %s" % f)
+
+			if os.path.isdir(f):
+				shutil.rmtree(f)
+			else:
+				os.remove(f)
 	elif args.monitor:
 		parent = os.path.abspath(os.path.join(args.monitor, os.pardir))
 		mount('mondir', parent, '9p', 0, 'trans=virtio,version=9p2000.L')
-- 
2.26.2

[PATCH 3/7] test-runner: require root user to run with --monitor

[James Prestwood]

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

Since the monitor file requires touching the host file
system, root access is required.
---
 tools/test-runner | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tools/test-runner b/tools/test-runner
index a390014b..8bf8d363 100755
--- a/tools/test-runner
+++ b/tools/test-runner
@@ -1434,6 +1434,10 @@ class Main:
 			options += ' --log-uid %u' % uid
 
 		if self.args.monitor:
+			if os.environ.get('SUDO_GID', None) is None:
+				print("--monitor can only be used as root user")
+				quit()
+
 			self.args.monitor = os.path.abspath(self.args.monitor)
 			mon_parent_dir = os.path.abspath(os.path.join(self.args.monitor, os.pardir))
 
-- 
2.26.2

[PATCH 4/7] test-runner: refactor Process class

[James Prestwood]

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

The process class was quite hard to understand, and somewhat
fragile when multiple output options were needed like verbose
and logging, and in some cases even an additional output file.

To make things simpler we can have all processes output to a
temporary file (/tmp/<name>-out) and set a GLib IO watch on
that file. When the IO watch callback fires any additional
files (stdout, log files, output files) can be written to.

For wait=True processes we do not use an IO watch, but do
the same thing once the process exits, write to any additional
output files using the process output we already have.
---
 tools/test-runner | 134 ++++++++++++++++++++++++++++++----------------
 1 file changed, 88 insertions(+), 46 deletions(-)

diff --git a/tools/test-runner b/tools/test-runner
index 8bf8d363..0649109b 100755
--- a/tools/test-runner
+++ b/tools/test-runner
@@ -167,54 +167,67 @@ class Process:
 		self.wait = wait
 		self.name = args[0]
 		self.multi_test = multi_test
-		self.stdout = subprocess.PIPE
-		self.stderr = subprocess.PIPE
 		self.ret = None
 		self.ctx = ctx
-		set_stdout = False
+		self.write_fds = []
+		self.io_watch = None
 
 		if namespace:
 			self.args = ['ip', 'netns', 'exec', namespace]
 			self.args.extend(args)
 
-		if ctx:
-			if ctx.is_verbose(args[0]):
-				print("Verbose on for %s" % args[0])
-				set_stdout = True
+		#
+		# For simplicity all processes will write to a temp file
+		# (/tmp/<name>-out). If any verbose options are required this file
+		# will get an IO watch and write out any bytes to the needed FDs.
+		#
+		self.stdout = open('/tmp/%s-out' % self.name, 'a+')
+		self.io_position = self.stdout.tell()
 
-			if os.path.basename(args[0]) == ctx.args.gdb:
-				self.args = ['gdb', '--args']
-				self.args.extend(args)
-				set_stdout = True
+		if ctx:
+			# Verbose requested, add stdout/stderr to write FD list
+			if self.name in ctx.args.verbose:
+				self.write_fds.append(sys.__stdout__)
+				self.write_fds.append(sys.__stderr__)
 
+			# Add output file to FD list
 			if outfile:
-				set_stdout = True
-
-			# Anything labeled as multi_test isn't important to
-			# log. These are processes such as dbus-daemon and
-			# haveged.
-			if set_stdout:
-				if ctx.args.log:
-					test = os.path.basename(os.getcwd())
-					test_dir = '%s/%s' % (ctx.args.log, test)
-
-					if not path_exists(test_dir):
-						os.mkdir(test_dir)
-						os.chown(test_dir, int(ctx.args.log_uid), \
+				try:
+					f = open(outfile, 'w')
+				except Exception as e:
+					dbg(e)
+					exit(0)
+
+				if ctx.args.log_uid:
+					os.fchown(f.fileno(), int(ctx.args.log_uid), int(ctx.args.log_gid))
+
+				self.write_fds.append(f)
+
+			# Add log file to FD list
+			if ctx.args.log:
+				test = os.path.basename(os.getcwd())
+				test_dir = '%s/%s' % (ctx.args.log, test)
+
+				if not path_exists(test_dir):
+					os.mkdir(test_dir)
+					os.chown(test_dir, int(ctx.args.log_uid),
 								int(ctx.args.log_gid))
 
-					self.stdout = open('%s/%s' % (test_dir, args[0]), 'w+')
-					self.stderr = open('%s/%s' % (test_dir, args[0]), 'w+')
-				elif outfile:
-					self.stdout = open(outfile, 'w')
-					self.stderr = open(outfile, 'w')
-				elif not need_out:
-					self.stdout = sys.__stdout__
-					self.stderr = sys.__stderr__
+				f = open('%s/%s' % (test_dir, args[0]), 'a+')
+				os.fchown(f.fileno(), int(ctx.args.log_uid), int(ctx.args.log_gid))
+				self.write_fds.append(f)
 
-		self.pid = subprocess.Popen(self.args, stdout=self.stdout, \
-							stderr=self.stderr, env=env, \
-							cwd=os.getcwd())
+			#
+			# Only add an IO watch for long running processes. If
+			# the process is being waited for, the log/outfile bits
+			# will be handled after the process exists.
+			#
+			if self.write_fds != [] and not wait and not check:
+				self.io_watch = GLib.io_add_watch(self.stdout, GLib.IO_IN,
+								self.io_callback)
+
+		self.pid = subprocess.Popen(self.args, stdout=self.stdout, stderr=subprocess.STDOUT,
+						env=env, cwd=os.getcwd())
 
 		print("Starting process {}".format(self.pid.args))
 
@@ -223,25 +236,54 @@ class Process:
 
 		self.pid.wait()
 
-		if ctx and ctx.args.log or outfile:
-			self.stdout.seek(0)
-			self.out = self.stdout.read()
-		else:
-			self.out, _ = self.pid.communicate()
-
-			if self.out:
-				self.out = self.out.decode('utf-8')
-
+		self.stdout.seek(self.io_position)
+		self.out = self.stdout.read()
+		self.stdout.seek(0, 2)
 		self.ret = self.pid.returncode
 
+		#
+		# No IO callback for wait/check=True processes so write out
+		# the data to any FD's now.
+		#
+		if len(self.write_fds) > 0:
+			for fd in self.write_fds:
+				fd.write(self.out)
+
 		print("%s returned %d" % (args[0], self.ret))
 		if check and self.ret != 0:
 			raise subprocess.CalledProcessError(returncode=self.ret, cmd=self.args)
 
+	def io_callback(self, source, cb_condition):
+		#
+		# The file will have already been written to, meaning the seek
+		# position points to EOF. This is why the position is saved so
+		# we can seek to where we were last time, read data, and seek
+		# back to EOF.
+		#
+		source.seek(self.io_position)
+		data = source.read()
+
+		self.io_position += len(data)
+		source.seek(self.io_position)
+
+		if len(data) == 0:
+			return True
+
+		for f in self.write_fds:
+			f.write(data)
+
+		return True
+
 	def __del__(self):
 		print("Del process %s" % self.args)
-		self.stdout = None
-		self.stderr = None
+		self.stdout.close()
+
+		if self.io_watch:
+			GLib.source_remove(self.io_watch)
+
+		if self.write_fds != []:
+			for f in self.write_fds:
+				f.close()
 
 	def kill(self, force=False):
 		print("Killing process %s" % self.args)
-- 
2.26.2

[PATCH 5/7] test-runner: use unique name for namespace output files

[James Prestwood]

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

Output files in namespaces were not handled differently and would
end up overwriting/duplicating files from the root namespace. These
are now named /tmp/<process>-<namespace>-out.
---
 tools/test-runner | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/tools/test-runner b/tools/test-runner
index 0649109b..f898ae1e 100755
--- a/tools/test-runner
+++ b/tools/test-runner
@@ -172,6 +172,11 @@ class Process:
 		self.write_fds = []
 		self.io_watch = None
 
+		if not namespace:
+			self.output_name = '/tmp/%s-out' % self.name
+		else:
+			self.output_name = '/tmp/%s-%s-out' % (self.name, namespace)
+
 		if namespace:
 			self.args = ['ip', 'netns', 'exec', namespace]
 			self.args.extend(args)
@@ -181,7 +186,7 @@ class Process:
 		# (/tmp/<name>-out). If any verbose options are required this file
 		# will get an IO watch and write out any bytes to the needed FDs.
 		#
-		self.stdout = open('/tmp/%s-out' % self.name, 'a+')
+		self.stdout = open(self.output_name, 'a+')
 		self.io_position = self.stdout.tell()
 
 		if ctx:
@@ -285,6 +290,8 @@ class Process:
 			for f in self.write_fds:
 				f.close()
 
+		os.remove(self.output_name)
+
 	def kill(self, force=False):
 		print("Killing process %s" % self.args)
 
-- 
2.26.2

[PATCH 6/7] test-runner: add __str__ method to Process

[James Prestwood]

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

Printing out processes was done manually but instead we can
make Process printing extendable by adding its own __str__
method. This now will print if the process is a multi-test
process as well.
---
 tools/test-runner | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/tools/test-runner b/tools/test-runner
index f898ae1e..c9f624d4 100755
--- a/tools/test-runner
+++ b/tools/test-runner
@@ -310,6 +310,11 @@ class Process:
 			if waited > wait:
 				raise Exception("Timed out waiting for socket")
 
+	def __str__(self):
+		ret = str(self.args) + ' multi_test=%s' % str(self.multi_test)
+		ret += '\n'
+		return ret
+
 class Interface:
 	def __init__(self, name, config):
 		self.name = name
@@ -678,7 +683,7 @@ class Namespace:
 		ret = 'Namespace: %s\n' % self.name
 		ret += 'Processes:\n'
 		for p in self.processes:
-			ret += '\t%s\n' % str(p.args)
+			ret += '\t%s' % str(p)
 
 		ret += 'Radios:\n'
 		if len(self.radios) > 0:
-- 
2.26.2

[PATCH 7/7] test-runner: fix start_dbus and clean up config

[James Prestwood]

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

start_dbus was not returning the Process class which was
expected by the caller. This resulted in the config not
being cleaned up.
---
 tools/test-runner | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/tools/test-runner b/tools/test-runner
index c9f624d4..662931b1 100755
--- a/tools/test-runner
+++ b/tools/test-runner
@@ -553,7 +553,7 @@ class Namespace:
 			self.stop_process(p)
 
 		if self.dbus_pid and not self.dbus_pid.multi_test:
-			os.remove(self.dbus_address.split('=')[1])
+			os.remove(self.dbus_cfg)
 
 	def __del__(self):
 		print("Removing namespace %s" % self.name)
@@ -598,21 +598,23 @@ class Namespace:
 		global dbus_count
 
 		self.dbus_address = 'unix:path=/tmp/dbus%d' % dbus_count
-		dbus_cfg = '/tmp/dbus%d.conf' % dbus_count
+		self.dbus_cfg = '/tmp/dbus%d.conf' % dbus_count
 		dbus_count += 1
 
-		with open(dbus_cfg, 'w+') as f:
+		with open(self.dbus_cfg, 'w+') as f:
 			f.write(dbus_config)
 			f.write('<listen>%s</listen>\n' % self.dbus_address)
 			f.write('</busconfig>\n')
 
-		p = self.start_process(['dbus-daemon', '--config-file=%s' % dbus_cfg],
+		p = self.start_process(['dbus-daemon', '--config-file=%s' % self.dbus_cfg],
 					wait=False, multi_test=multi_test)
 
 		p.wait_for_socket(self.dbus_address.split('=')[1], wait=5)
 
 		self._bus = dbus.bus.BusConnection(address_or_type=self.dbus_address)
 
+		return p
+
 	def start_iwd(self, config_dir = '/tmp', storage_dir = '/tmp/iwd'):
 		args = []
 		iwd_radios = ','.join([r.name for r in self.radios if r.use == 'iwd'])
-- 
2.26.2

Re: [PATCH 1/7] test-runner: start dmesg process with start_process

[Denis Kenzior]

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

Hi James,

On 2/18/21 2:19 PM, James Prestwood wrote:
> This avoids the need to pass in the context explicitly.
> ---
>   tools/test-runner | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 

All applied, thanks.

Regards,
-Denis