Merge c73f961c7c into a035eaa227

A new core_pattern specifier was added, %F, to provide a PIDFD
to the usermode helper process referring to the crashed process.
This removes all possible race conditions, ensuring only the
crashed process gets inspected by systemd-coredump.

.github/workflows/coverage.yml

@@ -25,7 +25,7 @@ jobs:
 
     steps:
       - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683
-      - uses: systemd/mkosi@dbb4020beee2cdf250f93a425794f1cf8b0fe693
+      - uses: systemd/mkosi@7e4ec15aee6b98300b2ee14265bc647a716a9f8a
 
       # Freeing up disk space with rm -rf can take multiple minutes. Since we don't need the extra free space
       # immediately, we remove the files in the background. However, we first move them to a different location
@@ -90,6 +90,7 @@ jobs:
           sudo mkosi sandbox -- \
             meson setup \
             --buildtype=debugoptimized \
+            -Dintegration-tests=true \
             build
 
       - name: Build image
@@ -119,8 +120,7 @@ jobs:
             meson test \
             -C build \
             --no-rebuild \
-            --setup=integration \
-            --suite=integration-tests \
+            --suite integration-tests \
             --print-errorlogs \
             --no-stdsplit \
             --num-processes "$(($(nproc) - 1))" \

.github/workflows/mkosi.yml

@@ -120,7 +120,7 @@ jobs:
 
     steps:
       - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683
-      - uses: systemd/mkosi@dbb4020beee2cdf250f93a425794f1cf8b0fe693
+      - uses: systemd/mkosi@7e4ec15aee6b98300b2ee14265bc647a716a9f8a
 
       # Freeing up disk space with rm -rf can take multiple minutes. Since we don't need the extra free space
       # immediately, we remove the files in the background. However, we first move them to a different location
@@ -197,6 +197,7 @@ jobs:
           sudo mkosi sandbox -- \
             meson setup \
             --buildtype=debugoptimized \
+            -Dintegration-tests=true \
             -Dbpf-framework=disabled \
             build
 
@@ -232,8 +233,7 @@ jobs:
             meson test \
             -C build \
             --no-rebuild \
-            --setup=integration \
-            --suite=integration-tests \
+            --suite integration-tests \
             --print-errorlogs \
             --no-stdsplit \
             --num-processes "$(($(nproc) - 1))" \

meson.build

@@ -13,12 +13,6 @@ project('systemd', 'c',
         meson_version : '>= 0.62.0',
 )
 
-add_test_setup(
-        'default',
-        exclude_suites : ['integration-tests'],
-        is_default : true,
-)
-
 project_major_version = meson.project_version().split('.')[0].split('~')[0]
 if meson.project_version().contains('.')
         project_minor_version = meson.project_version().split('.')[-1].split('~')[0]
@@ -345,6 +339,7 @@ meson_build_sh = find_program('tools/meson-build.sh')
 want_tests = get_option('tests')
 want_slow_tests = want_tests != 'false' and get_option('slow-tests')
 want_fuzz_tests = want_tests != 'false' and get_option('fuzz-tests')
+want_integration_tests = want_tests != 'false' and get_option('integration-tests')
 install_tests = want_tests != 'false' and get_option('install-tests')
 
 if add_languages('cpp', native : false, required : fuzzer_build)
@@ -2666,6 +2661,10 @@ endif
 #####################################################################
 
 mkosi = find_program('mkosi', required : false)
+if want_integration_tests and not mkosi.found()
+        error('Could not find mkosi which is required to run the integration tests')
+endif
+
 mkosi_depends = public_programs
 
 foreach executable : ['systemd-journal-remote', 'systemd-sbsign', 'systemd-keyutil']

meson_options.txt

@@ -509,7 +509,7 @@ option('install-tests', type : 'boolean', value : false,
        description : 'install test executables')
 option('log-message-verification', type : 'feature', deprecated : { 'true' : 'enabled', 'false' : 'disabled' },
        description : 'do fake printf() calls to verify format strings')
