From 1a90c470fba225b69d5f3d4633464d8a013bf5b9 Mon Sep 17 00:00:00 2001 From: Douglas Raillard Date: Thu, 30 May 2024 10:41:50 +0100 Subject: [PATCH 1/7] connection: Support all signals in BackgroundCommand.send_signal() Support sending any signal to background commands, instead of only supporting properly SIGKILL/SIGTERM/SIGQUIT. The main issue is figuring out what PID to send the signal to, as the devlib API allows running a whole snippet of shell script that typically is wrapped under many layers of sh -c and sudo calls. In order to lift the ambiguity, the user has access to a "devlib-signal-target" command that points devlib at what process should be the target of signals: # Run a "setup" command, then the main command that will receive the # signals cmd = 'echo setup; devlib-signal-target echo hello world' with target.background(cmd) as bg: bg.communicate() The devlib-signal-target script can only be used once per background command, so that it is never ambiguous what process is targeted, and so that the Python code can cache the target PID. Subsequent invocations of devlib-signal-target will fail. --- devlib/bin/scripts/devlib-signal-target | 20 ++ devlib/connection.py | 147 ++++++++++---- devlib/host.py | 27 ++- devlib/target.py | 11 +- devlib/utils/android.py | 37 ++-- devlib/utils/ssh.py | 244 ++++++++++++------------ 6 files changed, 297 insertions(+), 189 deletions(-) create mode 100644 devlib/bin/scripts/devlib-signal-target diff --git a/devlib/bin/scripts/devlib-signal-target b/devlib/bin/scripts/devlib-signal-target new file mode 100644 index 000000000..26ac6fe84 --- /dev/null +++ b/devlib/bin/scripts/devlib-signal-target @@ -0,0 +1,20 @@ +( + # If there is no data dir, it means we are not running as a background + # command so we just do nothing + if [ -e "$_DEVLIB_BG_CMD_DATA_DIR" ]; then + pid_file="$_DEVLIB_BG_CMD_DATA_DIR/pid" + # Atomically check if the PID file already exist and make the write + # fail if it already does. This way we don't have any race condition + # with the Python API, as there is either no PID or the same PID for + # the duration of the command + set -o noclobber + if ! printf "%u\n" $$ > "$pid_file"; then + echo "$0 was already called for this command" >&2 + exit 1 + fi + fi +) || exit $? + +# Use exec so that the PID of the command we run is the same as the current $$ +# PID that we just registered +exec "$@" diff --git a/devlib/connection.py b/devlib/connection.py index 460997580..99055a3c1 100644 --- a/devlib/connection.py +++ b/devlib/connection.py @@ -17,6 +17,7 @@ from contextlib import contextmanager, nullcontext from shlex import quote import os +from pathlib import Path import signal import subprocess import threading @@ -25,14 +26,11 @@ import select import fcntl -from devlib.utils.misc import InitCheckpoint +from devlib.utils.misc import InitCheckpoint, memoized _KILL_TIMEOUT = 3 -def _kill_pgid_cmd(pgid, sig, busybox): - return '{} kill -{} -{}'.format(busybox, sig.value, pgid) - def _popen_communicate(bg, popen, input, timeout): try: stdout, stderr = popen.communicate(input=input, timeout=timeout) @@ -130,8 +128,11 @@ class BackgroundCommand(ABC): semantic as :class:`subprocess.Popen`. """ - def __init__(self, conn): + def __init__(self, conn, data_dir, cmd, as_root): self.conn = conn + self._data_dir = data_dir + self.as_root = as_root + self.cmd = cmd # Poll currently opened background commands on that connection to make # them deregister themselves if they are completed. This avoids @@ -147,15 +148,65 @@ def __init__(self, conn): conn._current_bg_cmds.add(self) + @classmethod + def from_factory(cls, conn, cmd, as_root, make_init_kwargs): + cmd, data_dir = cls._with_data_dir(conn, cmd) + return cls( + conn=conn, + data_dir=data_dir, + cmd=cmd, + as_root=as_root, + **make_init_kwargs(cmd), + ) + def _deregister(self): try: self.conn._current_bg_cmds.remove(self) except KeyError: pass - @abstractmethod - def _send_signal(self, sig): - pass + @property + def _pid_file(self): + return str(Path(self._data_dir, 'pid')) + + @property + @memoized + def _targeted_pid(self): + """ + PID of the process pointed at by ``devlib-signal-target`` command. + """ + path = quote(self._pid_file) + busybox = quote(self.conn.busybox) + + def execute(cmd): + return self.conn.execute(cmd, as_root=self.as_root) + + while self.poll() is None: + try: + pid = execute(f'{busybox} cat {path}') + except subprocess.CalledProcessError: + time.sleep(0.01) + else: + if pid.endswith('\n'): + return int(pid.strip()) + else: + # We got a partial write in the PID file + continue + + raise ValueError(f'The background commmand did not use devlib-signal-target wrapper to designate which command should be the target of signals') + + @classmethod + def _with_data_dir(cls, conn, cmd): + busybox = quote(conn.busybox) + data_dir = conn.execute(f'{busybox} mktemp -d').strip() + cmd = f'_DEVLIB_BG_CMD_DATA_DIR={data_dir} exec {busybox} sh -c {quote(cmd)}' + return cmd, data_dir + + def _cleanup_data_dir(self): + path = quote(self._data_dir) + busybox = quote(self.conn.busybox) + cmd = f'{busybox} rm -r {path} || true' + self.conn.execute(cmd, as_root=self.as_root) def send_signal(self, sig): """ @@ -165,8 +216,29 @@ def send_signal(self, sig): :param signal: Signal to send. :type signal: signal.Signals """ + + def execute(cmd): + return self.conn.execute(cmd, as_root=self.as_root) + + def send(sig): + busybox = quote(self.conn.busybox) + # If the command has already completed, we don't want to send a + # signal to another process that might have gotten that PID in the + # meantime. + if self.poll() is None: + if sig in (signal.SIGTERM, signal.SIGQUIT, signal.SIGKILL): + # Use -PGID to target a process group rather than just the + # process itself. This will work in any condition and will + # not require cooperation from the command. + execute(f'{busybox} kill -{sig.value} -{self.pid}') + else: + # Other signals require cooperation from the shell command + # so that it points to a specific process using + # devlib-signal-target + pid = self._targeted_pid + execute(f'{busybox} kill -{sig.value} {pid}') try: - return self._send_signal(sig) + return send(sig) finally: # Deregister if the command has finished self.poll() @@ -287,6 +359,7 @@ def close(self): return self._close() finally: self._deregister() + self._cleanup_data_dir() def __enter__(self): return self @@ -300,13 +373,15 @@ class PopenBackgroundCommand(BackgroundCommand): :class:`subprocess.Popen`-based background command. """ - def __init__(self, conn, popen): - super().__init__(conn=conn) + def __init__(self, conn, data_dir, cmd, as_root, popen): + super().__init__( + conn=conn, + data_dir=data_dir, + cmd=cmd, + as_root=as_root, + ) self.popen = popen - def _send_signal(self, sig): - return os.killpg(self.popen.pid, sig) - @property def stdin(self): return self.popen.stdin @@ -354,26 +429,20 @@ class ParamikoBackgroundCommand(BackgroundCommand): """ :mod:`paramiko`-based background command. """ - def __init__(self, conn, chan, pid, as_root, cmd, stdin, stdout, stderr, redirect_thread): - super().__init__(conn=conn) + def __init__(self, conn, data_dir, cmd, as_root, chan, pid, stdin, stdout, stderr, redirect_thread): + super().__init__( + conn=conn, + data_dir=data_dir, + cmd=cmd, + as_root=as_root, + ) + self.chan = chan - self.as_root = as_root self._pid = pid self._stdin = stdin self._stdout = stdout self._stderr = stderr self.redirect_thread = redirect_thread - self.cmd = cmd - - def _send_signal(self, sig): - # If the command has already completed, we don't want to send a signal - # to another process that might have gotten that PID in the meantime. - if self.poll() is not None: - return - # Use -PGID to target a process group rather than just the process - # itself - cmd = _kill_pgid_cmd(self.pid, sig, self.conn.busybox) - self.conn.execute(cmd, as_root=self.as_root) @property def pid(self): @@ -517,18 +586,16 @@ class AdbBackgroundCommand(BackgroundCommand): ``adb``-based background command. """ - def __init__(self, conn, adb_popen, pid, as_root): - super().__init__(conn=conn) - self.as_root = as_root + def __init__(self, conn, data_dir, cmd, as_root, adb_popen, pid): + super().__init__( + conn=conn, + data_dir=data_dir, + cmd=cmd, + as_root=as_root, + ) self.adb_popen = adb_popen self._pid = pid - def _send_signal(self, sig): - self.conn.execute( - _kill_pgid_cmd(self.pid, sig, self.conn.busybox), - as_root=self.as_root, - ) - @property def stdin(self): return self.adb_popen.stdin @@ -638,7 +705,7 @@ def cancel(self): class PopenTransferHandle(TransferHandleBase): - def __init__(self, bg_cmd, dest, direction, *args, **kwargs): + def __init__(self, popen, dest, direction, *args, **kwargs): super().__init__(*args, **kwargs) if direction == 'push': @@ -650,7 +717,7 @@ def __init__(self, bg_cmd, dest, direction, *args, **kwargs): self.sample_size = lambda: sample_size(dest) - self.bg_cmd = bg_cmd + self.popen = popen self.last_sample = 0 @staticmethod @@ -671,7 +738,7 @@ def _push_dest_size(self, dest): return int(out.split()[0]) def cancel(self): - self.bg_cmd.cancel() + self.popen.terminate() def isactive(self): try: diff --git a/devlib/host.py b/devlib/host.py index a9958a349..a65b00f9f 100644 --- a/devlib/host.py +++ b/devlib/host.py @@ -147,16 +147,25 @@ def background(self, command, stdout=subprocess.PIPE, stderr=subprocess.PIPE, as def preexec_fn(): os.setpgrp() - popen = subprocess.Popen( - command, - stdout=stdout, - stderr=stderr, - stdin=subprocess.PIPE, - shell=True, - preexec_fn=preexec_fn, + def make_init_kwargs(command): + popen = subprocess.Popen( + command, + stdout=stdout, + stderr=stderr, + stdin=subprocess.PIPE, + shell=True, + preexec_fn=preexec_fn, + ) + return dict( + popen=popen, + ) + + return PopenBackgroundCommand.from_factory( + conn=self, + cmd=command, + as_root=as_root, + make_init_kwargs=make_init_kwargs, ) - bg_cmd = PopenBackgroundCommand(self, popen) - return bg_cmd def _close(self): pass diff --git a/devlib/target.py b/devlib/target.py index 44e7fc685..41095ccfb 100644 --- a/devlib/target.py +++ b/devlib/target.py @@ -281,7 +281,7 @@ def page_size_kb(self): @property def shutils(self): if self._shutils is None: - self._setup_shutils() + self._setup_scripts() return self._shutils def is_running(self, comm): @@ -591,7 +591,7 @@ def wait_boot_complete(self, timeout=10): @asyn.asyncf async def setup(self, executables=None): - await self._setup_shutils.asyn() + await self._setup_scripts.asyn() for host_exe in (executables or []): # pylint: disable=superfluous-parens await self.install.asyn(host_exe) @@ -1562,8 +1562,9 @@ def install_module(self, mod, **params): # internal methods @asyn.asyncf - async def _setup_shutils(self): - shutils_ifile = os.path.join(PACKAGE_BIN_DIRECTORY, 'scripts', 'shutils.in') + async def _setup_scripts(self): + scripts = os.path.join(PACKAGE_BIN_DIRECTORY, 'scripts') + shutils_ifile = os.path.join(scripts, 'shutils.in') with open(shutils_ifile) as fh: lines = fh.readlines() with tempfile.TemporaryDirectory() as folder: @@ -1574,6 +1575,8 @@ async def _setup_shutils(self): ofile.write(line) self._shutils = await self.install.asyn(shutils_ofile) + await self.install.asyn(os.path.join(scripts, 'devlib-signal-target')) + @asyn.asyncf @call_conn async def _execute_util(self, command, timeout=None, check_exit_code=True, as_root=False): diff --git a/devlib/utils/android.py b/devlib/utils/android.py index c77a86446..001cb93be 100755 --- a/devlib/utils/android.py +++ b/devlib/utils/android.py @@ -40,7 +40,7 @@ from devlib.exception import TargetTransientError, TargetStableError, HostError, TargetTransientCalledProcessError, TargetStableCalledProcessError, AdbRootError from devlib.utils.misc import check_output, which, ABI_MAP, redirect_streams, get_subprocess -from devlib.connection import ConnectionBase, AdbBackgroundCommand, PopenBackgroundCommand, PopenTransferHandle +from devlib.connection import ConnectionBase, AdbBackgroundCommand, PopenTransferHandle logger = logging.getLogger('android') @@ -341,7 +341,7 @@ def _push_pull(self, action, sources, dest, timeout): if timeout: adb_command(self.device, command, timeout=timeout, adb_server=self.adb_server, adb_port=self.adb_port) else: - bg_cmd = adb_command_background( + popen = adb_command_popen( device=self.device, conn=self, command=command, @@ -351,12 +351,12 @@ def _push_pull(self, action, sources, dest, timeout): handle = PopenTransferHandle( manager=self.transfer_manager, - bg_cmd=bg_cmd, + popen=popen, dest=dest, direction=action ) - with bg_cmd, self.transfer_manager.manage(sources, dest, action, handle): - bg_cmd.communicate() + with popen, self.transfer_manager.manage(sources, dest, action, handle): + popen.communicate() # pylint: disable=unused-argument def execute(self, command, timeout=None, check_exit_code=False, @@ -387,12 +387,18 @@ def background(self, command, stdout=subprocess.PIPE, stderr=subprocess.PIPE, as return bg_cmd def _background(self, command, stdout, stderr, as_root): - adb_shell, pid = adb_background_shell(self, command, stdout, stderr, as_root) - bg_cmd = AdbBackgroundCommand( + def make_init_kwargs(command): + adb_popen, pid = adb_background_shell(self, command, stdout, stderr, as_root) + return dict( + adb_popen=adb_popen, + pid=pid, + ) + + bg_cmd = AdbBackgroundCommand.from_factory( conn=self, - adb_popen=adb_shell, - pid=pid, - as_root=as_root + cmd=command, + as_root=as_root, + make_init_kwargs=make_init_kwargs, ) return bg_cmd @@ -756,12 +762,11 @@ def adb_command(device, command, timeout=None, adb_server=None, adb_port=None): return output -def adb_command_background(device, conn, command, adb_server=None, adb_port=None): - full_command = get_adb_command(device, command, adb_server, adb_port) - logger.debug(full_command) - popen = get_subprocess(full_command, shell=True) - cmd = PopenBackgroundCommand(conn=conn, popen=popen) - return cmd +def adb_command_popen(device, conn, command, adb_server=None, adb_port=None): + command = get_adb_command(device, command, adb_server, adb_port) + logger.debug(command) + popen = get_subprocess(command, shell=True) + return popen def grant_app_permissions(target, package): diff --git a/devlib/utils/ssh.py b/devlib/utils/ssh.py index 9fe4c613a..f9bfacdcd 100644 --- a/devlib/utils/ssh.py +++ b/devlib/utils/ssh.py @@ -602,138 +602,142 @@ def background(self, command, stdout=subprocess.PIPE, stderr=subprocess.PIPE, as return self._background(command, stdout, stderr, as_root) def _background(self, command, stdout, stderr, as_root): - orig_command = command - stdout, stderr, command = redirect_streams(stdout, stderr, command) - - command = "printf '%s\n' $$; exec sh -c {}".format(quote(command)) - channel = self._make_channel() - - def executor(cmd, timeout): - channel.exec_command(cmd) - # Read are not buffered so we will always get the data as soon as - # they arrive - return ( - channel.makefile_stdin('w', 0), - channel.makefile(), - channel.makefile_stderr(), - ) + def make_init_kwargs(command): + _stdout, _stderr, _command = redirect_streams(stdout, stderr, command) + + _command = "printf '%s\n' $$; exec sh -c {}".format(quote(_command)) + channel = self._make_channel() + + def executor(cmd, timeout): + channel.exec_command(cmd) + # Read are not buffered so we will always get the data as soon as + # they arrive + return ( + channel.makefile_stdin('w', 0), + channel.makefile(), + channel.makefile_stderr(), + ) - stdin, stdout_in, stderr_in = self._execute_command( - command, - as_root=as_root, - log=False, - timeout=None, - executor=executor, - ) - pid = stdout_in.readline() - if not pid: - stderr = stderr_in.read() - if channel.exit_status_ready(): - ret = channel.recv_exit_status() - else: - ret = 126 - raise subprocess.CalledProcessError( - ret, - command, - b'', - stderr, + stdin, stdout_in, stderr_in = self._execute_command( + _command, + as_root=as_root, + log=False, + timeout=None, + executor=executor, ) - pid = int(pid) - - def create_out_stream(stream_in, stream_out): - """ - Create a pair of file-like objects. The first one is used to read - data and the second one to write. - """ - - if stream_out == subprocess.DEVNULL: - r, w = None, None - # When asked for a pipe, we just give the file-like object as the - # reading end and no writing end, since paramiko already writes to - # it - elif stream_out == subprocess.PIPE: - r, w = os.pipe() - r = os.fdopen(r, 'rb') - w = os.fdopen(w, 'wb') - # Turn a file descriptor into a file-like object - elif isinstance(stream_out, int) and stream_out >= 0: - r = os.fdopen(stream_in, 'rb') - w = os.fdopen(stream_out, 'wb') - # file-like object - else: - r = stream_in - w = stream_out + pid = stdout_in.readline() + if not pid: + _stderr = stderr_in.read() + if channel.exit_status_ready(): + ret = channel.recv_exit_status() + else: + ret = 126 + raise subprocess.CalledProcessError( + ret, + _command, + b'', + _stderr, + ) + pid = int(pid) + + def create_out_stream(stream_in, stream_out): + """ + Create a pair of file-like objects. The first one is used to read + data and the second one to write. + """ + + if stream_out == subprocess.DEVNULL: + r, w = None, None + # When asked for a pipe, we just give the file-like object as the + # reading end and no writing end, since paramiko already writes to + # it + elif stream_out == subprocess.PIPE: + r, w = os.pipe() + r = os.fdopen(r, 'rb') + w = os.fdopen(w, 'wb') + # Turn a file descriptor into a file-like object + elif isinstance(stream_out, int) and stream_out >= 0: + r = os.fdopen(stream_in, 'rb') + w = os.fdopen(stream_out, 'wb') + # file-like object + else: + r = stream_in + w = stream_out - return (r, w) + return (r, w) - out_streams = { - name: create_out_stream(stream_in, stream_out) - for stream_in, stream_out, name in ( - (stdout_in, stdout, 'stdout'), - (stderr_in, stderr, 'stderr'), - ) - } + out_streams = { + name: create_out_stream(stream_in, stream_out) + for stream_in, stream_out, name in ( + (stdout_in, _stdout, 'stdout'), + (stderr_in, _stderr, 'stderr'), + ) + } + + def redirect_thread_f(stdout_in, stderr_in, out_streams, select_timeout): + def callback(out_streams, name, chunk): + try: + r, w = out_streams[name] + except KeyError: + return out_streams + + try: + w.write(chunk) + # Write failed + except ValueError: + # Since that stream is now closed, stop trying to write to it + del out_streams[name] + # If that was the last open stream, we raise an + # exception so the thread can terminate. + if not out_streams: + raise - def redirect_thread_f(stdout_in, stderr_in, out_streams, select_timeout): - def callback(out_streams, name, chunk): - try: - r, w = out_streams[name] - except KeyError: return out_streams try: - w.write(chunk) - # Write failed + _read_paramiko_streams(stdout_in, stderr_in, select_timeout, callback, copy.copy(out_streams)) + # The streams closed while we were writing to it, the job is done here except ValueError: - # Since that stream is now closed, stop trying to write to it - del out_streams[name] - # If that was the last open stream, we raise an - # exception so the thread can terminate. - if not out_streams: - raise - - return out_streams - - try: - _read_paramiko_streams(stdout_in, stderr_in, select_timeout, callback, copy.copy(out_streams)) - # The streams closed while we were writing to it, the job is done here - except ValueError: - pass - - # Make sure the writing end are closed proper since we are not - # going to write anything anymore - for r, w in out_streams.values(): - w.flush() - if r is not w and w is not None: - w.close() - - # If there is anything we need to redirect to, spawn a thread taking - # care of that - select_timeout = 1 - thread_out_streams = { - name: (r, w) - for name, (r, w) in out_streams.items() - if w is not None - } - redirect_thread = threading.Thread( - target=redirect_thread_f, - args=(stdout_in, stderr_in, thread_out_streams, select_timeout), - # The thread will die when the main thread dies - daemon=True, - ) - redirect_thread.start() + pass + + # Make sure the writing end are closed proper since we are not + # going to write anything anymore + for r, w in out_streams.values(): + w.flush() + if r is not w and w is not None: + w.close() + + # If there is anything we need to redirect to, spawn a thread taking + # care of that + select_timeout = 1 + thread_out_streams = { + name: (r, w) + for name, (r, w) in out_streams.items() + if w is not None + } + redirect_thread = threading.Thread( + target=redirect_thread_f, + args=(stdout_in, stderr_in, thread_out_streams, select_timeout), + # The thread will die when the main thread dies + daemon=True, + ) + redirect_thread.start() + + return dict( + chan=channel, + pid=pid, + stdin=stdin, + # We give the reading end to the consumer of the data + stdout=out_streams['stdout'][0], + stderr=out_streams['stderr'][0], + redirect_thread=redirect_thread, + ) - return ParamikoBackgroundCommand( + return ParamikoBackgroundCommand.from_factory( conn=self, + cmd=command, as_root=as_root, - chan=channel, - pid=pid, - stdin=stdin, - # We give the reading end to the consumer of the data - stdout=out_streams['stdout'][0], - stderr=out_streams['stderr'][0], - redirect_thread=redirect_thread, - cmd=orig_command, + make_init_kwargs=make_init_kwargs, ) def _close(self): From 177762df10b0676de4ff8c27605a1f611c24c2b9 Mon Sep 17 00:00:00 2001 From: Douglas Raillard Date: Thu, 30 May 2024 14:59:03 +0100 Subject: [PATCH 2/7] ftrace: Add write-to-disk mode Allow using trace-cmd record that continuously dump the trace to disk, allowing to overcome the buffer size limitations when recording for extended periods of time. --- devlib/collector/ftrace.py | 73 +++++++++++++++++++++++++++++--------- 1 file changed, 57 insertions(+), 16 deletions(-) diff --git a/devlib/collector/ftrace.py b/devlib/collector/ftrace.py index 9a887efdf..319cd36cc 100644 --- a/devlib/collector/ftrace.py +++ b/devlib/collector/ftrace.py @@ -21,6 +21,7 @@ import sys import contextlib from shlex import quote +import signal from devlib.collector import (CollectorBase, CollectorOutput, CollectorOutputEntry) @@ -71,6 +72,7 @@ def __init__(self, target, report_on_target=False, trace_clock='local', saved_cmdlines_nr=4096, + mode='write-to-memory', ): super(FtraceCollector, self).__init__(target) self.events = events if events is not None else DEFAULT_EVENTS @@ -98,6 +100,8 @@ def __init__(self, target, self.trace_clock = trace_clock self.saved_cmdlines_nr = saved_cmdlines_nr self._reset_needed = True + self.mode = mode + self._bg_cmd = None # pylint: disable=bad-whitespace # Setup tracing paths @@ -288,18 +292,33 @@ async def start(self): with contextlib.suppress(TargetStableError): self.target.write_value('/proc/sys/kernel/kptr_restrict', 0) - self.target.execute( - '{} start -B devlib {buffer_size} {cmdlines_size} {clock} {events} {tracer} {functions}'.format( - self.target_binary, - events=self.event_string, - tracer=tracer_string, - functions=tracecmd_functions, - buffer_size='-b {}'.format(self.buffer_size) if self.buffer_size is not None else '', - clock='-C {}'.format(self.trace_clock) if self.trace_clock else '', - cmdlines_size='--cmdlines-size {}'.format(self.saved_cmdlines_nr) if self.saved_cmdlines_nr is not None else '', - ), - as_root=True, + params = '-B devlib {buffer_size} {cmdlines_size} {clock} {events} {tracer} {functions}'.format( + events=self.event_string, + tracer=tracer_string, + functions=tracecmd_functions, + buffer_size='-b {}'.format(self.buffer_size) if self.buffer_size is not None else '', + clock='-C {}'.format(self.trace_clock) if self.trace_clock else '', + cmdlines_size='--cmdlines-size {}'.format(self.saved_cmdlines_nr) if self.saved_cmdlines_nr is not None else '', ) + + mode = self.mode + if mode == 'write-to-disk': + bg_cmd = self.target.background( + # cd into the working_directory first to workaround this issue: + # https://lore.kernel.org/linux-trace-devel/20240119162743.1a107fa9@gandalf.local.home/ + f'cd {self.target.working_directory} && devlib-signal-target {self.target_binary} record -o {quote(self.target_output_file)} {params}', + as_root=True, + ) + assert self._bg_cmd is None + self._bg_cmd = bg_cmd.__enter__() + elif mode == 'write-to-memory': + self.target.execute( + f'{self.target_binary} start {params}', + as_root=True, + ) + else: + raise ValueError(f'Unknown mode {mode}') + if self.automark: self.mark_start() if 'cpufreq' in self.target.modules: @@ -334,8 +353,21 @@ def stop(self): self.stop_time = time.time() if self.automark: self.mark_stop() - self.target.execute('{} stop -B devlib'.format(self.target_binary), - timeout=TIMEOUT, as_root=True) + + mode = self.mode + if mode == 'write-to-disk': + bg_cmd = self._bg_cmd + self._bg_cmd = None + assert bg_cmd is not None + bg_cmd.send_signal(signal.SIGINT) + bg_cmd.communicate() + bg_cmd.__exit__(None, None, None) + elif mode == 'write-to-memory': + self.target.execute('{} stop -B devlib'.format(self.target_binary), + timeout=TIMEOUT, as_root=True) + else: + raise ValueError(f'Unknown mode {mode}') + self._reset_needed = True def set_output(self, output_path): @@ -346,9 +378,18 @@ def set_output(self, output_path): def get_data(self): if self.output_path is None: raise RuntimeError("Output path was not set.") - self.target.execute('{0} extract -B devlib -o {1}; chmod 666 {1}'.format(self.target_binary, - self.target_output_file), - timeout=TIMEOUT, as_root=True) + + busybox = quote(self.target.busybox) + + mode = self.mode + if mode == 'write-to-disk': + # Interrupting trace-cmd record will make it create the file + pass + elif mode == 'write-to-memory': + cmd = f'{self.target_binary} extract -B devlib -o {self.target_output_file} && {busybox} chmod 666 {self.target_output_file}' + self.target.execute(cmd, timeout=TIMEOUT, as_root=True) + else: + raise ValueError(f'Unknown mode {mode}') # The size of trace.dat will depend on how long trace-cmd was running. # Therefore timout for the pull command must also be adjusted From 4743ba6d7659a8170a10990292d197cc9f0861f1 Mon Sep 17 00:00:00 2001 From: Douglas Raillard Date: Wed, 20 Nov 2024 12:29:43 +0000 Subject: [PATCH 3/7] target: Rework temp folder management * Make use of Target.make_temp() in Target._xfer_cache_path() to deduplicate temp folder creation code * Introduce Target.tmp_directory attribute for the root of temporary files. * Make Target.tempfile() use Target.tmp_directory * Rework the Target._resolve_paths() implementations: * Target.tmp_directory is set to a default returned by "mktemp -d". This way, if "mktemp -d" works out of of the box, all devlib temporary files will be located in the expected location for the that operating system. * Target._resolve_paths() must set Target.working_directory and that is checked with an assert. Other directories have defaults based on this if _resolve_paths() does not set them. --- devlib/target.py | 100 +++++++++++++++++++++++++++++------------------ 1 file changed, 62 insertions(+), 38 deletions(-) diff --git a/devlib/target.py b/devlib/target.py index 41095ccfb..f980ea31e 100644 --- a/devlib/target.py +++ b/devlib/target.py @@ -320,6 +320,7 @@ def __init__(self, conn_cls=None, is_container=False, max_async=50, + tmp_directory=None, ): self._lock = threading.RLock() @@ -347,6 +348,7 @@ def __init__(self, self.connection_settings['platform'] = self.platform self.working_directory = working_directory self.executables_directory = executables_directory + self.tmp_directory = tmp_directory self.load_default_modules = load_default_modules self.shell_prompt = bytes_regex(shell_prompt) self.conn_cls = conn_cls @@ -356,7 +358,6 @@ def __init__(self, self._installed_modules = {} self._cache = {} self._shutils = None - self._file_transfer_cache = None self._max_async = max_async self.busybox = None @@ -477,10 +478,35 @@ async def connect(self, timeout=None, check_boot_completed=True, max_async=None) self.wait_boot_complete(timeout) self.check_connection() self._resolve_paths() - self.execute('mkdir -p {}'.format(quote(self.working_directory))) - self.execute('mkdir -p {}'.format(quote(self.executables_directory))) + assert self.working_directory + if self.executables_directory is None: + self.executables_directory = self.path.join( + self.working_directory, + 'bin' + ) + + for path in (self.working_directory, self.executables_directory): + self.makedirs(path) + self.busybox = self.install(os.path.join(PACKAGE_BIN_DIRECTORY, self.abi, 'busybox'), timeout=30) self.conn.busybox = self.busybox + + # If neither the mktemp call nor _resolve_paths() managed to get a + # temporary directory, we just make one in the working directory. + if self.tmp_directory is None: + assert self.busybox + try: + tmp = await self.execute.asyn(f'{quote(self.busybox)} mktemp -d') + except Exception: + # Some Android platforms don't have a working mktemp unless + # TMPDIR is set, so we let AndroidTarget._resolve_paths() deal + # with finding a suitable location. + tmp = self.path.join(self.working_directory, 'tmp') + else: + tmp = tmp.strip() + self.tmp_directory = tmp + self.makedirs(self.tmp_directory) + self._detect_max_async(max_async or self._max_async) self.platform.update_from_target(self) self._update_modules('connected') @@ -602,8 +628,6 @@ async def setup(self, executables=None): # Initialize modules which requires Busybox (e.g. shutil dependent tasks) self._update_modules('setup') - await self.execute.asyn('mkdir -p {}'.format(quote(self._file_transfer_cache))) - def reboot(self, hard=False, connect=True, timeout=180): if hard: if not self.has('hard_reset'): @@ -637,24 +661,11 @@ async def _xfer_cache_path(self, name): Context manager to provide a unique path in the transfer cache with the basename of the given name. """ - # Use a UUID to avoid race conditions on the target side - xfer_uuid = uuid.uuid4().hex - folder = self.path.join(self._file_transfer_cache, xfer_uuid) # Make sure basename will work on folders too name = os.path.normpath(name) - # Ensure the name is relative so that os.path.join() will actually - # join the paths rather than ignoring the first one. - name = './{}'.format(os.path.basename(name)) - - check_rm = False - try: - await self.makedirs.asyn(folder) - # Don't check the exit code as the folder might not even exist - # before this point, if creating it failed - check_rm = True - yield self.path.join(folder, name) - finally: - await self.execute.asyn('rm -rf -- {}'.format(quote(folder)), check_exit_code=check_rm) + name = os.path.basename(name) + async with self.make_temp() as tmp: + yield self.path.join(tmp, name) @asyn.asyncf async def _prepare_xfer(self, action, sources, dest, pattern=None, as_root=False): @@ -944,15 +955,17 @@ async def get_directory(self, source_dir, dest, as_root=False): # execution def _prepare_cmd(self, command, force_locale): + tmpdir = f'TMPDIR={quote(self.tmp_directory)}' if self.tmp_directory else '' + # Force the locale if necessary for more predictable output if force_locale: # Use an explicit export so that the command is allowed to be any # shell statement, rather than just a command invocation - command = 'export LC_ALL={} && {}'.format(quote(force_locale), command) + command = f'export LC_ALL={quote(force_locale)} {tmpdir} && {command}' # Ensure to use deployed command when availables if self.executables_directory: - command = "export PATH={}:$PATH && {}".format(quote(self.executables_directory), command) + command = f"export PATH={quote(self.executables_directory)}:$PATH && {command}" return command @@ -1778,6 +1791,7 @@ def __init__(self, conn_cls=SshConnection, is_container=False, max_async=50, + tmp_directory=None, ): super(LinuxTarget, self).__init__(connection_settings=connection_settings, platform=platform, @@ -1789,7 +1803,9 @@ def __init__(self, shell_prompt=shell_prompt, conn_cls=conn_cls, is_container=is_container, - max_async=max_async) + max_async=max_async, + tmp_directory=tmp_directory, + ) def wait_boot_complete(self, timeout=10): pass @@ -1871,10 +1887,8 @@ async def capture_screen(self, filepath): def _resolve_paths(self): if self.working_directory is None: + # This usually lands in the home directory self.working_directory = self.path.join(self.execute("pwd").strip(), 'devlib-target') - self._file_transfer_cache = self.path.join(self.working_directory, '.file-cache') - if self.executables_directory is None: - self.executables_directory = self.path.join(self.working_directory, 'bin') class AndroidTarget(Target): @@ -1989,6 +2003,7 @@ def __init__(self, package_data_directory="/data/data", is_container=False, max_async=50, + tmp_directory=None, ): super(AndroidTarget, self).__init__(connection_settings=connection_settings, platform=platform, @@ -2000,7 +2015,9 @@ def __init__(self, shell_prompt=shell_prompt, conn_cls=conn_cls, is_container=is_container, - max_async=max_async) + max_async=max_async, + tmp_directory=tmp_directory, + ) self.package_data_directory = package_data_directory self._init_logcat_lock() @@ -2590,9 +2607,16 @@ async def homescreen(self): def _resolve_paths(self): if self.working_directory is None: self.working_directory = self.path.join(self.external_storage, 'devlib-target') - self._file_transfer_cache = self.path.join(self.working_directory, '.file-cache') + if self.tmp_directory is None: + # Do not rely on the generic default here, as we need to provide an + # android-specific default in case it fails. + try: + tmp = self.execute(f'{quote(self.busybox)} mktemp -d') + except Exception: + tmp = '/data/local/tmp' + self.tmp_directory = tmp if self.executables_directory is None: - self.executables_directory = '/data/local/tmp/bin' + self.executables_directory = self.path.join(self.tmp_directory, 'bin') @asyn.asyncf async def _ensure_executables_directory_is_writable(self): @@ -3067,6 +3091,7 @@ def __init__(self, conn_cls=LocalConnection, is_container=False, max_async=50, + tmp_directory=None, ): super(LocalLinuxTarget, self).__init__(connection_settings=connection_settings, platform=platform, @@ -3078,14 +3103,13 @@ def __init__(self, shell_prompt=shell_prompt, conn_cls=conn_cls, is_container=is_container, - max_async=max_async) + max_async=max_async, + tmp_directory=tmp_directory, + ) def _resolve_paths(self): if self.working_directory is None: self.working_directory = '/tmp/devlib-target' - self._file_transfer_cache = self.path.join(self.working_directory, '.file-cache') - if self.executables_directory is None: - self.executables_directory = '/tmp/devlib-target/bin' def _get_model_name(section): @@ -3156,6 +3180,7 @@ def __init__(self, package_data_directory="/data/data", is_container=False, max_async=50, + tmp_directory=None, ): self.supports_android = None @@ -3184,7 +3209,9 @@ def __init__(self, shell_prompt=shell_prompt, conn_cls=SshConnection, is_container=is_container, - max_async=max_async) + max_async=max_async, + tmp_directory=tmp_directory, + ) # We can't determine if the target supports android until connected to the linux host so # create unconditionally. @@ -3246,6 +3273,3 @@ async def connect(self, timeout=30, check_boot_completed=True, max_async=None): def _resolve_paths(self): if self.working_directory is None: self.working_directory = '/mnt/stateful_partition/devlib-target' - self._file_transfer_cache = self.path.join(self.working_directory, '.file-cache') - if self.executables_directory is None: - self.executables_directory = self.path.join(self.working_directory, 'bin') From 709c89cfdf9104920abee3e7a78123e307cc9223 Mon Sep 17 00:00:00 2001 From: Douglas Raillard Date: Wed, 20 Nov 2024 12:53:12 +0000 Subject: [PATCH 4/7] target: Make Target.make_temp() use Target.tmp_directory Also avoid a "None" prefix when no prefix is asked for, and set None as the default prefix value. Remove the "devlib-test" default value as make_temp() has nothing to do with tests. --- devlib/target.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/devlib/target.py b/devlib/target.py index f980ea31e..16e6a74a1 100644 --- a/devlib/target.py +++ b/devlib/target.py @@ -1191,7 +1191,7 @@ async def write_value(self, path, value, verify=True, as_root=True): raise @asyn.asynccontextmanager - async def make_temp(self, is_directory=True, directory='', prefix='devlib-test'): + async def make_temp(self, is_directory=True, directory=None, prefix=None): """ Creates temporary file/folder on target and deletes it once it's done. @@ -1209,10 +1209,11 @@ async def make_temp(self, is_directory=True, directory='', prefix='devlib-test') :rtype: str """ - directory = directory or self.working_directory + directory = directory or self.tmp_directory + prefix = f'{prefix}-' if prefix else '' temp_obj = None try: - cmd = f'mktemp -p {quote(directory)} {quote(prefix)}-XXXXXX' + cmd = f'mktemp -p {quote(directory)} {quote(prefix)}XXXXXX' if is_directory: cmd += ' -d' From d366031c3abd902788510115d043377db0f7dab3 Mon Sep 17 00:00:00 2001 From: Douglas Raillard Date: Wed, 20 Nov 2024 12:56:36 +0000 Subject: [PATCH 5/7] target: Make Target.tempfile() use Target.tmp_directory --- devlib/target.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/devlib/target.py b/devlib/target.py index 16e6a74a1..3898df5f5 100644 --- a/devlib/target.py +++ b/devlib/target.py @@ -1316,7 +1316,7 @@ async def tempfile(self, prefix='', suffix=''): uuid=uuid.uuid4().hex, suffix=suffix, ) - path = self.get_workpath(name) + path = self.path.join(self.tmp_directory, name) if (await self.file_exists.asyn(path)): raise FileExistsError('Path already exists on the target: {}'.format(path)) else: From 7274ec98e5a559b1ce5d128660931894fcc40930 Mon Sep 17 00:00:00 2001 From: Douglas Raillard Date: Wed, 20 Nov 2024 12:57:04 +0000 Subject: [PATCH 6/7] target: Align Target.tempfile() with Target.make_temp() Align the parameters between the 2 methods: * Use "None" as default value * Do not add suffix or prefix if not asked for. * Separate components with "-" instead of "_" --- devlib/target.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/devlib/target.py b/devlib/target.py index 3898df5f5..6df4158a2 100644 --- a/devlib/target.py +++ b/devlib/target.py @@ -1310,8 +1310,10 @@ def get_workpath(self, name): return self.path.join(self.working_directory, name) @asyn.asyncf - async def tempfile(self, prefix='', suffix=''): - name = '{prefix}_{uuid}_{suffix}'.format( + async def tempfile(self, prefix=None, suffix=None): + prefix = f'{prefix}-' if prefix else '' + suffix = f'-{suffix}' if suffix else '' + name = '{prefix}{uuid}{suffix}'.format( prefix=prefix, uuid=uuid.uuid4().hex, suffix=suffix, From 982a59e3a1cb2d1ba8e650590cf1ff8127d85f8a Mon Sep 17 00:00:00 2001 From: Douglas Raillard Date: Wed, 12 Feb 2025 11:52:08 +0000 Subject: [PATCH 7/7] devlib: Replace Target.tempfile() by Target.make_temp() Replace as many uses of tempfile() by make_temp() as possible, as the latter provide more reliable resource control by way of a context manager. This also paves the way to having a single point in devlib where temporary files are created, simplifying maintenance. --- devlib/module/cgroups2.py | 95 +++++++++++++++++---------------------- devlib/target.py | 11 +++-- 2 files changed, 45 insertions(+), 61 deletions(-) diff --git a/devlib/module/cgroups2.py b/devlib/module/cgroups2.py index 83cbf3948..a632bbee6 100644 --- a/devlib/module/cgroups2.py +++ b/devlib/module/cgroups2.py @@ -335,35 +335,28 @@ def _mount_v2_controllers(target: LinuxTarget): :yield: The path to the root of the mounted V2 controller hierarchy. :rtype: str - - :raises TargetStableError: Occurs in the case where the root directory of the requested CGroup V2 Controller hierarchy + + :raises TargetStableError: Occurs in the case where the root directory of the requested CGroup V2 Controller hierarchy is unable to be created up on the target system. """ - path = target.tempfile() - - try: - target.makedirs(path, as_root=True) - except TargetStableCalledProcessError: - raise TargetStableError("Un-able to create the root directory of the requested CGroup V2 hierarchy") - - - try: - target.execute( - "{busybox} mount -t cgroup2 none {path}".format( - busybox=quote(target.busybox), path=quote(path) - ), - as_root=True, - ) - yield path - finally: - target.execute( - "{busybox} umount {path} && {busybox} rmdir -- {path}".format( - busybox=quote(target.busybox), - path=quote(path), - ), - as_root=True, - ) + with target.make_temp() as path: + try: + target.execute( + "{busybox} mount -t cgroup2 none {path}".format( + busybox=quote(target.busybox), path=quote(path) + ), + as_root=True, + ) + yield path + finally: + target.execute( + "{busybox} umount {path}".format( + busybox=quote(target.busybox), + path=quote(path), + ), + as_root=True, + ) @contextmanager @@ -379,8 +372,8 @@ def _mount_v1_controllers(target: LinuxTarget, controllers: Set[str]): :yield: A dictionary mapping CGroup controller names to the paths that they're currently mounted at. :rtype: Dict[str,str] - - :raises TargetStableError: Occurs in the case where the root directory of a requested CGroup V1 Controller hierarchy + + :raises TargetStableError: Occurs in the case where the root directory of a requested CGroup V1 Controller hierarchy is unable to be created up on the target system. """ @@ -388,33 +381,25 @@ def _mount_v1_controllers(target: LinuxTarget, controllers: Set[str]): # its mount path. @contextmanager def _mount_controller(controller): + with target.make_temp() as path: + try: + target.execute( + "{busybox} mount -t cgroup -o {controller} none {path}".format( + busybox=quote(target.busybox), + controller=quote(controller), + path=quote(path), + ), + ) - path = target.tempfile() - - try: - target.makedirs(path, as_root=True) - except TargetStableCalledProcessError as err: - raise TargetStableError("Un-able to create the root directory of the {controller} CGroup V1 hierarchy".format(controller = controller)) - - try: - target.execute( - "{busybox} mount -t cgroup -o {controller} none {path}".format( - busybox=quote(target.busybox), - controller=quote(controller), - path=quote(path), - ), - ) - - yield path - - finally: - target.execute( - "{busybox} umount {path} && {busybox} rmdir -- {path}".format( - busybox=quote(target.busybox), - path=quote(path), - ), - as_root=True, - ) + yield path + finally: + target.execute( + "{busybox} umount {path}".format( + busybox=quote(target.busybox), + path=quote(path), + ), + as_root=True, + ) with ExitStack() as stack: yield { @@ -569,7 +554,7 @@ def _add_process(self, pid: Union[str, int]): ) except TargetStableError: self._set_controller_attribute("cgroup", "procs", pid) - + else: if str(pid) not in member_processes: self._set_controller_attribute("cgroup", "procs", pid) diff --git a/devlib/target.py b/devlib/target.py index 6df4158a2..fee70e333 100644 --- a/devlib/target.py +++ b/devlib/target.py @@ -1876,12 +1876,11 @@ async def capture_screen(self, filepath): return try: - tmpfile = await self.tempfile.asyn() - cmd = 'DISPLAY=:0.0 scrot {} && {} date -u -Iseconds' - ts = (await self.execute.asyn(cmd.format(quote(tmpfile), quote(self.busybox)))).strip() - filepath = filepath.format(ts=ts) - await self.pull.asyn(tmpfile, filepath) - await self.remove.asyn(tmpfile) + async with self.make_temp(is_directory=False) as tmpfile: + cmd = 'DISPLAY=:0.0 scrot {} && {} date -u -Iseconds' + ts = (await self.execute.asyn(cmd.format(quote(tmpfile), quote(self.busybox)))).strip() + filepath = filepath.format(ts=ts) + await self.pull.asyn(tmpfile, filepath) except TargetStableError as e: if "Can't open X dispay." not in e.message: raise e