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 75d47c76e5554993504aa319ba086a08048f064b..d710d29c88f90aba219e66f4d5d4721c18f84151 100644 (file)
--- 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 c915504838904ca07c926be2b0aa485fe21e4d58..d94ce3ea93abfe0f89b24ce1e40fe4bc312c5607 100644 (file)
--- 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 d2cf23898bcab1826875d2e83f65630b85e65487..18b681157878fb8e834e338f69eb6a0b12c809e2 100644 (file)
--- 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 441125679ee9275f17fc2d382b471ce28c232a28..dc62e22bc515efe7db038a77552e4de77c186ce4 100644 (file)
--- 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(