Skip to content

Add an object encapsulating the execution environment for helper subprocesses

Simon McVittie requested to merge wip/smcv/subproc-runner into main

Enabler for !625 (merged), !628 (merged), !626 (merged). Best viewed a commit at a time.

The first two commits (or any prefix, really) could be separated into yet another MR if preferred.

  • pv-runtime, tests: Never pass NULL envp to _srt_recursive_list_content

    environ can in principle be NULL.

  • Pass environment as const char * const * if we will not modify it

    This makes us somewhat more const-correct, reducing the scope for accidents.

  • subprocess: Factor out an object for the execution environment

    This will let us bundle together more of the subprocess handling for running helpers. For now, it is just a container for the environment, helpers path and test flags.

    This is an immutable value-object which does not change after it has been constructed, meaning that we can share it between threads if that becomes useful in future.

    No functional change intended.

  • runtime: Don't unnecessarily copy the environment

    my_environ had the same content as custom_environ, so we can use our new const-correct wrapper _srt_environ_getenv without any casts or copying.

  • graphics: Pass execution environment to _srt_check_library_presence()

    Previously we ignored the parameter and used default values here.

  • library: Pass SrtSubprocessRunner to _srt_inspect_library

    If we make _srt_inspect_library responsible for filtering out the gameoverlayrenderer.so from LD_PRELOAD itself, then we can give it the unmodified execution environment as its argument.

  • subprocess: Move _srt_get_helper to _srt_subprocess_runner_get_helper

  • subprocess: Add and use _srt_subprocess_runner_spawn_sync()

    This encapsulates g_spawn_sync() with a useful set of flags and behaviours, and will let us move the responsibility for handling features like timeouts between get_helper() and spawn_sync(). To make that work, try harder to pass the same flags to both get_helper() and spawn_sync().

    For architecture, display, graphics-drivers and libdl, no behaviour change is intended.

    display and graphics-drivers should probably not be using KEEP_GAMEOVERLAYRENDERER (there's no real reason to do so) but for now, preserve existing behaviour.

    For library, we were previously passing G_SPAWN_SEARCH_PATH to g_spawn_sync(), but it would have had no practical effect, because we're using the absolute path to a helper executable. Drop that flag.

    For graphics, we were previously searching the PATH for everything except check-gl. Consistently search the PATH in all cases.

  • subprocess: Remove SRT_HELPER_FLAGS_TIME_OUT_SOONER

    Now that SrtSubprocessRunner knows both the SrtTestFlags and SrtHelperFlags, it can always set the timeout appropriately.

  • subprocess: Always filter out gameoverlayrenderer.so

    The display and graphics-drivers checks historically didn't, but that seems more like accident than design. We can add a way to avoid this filtering if we find that we need it in future.

  • architecture: Collect and log subprocess stdout/stderr

    During refactoring I noticed that these would previously have inherited the parent process's stdout and stderr file descriptors, potentially interfering with machine-readable output on stdout. Clean that up.

  • locale: Log diagnostic output instead of letting it go to stderr

    This ensures that if our log output has been redirected to the systemd Journal, the helper's diagnostic output will follow.

  • graphics: Log captured output at debug level

    During refactoring I noticed that this function was capturing stdout and stderr, but didn't actually do anything with them. Let's have these in a debug log.

Edited by Simon McVittie

Merge request reports