create dirs with systemd-tmpfiles instead of RuntimeDirectory

[Ali Erdinc Koroglu (aekoroglu)]

RuntimeDirectory= deletes /run/urm on every stop or restart, losing the journald backup needed for crash recovery. It also ignores URM_RUNSTATEDIR and doesn't create URM_CACHE_DIR.

Run systemd-tmpfiles with our urm.conf before starting instead. It creates both directories from the CMake paths and never removes them.

[qualcomm-ai-code-review-assistant]

Qualcomm AI Review

Click to expand Code Review

Reviewed Commits: 5fa7939

• 5fa7939: create dirs with systemd-tmpfiles instead of RuntimeDirectory

RuntimeDirectory= deletes /run/urm on every stop or restart, losing the
journald backup needed for crash recovery. It also ignores
URM_RUNSTATEDIR and doesn't create URM_CACHE_DIR.

Run systemd-tmpfiles with our urm.conf before starting instead. It
creates both directories from the CMake paths and never removes them.

Signed-off-by: Ali Erdinc Koroglu ali.koroglu@oss.qualcomm.com

PR Overview

This pull request modifies the systemd service configuration for the URM (User Resource Manager) service. The changes replace the RuntimeDirectory and RuntimeDirectoryMode directives with a systemd-tmpfiles approach for managing runtime directories.

Files Changed Summary

File	Lines Changed	Issues Found	Highest Severity
urm.service.in	5 lines modified	2	High

Key Changes

• Removed: RuntimeDirectory=urm and RuntimeDirectoryMode=0755 directives
• Added: ExecStartPre=+systemd-tmpfiles --create urm.conf to handle runtime directory creation
• Reordered: Service execution directives

Critical Issues Identified

1. [High Severity] Missing urm.conf tmpfiles configuration file - The service now depends on a tmpfiles configuration that must exist

Summary

The migration from RuntimeDirectory to systemd-tmpfiles is a valid approach but requires careful validation to ensure the urm.conf tmpfiles configuration exists and properly creates the necessary runtime directories with correct permissions before the service starts.

[Functionality] Missing tmpfiles configuration dependency - High Severity

The service configuration now relies on systemd-tmpfiles --create urm.conf to create runtime directories, but there's no guarantee that the urm.conf file exists in the systemd tmpfiles directory (typically /usr/lib/tmpfiles.d/ or /etc/tmpfiles.d/). If this file is missing, the service will fail to start properly.

Issue Details:

• The ExecStartPre=+systemd-tmpfiles --create urm.conf command references urm.conf which must exist in the tmpfiles.d directory
• The + prefix means the command runs with elevated privileges, but doesn't prevent failure if the file is missing
• The previous RuntimeDirectory=urm approach was more self-contained and didn't require external configuration files

Recommendation:
Ensure that:

1. The urm.conf tmpfiles configuration file is created and installed as part of the build/installation process
2. The tmpfiles configuration properly recreates the same directory structure with mode 0755 (or appropriate permissions)
3. Consider adding a comment in the service file indicating the dependency on the tmpfiles configuration

Expected urm.conf content should be similar to:

d /run/urm 0755 root root -

Fixed Code Snippet:

[Service]
Restart=on-failure
# Requires urm.conf in /usr/lib/tmpfiles.d/ to create /run/urm directory
ExecStartPre=+systemd-tmpfiles --create urm.conf
ExecStartPre=-/usr/libexec/urm/initscripts/post_boot/post_boot.sh
ExecStart=@CMAKE_INSTALL_FULL_SBINDIR@/urm

[Error Handling] Insufficient error handling for tmpfiles creation - Medium Severity

The ExecStartPre=+systemd-tmpfiles --create urm.conf command uses the + prefix (elevated privileges) but doesn't use the - prefix (ignore failure). If the tmpfiles command fails for any reason (missing config file, permission issues, disk full, etc.), the service will fail to start, but the error handling could be more explicit.

Issue Details:

• The + prefix grants elevated privileges but doesn't affect error handling
• Unlike the second ExecStartPre which uses - to ignore failures, this command will cause service startup to fail if it encounters any error
• This is stricter than the previous RuntimeDirectory approach, which would create the directory or fail more gracefully

Consideration:
While strict error handling is generally good, you should verify whether:

1. The tmpfiles creation should be mandatory (current behavior - service fails if it fails)
2. Or if it should be optional with - prefix (service continues even if it fails)

The current implementation treats it as mandatory, which is likely correct for a runtime directory requirement. However, ensure proper logging and error messages are in place for troubleshooting.

Current behavior (strict - recommended for critical directories):

ExecStartPre=+systemd-tmpfiles --create urm.conf

Alternative (permissive - only if directory creation is optional):

ExecStartPre=-+systemd-tmpfiles --create urm.conf

---

⚠️ This review was generated using AI assistance. Please verify all suggestions before applying.

[qualcomm-ai-code-review-assistant]

Qualcomm AI Review

[Kartik Nema (kartnema)]

Thanks for this patch, let me verify the patch through package installation as well as building and starting the service locally.

[Ali Erdinc Koroglu (aekoroglu)]

Here is the fedora rawhide package, it may help: https://copr.fedorainfracloud.org/coprs/aekoroglu/fedora/build/11034774/

[Kartik Nema (kartnema)]

Verified, changes are working correctly. Approving