-option('integration-tests', type : 'boolean', value : false, deprecated : true,
+option('integration-tests', type : 'boolean', value : false,
        description : 'run the integration tests')
 
 option('ok-color', type : 'combo',

mkosi/mkosi.conf

@@ -1,7 +1,7 @@
 # SPDX-License-Identifier: LGPL-2.1-or-later
 
 [Config]
-MinimumVersion=commit:dbb4020beee2cdf250f93a425794f1cf8b0fe693
+MinimumVersion=commit:7e4ec15aee6b98300b2ee14265bc647a716a9f8a
 Dependencies=
         exitrd
         initrd
@@ -78,7 +78,8 @@ KernelCommandLine=
         oops=panic
         panic=-1
         softlockup_panic=1
-        panic_on_warn=1
+        # Disabled due to BTRFS issue, waiting for the fix to become available
+        panic_on_warn=0
         psi=1
         mitigations=off
 

src/coredump/coredump.c

@@ -102,11 +102,7 @@ enum {
         META_ARGV_TIMESTAMP,    /* %t: time of dump, expressed as seconds since the Epoch (we expand this to μs granularity) */
         META_ARGV_RLIMIT,       /* %c: core file size soft resource limit */
         META_ARGV_HOSTNAME,     /* %h: hostname */
-        _META_ARGV_REQUIRED_MAX,
-        /* This counter includes META_COMM but excludes META_ARGV_PIDFD, as it's forwarded to foreign
-         * coredump that only knows about the existing mandatory arguments */
-        _META_MANDATORY_MAX = _META_ARGV_REQUIRED_MAX,
-        /* The following argvs are only available with new kernels, so they are optional */
+        _META_ARGV_REQUIRED_MAX,/* The following argvs are only available with new kernels, so they are optional */
         META_ARGV_PIDFD = _META_ARGV_REQUIRED_MAX, /* %F: pidfd of the process, since v6.16 */
         _META_ARGV_MAX,
 
@@ -116,11 +112,12 @@ enum {
          * environment. */
 
         META_COMM = _META_ARGV_MAX,
+        _META_MANDATORY_MAX,
 
         /* The rest are similar to the previous ones except that we won't fail if one of
          * them is missing. */
 
-        META_EXE,
+        META_EXE = _META_MANDATORY_MAX,
         META_UNIT,
         META_PROC_AUXV,
         _META_MAX
@@ -1319,7 +1316,7 @@ static int gather_pid_metadata_from_argv(
                 Context *context,
                 int argc, char **argv) {
 
-        _cleanup_(pidref_done) PidRef local_pidref = PIDREF_NULL;
+        _cleanup_(pidref_done) PidRef kernel_pidref = PIDREF_NULL;
         int r, kernel_fd = -EBADF;
 
         assert(iovw);
@@ -1331,7 +1328,7 @@ static int gather_pid_metadata_from_argv(
         if (argc < _META_ARGV_REQUIRED_MAX)
                 return log_error_errno(SYNTHETIC_ERRNO(EINVAL),
                                        "Not enough arguments passed by the kernel (%i, expected %i).",
-                                       argc, _META_ARGV_REQUIRED_MAX);
+                                       argc, _META_ARGV_MAX);
 
         for (int i = 0; i < MIN(argc, _META_ARGV_MAX); i++) {
                 _cleanup_free_ char *buf = NULL;
@@ -1353,41 +1350,45 @@ static int gather_pid_metadata_from_argv(
                         /* Store this so that we can check whether the core will be forwarded to a container
                          * even when the kernel doesn't provide a PIDFD. Can be dropped once baseline is
                          * >= v6.16. */
-                        r = pidref_set_pidstr(&local_pidref, t);
+                        r = pidref_set_pidstr(&kernel_pidref, t);
                         if (r < 0)
                                 return log_error_errno(r, "Failed to initialize pidref from pid %s: %m", t);
                 }
 
                 if (i == META_ARGV_PIDFD) {
-                        /* if the current kernel doesn't support the %F specifier (which resolves to a
-                         * pidfd), but we included it in the core_pattern expression, we'll receive an empty
-                         * string here. Deal with that gracefully. */
-                        if (isempty(t))
-                                continue;
-
                         assert(!pidref_is_set(&context->pidref));
-                        assert(kernel_fd < 0);
 
-                        kernel_fd = parse_fd(t);
-                        if (kernel_fd < 0)
-                                return log_error_errno(kernel_fd, "Failed to parse pidfd \"%s\": %m", t);
+                        if (!isempty(t)) {
+                                assert(kernel_fd < 0);
 
-                        r = pidref_set_pidfd(&context->pidref, kernel_fd);
-                        if (r < 0)
-                                return log_error_errno(r, "Failed to initialize pidref from pidfd %d: %m", kernel_fd);
+                                kernel_fd = parse_fd(t);
+                                if (kernel_fd < 0)
+                                        return log_error_errno(kernel_fd,
+                                                               "Failed to parse pidfd \"%s\": %m",
+                                                               t);
+
+                                r = pidref_set_pidfd(&context->pidref, kernel_fd);
+                                if (r < 0)
+                                        return log_error_errno(r,
+                                                               "Failed to initialize pidref from pidfd %d: %m",
+                                                               kernel_fd);
+                        } else
+                                /* If the kernel didn't give us a PIDFD, then use the one derived from the
+                                 * PID immediately, given we have it */
+                                context->pidref = TAKE_PIDREF(kernel_pidref);
 
                         /* If there are containers involved with different versions of the code they might
-                         * not be using pidfds, so it would be wrong to set the metadata, skip it. */
+                         * not be using pidfds, so it would be wrong to set the boolean, skip it. */
                         r = pidref_in_same_namespace(/* pid1 = */ NULL, &context->pidref, NAMESPACE_PID);
                         if (r < 0)
                                 log_debug_errno(r, "Failed to check pidns of crashing process, ignoring: %m");
-                        if (r <= 0)
+                        if (r == 0)
                                 continue;
 
                         /* We don't print the FD number in the journal as it's meaningless, but we still
                          * record that the parsing was done with a kernel-provided fd as it means it's safe
                          * from races, which is valuable information to provide in the journal record. */
-                        t = "1";
+                        t = one_zero(kernel_fd >= 0);
                 }
 
                 r = iovw_put_string_field(iovw, meta_field_names[i], t);
@@ -1401,11 +1402,6 @@ static int gather_pid_metadata_from_argv(
         if (r < 0)
                 return r;
 
-        /* If the kernel didn't give us a PIDFD, then use the one derived from the
-         * PID immediately, given we have it. */
-        if (!pidref_is_set(&context->pidref))
-                context->pidref = TAKE_PIDREF(local_pidref);
-
         /* Close the kernel-provided FD as the last thing after everything else succeeded */
         kernel_fd = safe_close(kernel_fd);
 

test/integration-tests/README.md

@@ -38,14 +38,14 @@ directory (`OutputDirectory=`) to point to the other directory using `mkosi/mkos
 After the image has been built, the integration tests can be run with:
 
 ```shell
-$ mkosi -f sandbox -- meson test -C build --setup=integration --suite integration-tests --num-processes "$(($(nproc) / 4))"
+$ env SYSTEMD_INTEGRATION_TESTS=1 mkosi -f sandbox -- meson test -C build --suite integration-tests --num-processes "$(($(nproc) / 4))"
 ```
 
 As usual, specific tests can be run in meson by appending the name of the test
 which is usually the name of the directory e.g.
 
 ```shell
-$ mkosi -f sandbox -- meson test -C build --setup=integration -v TEST-01-BASIC
+$ env SYSTEMD_INTEGRATION_TESTS=1 mkosi -f sandbox -- meson test -C build -v TEST-01-BASIC
 ```
 
 See `mkosi -f sandbox -- meson introspect build --tests` for a list of tests.
@@ -55,7 +55,7 @@ To interactively debug a failing integration test, the `--interactive` option
 newer:
 
 ```shell
-$ mkosi -f sandbox -- meson test -C build --setup=integration -i TEST-01-BASIC
+$ env SYSTEMD_INTEGRATION_TESTS=1 mkosi -f sandbox -- meson test -C build -i TEST-01-BASIC
 ```
 
 Due to limitations in meson, the integration tests do not yet depend on the
@@ -64,7 +64,7 @@ running the integration tests. To rebuild the image and rerun a test, the
 following command can be used:
 
 ```shell
-$ mkosi -f sandbox -- meson compile -C build mkosi && mkosi -f sandbox -- meson test -C build --setup=integration -v TEST-01-BASIC
+$ mkosi -f sandbox -- meson compile -C build mkosi && env SYSTEMD_INTEGRATION_TESTS=1 mkosi -f sandbox -- meson test -C build -v TEST-01-BASIC
 ```
 
 The integration tests use the same mkosi configuration that's used when you run
@@ -78,7 +78,7 @@ To iterate on an integration test, let's first get a shell in the integration te
 the following:
 
 ```shell
-$ mkosi -f sandbox -- meson compile -C build mkosi && mkosi -f sandbox -- meson test -C build --setup=shell -i TEST-01-BASIC
+$ mkosi -f sandbox -- meson compile -C build mkosi && env SYSTEMD_INTEGRATION_TESTS=1 TEST_SHELL=1 mkosi -f sandbox -- meson test -C build -i TEST-01-BASIC
 ```
 
 This will get us a shell in the integration test environment after booting the machine without running the
@@ -107,7 +107,7 @@ re-running the test will first install the new packages we just built, make a ne
 the test again. You can keep running the loop of `mkosi -R`, `systemctl soft-reboot` and
 `systemctl start ...` until the changes to the integration test are working.
 
-If you're debugging a failing integration test (running `meson test --interactive`),
+If you're debugging a failing integration test (running `meson test --interactive` without `TEST_SHELL`),
 there's no need to run `systemctl start ...`, running `systemctl soft-reboot` on its own is sufficient to
 rerun the test.
 
@@ -120,6 +120,10 @@ rerun the test.
 `TEST_NO_KVM=1`: Disable qemu KVM auto-detection (may be necessary when you're
 trying to run the *vanilla* qemu and have both qemu and qemu-kvm installed)
 
+`TEST_SHELL=1`: Configure the machine to be more *user-friendly* for
+interactive debugging (e.g. by setting a usable default terminal, suppressing
+the shutdown after the test, etc.).
+
 `TEST_MATCH_SUBTEST=subtest`:  If the test makes use of `run_subtests` use this
 variable to provide a POSIX extended regex to run only subtests matching the
 expression.

test/integration-tests/integration-test-wrapper.py

@@ -361,7 +361,7 @@ def statfs(path: Path) -> str:
 
 def main() -> None:
     parser = argparse.ArgumentParser(description=__doc__)
-    parser.add_argument('--mkosi', default=None)
+    parser.add_argument('--mkosi', required=True)
     parser.add_argument('--meson-source-dir', required=True, type=Path)
     parser.add_argument('--meson-build-dir', required=True, type=Path)
     parser.add_argument('--name', required=True)
@@ -379,12 +379,6 @@ def main() -> None:
     parser.add_argument('mkosi_args', nargs='*')
     args = parser.parse_args()
 
-    if not args.mkosi:
-        args.mkosi = shutil.which('mkosi')
-        if not args.mkosi:
-            print('Could not find mkosi which is required to run the integration tests', file=sys.stderr)
-            sys.exit(1)
-
     # The meson source directory can either be the top-level repository directory or the
     # test/integration-tests/standalone subdirectory in the repository directory. The mkosi configuration
     # will always be a parent directory of one of these directories and at most 4 levels upwards, so don't
@@ -401,6 +395,13 @@ def main() -> None:
         )
         exit(1)
 
+    if not bool(int(os.getenv('SYSTEMD_INTEGRATION_TESTS', '0'))):
+        print(
+            f'SYSTEMD_INTEGRATION_TESTS=1 not found in environment, skipping {args.name}',
+            file=sys.stderr,
+        )
+        exit(77)
+
     if args.slow and not bool(int(os.getenv('SYSTEMD_SLOW_TESTS', '0'))):
         print(
             f'SYSTEMD_SLOW_TESTS=1 not found in environment, skipping {args.name}',

test/integration-tests/meson.build

@@ -1,9 +1,5 @@
 # SPDX-License-Identifier: LGPL-2.1-or-later
 
-# We'd give these more descriptive names but only alphanumeric characters are allowed.
-add_test_setup('integration')
-add_test_setup('shell', env : {'TEST_SHELL' : '1'})
-
 integration_test_wrapper = find_program('integration-test-wrapper.py')
 integration_tests = []
 integration_test_template = {
@@ -133,11 +129,11 @@ foreach integration_test : integration_tests
                 integration_test_args += ['--skip']
         endif
 
-        if mkosi.found()
-                integration_test_args += ['--mkosi', mkosi.full_path()]
+        if not mkosi.found()
+                continue
         endif
 
-        integration_test_args += ['--']
+        integration_test_args += ['--mkosi', mkosi.full_path(), '--']
 
         if integration_test['cmdline'].length() > 0
                 integration_test_args += [
@@ -155,12 +151,19 @@ foreach integration_test : integration_tests
 
         integration_test_args += integration_test['mkosi-args']
 
+        integration_test_env = {}
+
+        if want_integration_tests
+                integration_test_env += {'SYSTEMD_INTEGRATION_TESTS': '1'}
+        endif
+
         # We don't explicitly depend on the "mkosi" target because that means the image is rebuilt on every
         # "ninja -C build". Instead, the mkosi target has to be rebuilt manually before running the
         # integration tests with mkosi.
         test(
                 integration_test['name'],
                 integration_test_wrapper,
+                env : integration_test_env,
                 args : integration_test_args,
                 timeout : integration_test['timeout'],
                 priority : integration_test['priority'],

test/integration-tests/standalone/meson.build

@@ -16,6 +16,7 @@ project('systemd-testsuite',
 
 fs = import('fs')
 mkosi = find_program('mkosi', required : true)
+want_integration_tests = true
 
 # meson refuses .. in subdir() so we use a symlink to trick it into accepting it anyway.
 subdir('integration-tests')