From: Daan De Meyer Date: Tue, 11 Feb 2025 09:29:37 +0000 (+0100) Subject: Drop foreground process logic X-Git-Tag: v26~398^2~4 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=b3be819fe77ac8a07ad0609ab258a9b830abd04e;p=thirdparty%2Fmkosi.git Drop foreground process logic Let's stop messing around with process groups in favor of handling terminal signals properly ourselves. We had to use process groups in the past because we still used subprocess.run() which meant that by the time we had a chance to handle KeyboardInterrupt(), the subprocess would have already been SIGKILLed by subprocess.run(). Now that we don't use subprocess.run() anymore, we can catch KeyboardInterrupt() at the right time and forward it to the child process. For the mkosi process itself, we have to modify the signal handlers slightly to make sure we only raise the KeyboardInterrupt() exception once, as when we're running a forked subprocess, both the parent process and the forked subprocess will receive SIGINT if Ctrl+C is entered on the terminal, and the parent process will forward the SIGINT to the child process, which is already handling its own SIGINT, causing KeyboardInterrupt() to get raised twice if we don't add an extra check to the signal handler to make sure it only gets raised once. --- diff --git a/mkosi/__init__.py b/mkosi/__init__.py index 75d47c76e..d710d29c8 100644 --- a/mkosi/__init__.py +++ b/mkosi/__init__.py @@ -4328,7 +4328,6 @@ def run_serve(args: Args, config: Config) -> None: relaxed=True, options=["--chdir", config.output_dir_or_cwd()], ), - foreground=not want_storagetm, ) ) @@ -4344,7 +4343,6 @@ def run_serve(args: Args, config: Config) -> None: options=["--chdir", config.output_dir_or_cwd()], setup=become_root_cmd(), ), - foreground=True, ) ) diff --git a/mkosi/__main__.py b/mkosi/__main__.py index c91550483..d94ce3ea9 100644 --- a/mkosi/__main__.py +++ b/mkosi/__main__.py @@ -14,13 +14,21 @@ from mkosi.log import log_setup from mkosi.run import find_binary, run, uncaught_exception_handler from mkosi.util import resource_path +INTERRUPTED = False + def onsignal(signal: int, frame: Optional[FrameType]) -> None: + global INTERRUPTED + if INTERRUPTED: + return + + INTERRUPTED = True raise KeyboardInterrupt() @uncaught_exception_handler() def main() -> None: + signal.signal(signal.SIGINT, onsignal) signal.signal(signal.SIGTERM, onsignal) signal.signal(signal.SIGHUP, onsignal) diff --git a/mkosi/qemu.py b/mkosi/qemu.py index d2cf23898..18b681157 100644 --- a/mkosi/qemu.py +++ b/mkosi/qemu.py @@ -566,7 +566,6 @@ def start_journal_remote(config: Config, sockfd: int) -> Iterator[None]: ), user=user if not scope else None, group=group if not scope else None, - foreground=False, ) as proc: # fmt: skip yield proc.terminate() @@ -991,7 +990,6 @@ def machine1_is_available(config: Config) -> bool: services = json.loads( run( ["busctl", "list", "--json=pretty"], - foreground=False, env=os.environ | config.environment, sandbox=config.sandbox(relaxed=True), stdout=subprocess.PIPE, @@ -1047,7 +1045,6 @@ def register_machine(config: Config, pid: int, fname: Path, cid: Optional[int]) } ), ], - foreground=False, env=os.environ | config.environment, sandbox=config.sandbox(relaxed=True), # Make sure varlinkctl doesn't write to stdout which messes up the terminal. @@ -1072,7 +1069,6 @@ def register_machine(config: Config, pid: int, fname: Path, cid: Optional[int]) str(pid), fname if fname.is_dir() else "", ], # fmt: skip - foreground=False, env=os.environ | config.environment, sandbox=config.sandbox(relaxed=True), # systemd-machined might not be installed so let's ignore any failures unless running in debug @@ -1549,7 +1545,6 @@ def run_qemu(args: Args, config: Config) -> None: stderr=stderr, pass_fds=qemu_device_fds.values(), env=os.environ | config.environment, - foreground=True, sandbox=config.sandbox( network=True, devices=True, diff --git a/mkosi/run.py b/mkosi/run.py index 441125679..dc62e22bc 100644 --- a/mkosi/run.py +++ b/mkosi/run.py @@ -38,24 +38,6 @@ else: Popen = subprocess.Popen -def make_foreground_process(*, new_process_group: bool = True) -> None: - """ - If we're connected to a terminal, put the process in a new process group and make that the foreground - process group so that only this process receives SIGINT. - """ - STDERR_FILENO = 2 - if os.isatty(STDERR_FILENO): - if new_process_group: - os.setpgrp() - old = signal.signal(signal.SIGTTOU, signal.SIG_IGN) - try: - os.tcsetpgrp(STDERR_FILENO, os.getpgrp()) - except OSError as e: - if e.errno != errno.ENOTTY: - raise e - signal.signal(signal.SIGTTOU, old) - - def ensure_exc_info() -> tuple[type[BaseException], BaseException, TracebackType]: exctype, exc, tb = sys.exc_info() assert exctype @@ -108,16 +90,16 @@ def fork_and_wait(target: Callable[..., None], *args: Any, **kwargs: Any) -> Non pid = os.fork() if pid == 0: with uncaught_exception_handler(exit=os._exit): - make_foreground_process() target(*args, **kwargs) try: _, status = os.waitpid(pid, 0) + except KeyboardInterrupt: + os.kill(pid, signal.SIGINT) + _, status = os.waitpid(pid, 0) except BaseException: os.kill(pid, signal.SIGTERM) _, status = os.waitpid(pid, 0) - finally: - make_foreground_process(new_process_group=False) rc = os.waitstatus_to_exitcode(status) @@ -162,7 +144,6 @@ def run( group: Optional[int] = None, env: Mapping[str, str] = {}, log: bool = True, - foreground: bool = True, success_exit_status: Sequence[int] = (0,), sandbox: AbstractContextManager[Sequence[PathString]] = contextlib.nullcontext([]), ) -> CompletedProcess: @@ -180,7 +161,6 @@ def run( group=group, env=env, log=log, - foreground=foreground, success_exit_status=success_exit_status, sandbox=sandbox, ) as process: @@ -197,12 +177,9 @@ def fd_move_above(fd: int, above: int) -> int: def preexec( *, - foreground: bool, preexec_fn: Optional[Callable[[], None]], pass_fds: Collection[int], ) -> None: - if foreground: - make_foreground_process() if preexec_fn: preexec_fn() @@ -245,7 +222,6 @@ def spawn( pass_fds: Collection[int] = (), env: Mapping[str, str] = {}, log: bool = True, - foreground: bool = False, preexec_fn: Optional[Callable[[], None]] = None, success_exit_status: Sequence[int] = (0,), sandbox: AbstractContextManager[Sequence[PathString]] = contextlib.nullcontext([]), @@ -318,12 +294,7 @@ def spawn( # transformation in preexec(). pass_fds=[SD_LISTEN_FDS_START + i for i in range(len(pass_fds))], env=env, - preexec_fn=functools.partial( - preexec, - foreground=foreground, - preexec_fn=preexec_fn, - pass_fds=pass_fds, - ), + preexec_fn=functools.partial(preexec, preexec_fn=preexec_fn, pass_fds=pass_fds), ) as proc: if pwfd is not None: os.close(pwfd) @@ -340,6 +311,9 @@ def spawn( # exception later on so it's not a problem that we don't yield at all. if not failed(): yield proc + except KeyboardInterrupt: + proc.send_signal(signal.SIGINT) + raise except BaseException: proc.terminate() raise @@ -358,19 +332,11 @@ def spawn( user=user, group=group, env=env, - preexec_fn=functools.partial( - preexec, - foreground=True, - preexec_fn=preexec_fn, - pass_fds=tuple(), - ), + preexec_fn=functools.partial(preexec, preexec_fn=preexec_fn, pass_fds=tuple()), ) raise subprocess.CalledProcessError(returncode, cmdline) except FileNotFoundError as e: die(f"{e.filename} not found.") - finally: - if foreground: - make_foreground_process(new_process_group=False) def finalize_path(