Create default.service - #2
Conversation
|
@copilot review this pull request. |
|
@fzahir786 I've opened a new pull request, #3, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
|
||
| [Service] | ||
| Type=oneshot | ||
| ExecStart=/bin/sh -c 'if [ -f /lib/rdk/bootversion-loader.sh ]; then exec /lib/rdk/bootversion-loader.sh; else exit 0; fi' |
There was a problem hiding this comment.
The ExecStart directive uses shell wrapper scripts instead of direct commands. According to systemd best practices, systemd should manage service lifecycle directly rather than using wrapper scripts. Shell scripts add complexity, failure points, and increase boot time. Additionally, systemd cannot track processes properly through scripts, making debugging and maintenance harder.
Consider refactoring to use direct commands or ConditionPathExists to handle file existence checks, similar to how other services in this repository use ConditionPathExists (e.g., wpeframework-analytics.service, wpeframework-appmanager.service).
| [Service] | ||
| Type=oneshot | ||
| ExecStart=/bin/sh -c 'if [ -f /lib/rdk/bootversion-loader.sh ]; then exec /lib/rdk/bootversion-loader.sh; else exit 0; fi' | ||
| ExecStartPost=/bin/sh -c 'if [ -f /lib/rdk/boot_FSR.sh ]; then exec /lib/rdk/boot_FSR.sh; else exit 0; fi' |
There was a problem hiding this comment.
The ExecStartPost directive uses shell wrapper scripts instead of direct commands. According to systemd best practices, systemd should manage service lifecycle directly rather than using wrapper scripts. Shell scripts add complexity, failure points, and increase boot time. Additionally, systemd cannot track processes properly through scripts, making debugging and maintenance harder.
Consider refactoring to use direct commands or ConditionPathExists to handle file existence checks, similar to how other services in this repository use ConditionPathExists (e.g., wpeframework-analytics.service, wpeframework-appmanager.service).
| Type=oneshot | ||
| ExecStart=/bin/sh -c 'if [ -f /lib/rdk/bootversion-loader.sh ]; then exec /lib/rdk/bootversion-loader.sh; else exit 0; fi' | ||
| ExecStartPost=/bin/sh -c 'if [ -f /lib/rdk/boot_FSR.sh ]; then exec /lib/rdk/boot_FSR.sh; else exit 0; fi' | ||
| RemainAfterExit=yes |
There was a problem hiding this comment.
The service is missing TimeoutStartSec and TimeoutStopSec directives. These timeouts define how long systemd waits for service start/stop operations. Without explicit timeouts, systemd uses the default timeout value of 90 seconds, which may not be appropriate for this service.
According to systemd best practices, you should always specify both TimeoutStartSec (recommended 30 seconds or less) and TimeoutStopSec (recommended 10 seconds or less). This prevents hung services from blocking boot or shutdown and provides clear expectations for service lifecycle timing. If a timeout value greater than 90 seconds is needed, proper justification should be provided.
| @@ -0,0 +1,12 @@ | |||
| [Unit] | |||
| Description=Boot version loader for RDK | |||
| After=ecfs-init.service | |||
There was a problem hiding this comment.
The service uses only the After= directive but lacks Requires= or Wants= for dependency management. While After= controls startup ordering, it doesn't establish a true dependency relationship with ecfs-init.service.
According to systemd best practices:
- Use Requires= when the service cannot start if the dependency fails (strict dependency)
- Use Wants= when the service can start even if the dependency is missing (soft dependency), paired with After= for ordering
Without Requires= or Wants=, the service may start even if ecfs-init.service has failed, which could lead to unexpected behavior. Consider adding either Requires=ecfs-init.service or Wants=ecfs-init.service depending on whether ecfs-init.service is a strict or optional dependency.
There was a problem hiding this comment.
@copilot add a new commit to resolve the issue (#2 (comment))
|
@fzahir786 I've opened a new pull request, #4, to work on those changes. Once the pull request is ready, I'll request review from you. |
No description provided.