Repository navigation
linux: add support Dinit, Runit, S6, SysVInit meters - #2061
GermanAizek wants to merge 4 commits into
Conversation
|
This continue work in this PR: #1887 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Priority: ⬇️ Low Change: Feature Merge Risk: 🟡 Moderate · up to On affected systems, SysV status failures can accumulate unreaped child processes, while Runit and S6 meters can delay refreshes as service counts grow. Some SysV and Dinit status displays can also be inaccurate, so these behaviors should be addressed before merging. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Four init paths now speak in view, Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8ecded6e-9af2-4031-b6c7-b826b126ea14
📒 Files selected for processing (10)
Makefile.amlinux/DinitMeter.clinux/DinitMeter.hlinux/Platform.clinux/RunitMeter.clinux/RunitMeter.hlinux/S6Meter.clinux/S6Meter.hlinux/SysVInitMeter.clinux/SysVInitMeter.h
| static void DinitMeter_updateValues(Meter* this) { | ||
| ctx_dinit.servicesStarted = INVALID_VALUE; | ||
| ctx_dinit.servicesStopped = INVALID_VALUE; | ||
| ctx_dinit.servicesFailed = INVALID_VALUE; | ||
|
|
||
| if (Settings_isReadonly()) | ||
| goto done; | ||
|
|
||
| /* Check dinit is present */ | ||
| if (access("/sbin/dinit", F_OK) != 0 && | ||
| access("/usr/sbin/dinit", F_OK) != 0 && | ||
| access("/usr/bin/dinit", F_OK) != 0) | ||
| goto done; | ||
|
|
||
| int fdpair[2] = {-1, -1}; | ||
| if (pipe(fdpair) < 0) | ||
| goto done; | ||
|
|
||
| pid_t child = fork(); | ||
| if (child < 0) { | ||
| close(fdpair[1]); | ||
| close(fdpair[0]); | ||
| goto done; | ||
| } | ||
|
|
||
| if (child == 0) { | ||
| close(fdpair[0]); | ||
| dup2(fdpair[1], STDOUT_FILENO); | ||
| close(fdpair[1]); | ||
| int fdnull = open("/dev/null", O_WRONLY); | ||
| if (fdnull < 0) | ||
| _exit(1); | ||
| dup2(fdnull, STDERR_FILENO); | ||
| close(fdnull); | ||
| execlp("dinitctl", "dinitctl", "list", (char*)NULL); | ||
| _exit(127); | ||
| } | ||
|
|
||
| close(fdpair[1]); | ||
|
|
||
| FILE* commandOutput = fdopen(fdpair[0], "r"); | ||
| if (!commandOutput) { | ||
| close(fdpair[0]); | ||
| xWaitpid(child, NULL, 0, false); | ||
| goto done; | ||
| } | ||
|
|
||
| ctx_dinit.servicesStarted = 0; | ||
| ctx_dinit.servicesStopped = 0; | ||
| ctx_dinit.servicesFailed = 0; | ||
|
|
||
| /* | ||
| * Parse the human-readable dinitctl list output. | ||
| * Lines look like: | ||
| * [[+] ] servicename | ||
| * [[ ] ] servicename | ||
| * [[!] ] servicename <- failed | ||
| * | ||
| * Character at index 2 is the state indicator: | ||
| * '+' = started, ' ' = stopped, '-' = stopping/starting, | ||
| * '{' = starting, '}' = stopping, '!' = failed/error | ||
| */ | ||
| char lineBuffer[512]; | ||
| while (fgets(lineBuffer, sizeof(lineBuffer), commandOutput)) { | ||
| /* Skip lines that don't start with '[' */ | ||
| if (lineBuffer[0] != '[') | ||
| continue; | ||
|
|
||
| char stateChar = lineBuffer[2]; | ||
| if (stateChar == '+') { | ||
| ctx_dinit.servicesStarted++; | ||
| } else if (stateChar == '!') { | ||
| ctx_dinit.servicesFailed++; | ||
| ctx_dinit.servicesStopped++; | ||
| } else { | ||
| ctx_dinit.servicesStopped++; | ||
| } | ||
| } | ||
| fclose(commandOutput); | ||
| xWaitpid(child, NULL, 0, false); | ||
|
|
||
| done: | ||
| if (ctx_dinit.servicesStarted != INVALID_VALUE) { | ||
| xSnprintf(this->txtBuffer, sizeof(this->txtBuffer), | ||
| "%zu started, %zu failed", | ||
| ctx_dinit.servicesStarted, ctx_dinit.servicesFailed); | ||
| } else { | ||
| xSnprintf(this->txtBuffer, sizeof(this->txtBuffer), "N/A"); | ||
| } | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
A blocking fork/exec runs on every meter refresh.
updateValues forks, executes dinitctl, reads the full pipe, and blocks in xWaitpid on the UI thread. htop calls this at every refresh interval, so the UI stalls for the duration of the child process. linux/RunitMeter.c and linux/SysVInitMeter.c use the same pattern.
Recommendation: cache the result and re-run the child process on a longer interval, or read the dinit control socket or service state files directly instead of spawning a process. Compare with linux/SystemdMeter.c, which queries D-Bus without a fork.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 84-84: execlp/execvp/execvpe resolve the program name via ; if PATH is attacker-influenced this runs an unintended binary. Use execv/execve with an absolute path.
Context: execlp("dinitctl", "dinitctl", "list", (char*)NULL)
Note: [CWE-426] Untrusted Search Path.
(untrusted-search-path-c)
[warning] 133-133: Do not use sizeof(this) to get the number of bytes of the object in memory. It returns the size of the pointer, not the size of the object.
Context: sizeof(this->txtBuffer)
Note: [CWE-467]: Use of sizeof() on a Pointer Type
(sizeof-this-c)
[warning] 137-137: Do not use sizeof(this) to get the number of bytes of the object in memory. It returns the size of the pointer, not the size of the object.
Context: sizeof(this->txtBuffer)
Note: [CWE-467]: Use of sizeof() on a Pointer Type
(sizeof-this-c)
| char stateChar = lineBuffer[2]; | ||
| if (stateChar == '+') { | ||
| ctx_dinit.servicesStarted++; | ||
| } else if (stateChar == '!') { | ||
| ctx_dinit.servicesFailed++; | ||
| ctx_dinit.servicesStopped++; | ||
| } else { | ||
| ctx_dinit.servicesStopped++; | ||
| } | ||
| } | ||
| fclose(commandOutput); | ||
| xWaitpid(child, NULL, 0, false); | ||
|
|
||
| done: | ||
| if (ctx_dinit.servicesStarted != INVALID_VALUE) { | ||
| xSnprintf(this->txtBuffer, sizeof(this->txtBuffer), | ||
| "%zu started, %zu failed", | ||
| ctx_dinit.servicesStarted, ctx_dinit.servicesFailed); | ||
| } else { | ||
| xSnprintf(this->txtBuffer, sizeof(this->txtBuffer), "N/A"); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Failed services are counted twice, and the two output paths disagree.
Line 123 and line 124 increment both servicesFailed and servicesStopped for the same service. display() then prints "N started, M stopped, K failed", where the failed services are included in M. The sum exceeds the service total.
txtBuffer reports "started, failed" while display() reports started, stopped and failed. Both paths render the same meter; make them consistent.
Proposed fix
char stateChar = lineBuffer[2];
if (stateChar == '+') {
ctx_dinit.servicesStarted++;
} else if (stateChar == '!') {
ctx_dinit.servicesFailed++;
- ctx_dinit.servicesStopped++;
} else {
ctx_dinit.servicesStopped++;
}
}
fclose(commandOutput);
xWaitpid(child, NULL, 0, false);
done:
if (ctx_dinit.servicesStarted != INVALID_VALUE) {
xSnprintf(this->txtBuffer, sizeof(this->txtBuffer),
- "%zu started, %zu failed",
- ctx_dinit.servicesStarted, ctx_dinit.servicesFailed);
+ "%zu started, %zu stopped, %zu failed",
+ ctx_dinit.servicesStarted, ctx_dinit.servicesStopped, ctx_dinit.servicesFailed);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| char stateChar = lineBuffer[2]; | |
| if (stateChar == '+') { | |
| ctx_dinit.servicesStarted++; | |
| } else if (stateChar == '!') { | |
| ctx_dinit.servicesFailed++; | |
| ctx_dinit.servicesStopped++; | |
| } else { | |
| ctx_dinit.servicesStopped++; | |
| } | |
| } | |
| fclose(commandOutput); | |
| xWaitpid(child, NULL, 0, false); | |
| done: | |
| if (ctx_dinit.servicesStarted != INVALID_VALUE) { | |
| xSnprintf(this->txtBuffer, sizeof(this->txtBuffer), | |
| "%zu started, %zu failed", | |
| ctx_dinit.servicesStarted, ctx_dinit.servicesFailed); | |
| } else { | |
| xSnprintf(this->txtBuffer, sizeof(this->txtBuffer), "N/A"); | |
| } | |
| char stateChar = lineBuffer[2]; | |
| if (stateChar == '+') { | |
| ctx_dinit.servicesStarted++; | |
| } else if (stateChar == '!') { | |
| ctx_dinit.servicesFailed++; | |
| } else { | |
| ctx_dinit.servicesStopped++; | |
| } | |
| } | |
| fclose(commandOutput); | |
| xWaitpid(child, NULL, 0, false); | |
| done: | |
| if (ctx_dinit.servicesStarted != INVALID_VALUE) { | |
| xSnprintf(this->txtBuffer, sizeof(this->txtBuffer), | |
| "%zu started, %zu stopped, %zu failed", | |
| ctx_dinit.servicesStarted, ctx_dinit.servicesStopped, ctx_dinit.servicesFailed); | |
| } else { | |
| xSnprintf(this->txtBuffer, sizeof(this->txtBuffer), "N/A"); | |
| } |
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 133-133: Do not use sizeof(this) to get the number of bytes of the object in memory. It returns the size of the pointer, not the size of the object.
Context: sizeof(this->txtBuffer)
Note: [CWE-467]: Use of sizeof() on a Pointer Type
(sizeof-this-c)
[warning] 137-137: Do not use sizeof(this) to get the number of bytes of the object in memory. It returns the size of the pointer, not the size of the object.
Context: sizeof(this->txtBuffer)
Note: [CWE-467]: Use of sizeof() on a Pointer Type
(sizeof-this-c)
| /* Try /var/service first, fall back to /service */ | ||
| if (access("/var/service", F_OK) == 0) { | ||
| execlp("sv", "sv", "status", "/var/service/.", (char*)NULL); | ||
| } else { | ||
| execlp("sv", "sv", "status", "/service/.", (char*)NULL); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
sv status receives the scan directory, not the services.
sv status expects one or more service directories as arguments. /var/service/. resolves to the scan directory itself, which has no supervise subdirectory. sv reports an error, stderr is redirected to /dev/null, and the parse loop matches nothing. The meter then displays "0 up, 0 down" on a working runit system.
Enumerate the scan directory and pass each service entry to sv status, or read <service>/supervise/stat directly with opendir/fopen and avoid the child process entirely. The direct read also removes the per-refresh fork.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 79-79: execlp/execvp/execvpe resolve the program name via ; if PATH is attacker-influenced this runs an unintended binary. Use execv/execve with an absolute path.
Context: execlp("sv", "sv", "status", "/var/service/.", (char*)NULL)
Note: [CWE-426] Untrusted Search Path.
(untrusted-search-path-c)
[warning] 81-81: execlp/execvp/execvpe resolve the program name via ; if PATH is attacker-influenced this runs an unintended binary. Use execv/execve with an absolute path.
Context: execlp("sv", "sv", "status", "/service/.", (char*)NULL)
Note: [CWE-426] Untrusted Search Path.
(untrusted-search-path-c)
| if (Settings_isReadonly()) | ||
| goto done; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Drop the readonly gate in this meter.
S6Meter_updateValues only reads files. It spawns no process and writes nothing. The Settings_isReadonly() check makes the meter report "N/A" in readonly mode without a reason. Keep the gate only in the meters that fork a child process.
| linux/DinitMeter.h \ | ||
| linux/OpenRCMeter.h \ | ||
| linux/Platform.h \ | ||
| linux/RunitMeter.h \ | ||
| linux/S6Meter.h \ | ||
| linux/PressureStallMeter.h \ | ||
| linux/ProcessField.h \ | ||
| linux/SELinuxMeter.h \ | ||
| linux/SysVInitMeter.h \ |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Fix the alphabetical order of the new header entries.
linux/DinitMeter.h belongs after linux/Compat.h. linux/RunitMeter.h and linux/S6Meter.h belong after linux/ProcessField.h, before linux/SELinuxMeter.h.
Proposed fix
linux/CGroupUtils.h \
linux/Compat.h \
+ linux/DinitMeter.h \
linux/GPU.h \- linux/DinitMeter.h \
linux/OpenRCMeter.h \
linux/Platform.h \
- linux/RunitMeter.h \
- linux/S6Meter.h \
linux/PressureStallMeter.h \
linux/ProcessField.h \
+ linux/RunitMeter.h \
+ linux/S6Meter.h \
linux/SELinuxMeter.h \
linux/SysVInitMeter.h \Source: Coding guidelines
| linux/LinuxMachine.c \ | ||
| linux/LinuxProcess.c \ | ||
| linux/LinuxProcessTable.c \ | ||
| linux/DinitMeter.c \ |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Move linux/DinitMeter.c into alphabetical position.
Place it after linux/Compat.c. The other three source entries are already ordered correctly.
Source: Coding guidelines
|
Reviewed at 1.
So The 2.
3.
Only Consequences: a starting or stopping service ( Minor
Happy to re-run all of the above against a revision. I have a working s6/dinit/runit |
|
Given that the init systems/service managers are mostly independent of the OS/platform I'd suggest to put them in a generic place instead of binding them to Linux. Also implementing these meters to also be available for the various *BSDs would be nice. |
|
Agreed on the generic location, and it fits the findings above. All three parser bugs I posted are OS-independent: That also means the BSD support is mostly wiring rather than new parsing, once the scan-dir default is not hardcoded to the Linux paths. If the meters move, I am happy to re-run the s6/runit/dinit harness against the revision — that harness is what produced findings 1 to 3, and it would catch a scan-dir regression on the move. |
@ravi-arnan many thanks for code review. I’ll start fixing it now. |
0619c20 to
ee32117
Compare
|
@ravi-arnan @BenBE I fixed from code review here: ee32117 |
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: ASSERTIVE
- Plan: Advanced
- Run ID:
a7badea2-cf39-44bb-9aff-0fc2d3911928
📒 Files selected for processing (14)
DinitMeter.cDinitMeter.hMakefile.amRunitMeter.cRunitMeter.hS6Meter.cS6Meter.hSysVInitMeter.cSysVInitMeter.hdragonflybsd/Platform.cfreebsd/Platform.clinux/Platform.cnetbsd/Platform.copenbsd/Platform.c
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review.
| if (lineBuffer[2] == '+') { | ||
| started++; | ||
| } else if (lineBuffer[7] == 'X') { | ||
| failed++; | ||
| } else { | ||
| stopped++; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
sed -n '32,145p' DinitMeter.c
rg -n 'dinitctl|starting|stopping|skipped|failed' tests DinitMeter.c 2>/dev/null | head -90Repository: htop-dev/htop
Length of output: 4186
🌐 Web query:
official Dinit dinitctl list output state field skipped starting stopping stopped markers fixed width
💡 Result:
`dinitctl list` uses a **fixed-width, 9-character state indicator** before each service name. The official man page describes the markers; the implementation prints them in this order: ([github.com](https://github.com/davmac314/dinit/blob/master/doc/manpages/dinitctl.8.m4?utm_source=openai))
```text
[<active/start><transition><stop-target/failure>]
```
- `[...]` in the first position means the service is **marked active**. Otherwise, `{...}` there means its **target is started**.
- `+` means started; `s` means startup was skipped; blank means neither.
- `<<` means **starting**; `>>` means **stopping**; two blanks mean no transition.
- `{...}` in the last position means the **target is stopped**. Inside it, `-` means stopped normally and `X` means stopped due to failure. A blank means the service isn’t stopped.
Examples: `[+ ]` started; `[{ }<< ]` starting; `[ >>{-}]` stopping and will remain stopped. The official examples similarly show transitional states and target states. ([github.com](https://github.com/davmac314/dinit/blob/master/doc/manpages/dinitctl.8.m4?utm_source=openai))
Citations:
- 1: https://github.com/davmac314/dinit/blob/master/doc/manpages/dinitctl.8.m4?utm_source=openai
- 2: https://github.com/davmac314/dinit/blob/master/doc/manpages/dinitctl.8.m4?utm_source=openai
🌐 Web query:
site:github.com/davmac314/dinit dinitctl list "skipped" "started" state indicator
💡 Result:
In `dinitctl list`, **`s` means the service is started, but its startup was skipped**—possible only for a service configured as skippable. It appears where `+` would normally appear in the state indicator. ([github.com](https://github.com/davmac314/dinit/blob/master/doc/manpages/dinitctl.8.m4?utm_source=openai))
The code confirms the indicator: when the current state is `STARTED`, it prints `s` if startup was skipped, otherwise `+`. ([github.com](https://github.com/davmac314/dinit/blob/master/src/dinitctl.cc?utm_source=openai))
Citations:
- 1: https://github.com/davmac314/dinit/blob/master/doc/manpages/dinitctl.8.m4?utm_source=openai
- 2: https://github.com/davmac314/dinit/blob/master/src/dinitctl.cc?utm_source=openai
Exclude transitional services from the stopped count.
Dinit uses line[2] == 's' for a started service whose startup was skipped, so include it with +. The line[2] position is already correct.
A normal stopped service has line[7] == '-'. However, a stopping service can also have - at that position, as in [ >>{-}]. Check the transition markers at positions 4–5 before incrementing stopped.
Suggested fix
--- "a/DinitMeter.c"
+++ "b/DinitMeter.c"
@@ -111,13 +111,15 @@
continue;
hasOutput = true;
- if (lineBuffer[2] == '+') {
+ if (lineBuffer[2] == '+' || lineBuffer[2] == 's') {
started++;
} else if (lineBuffer[7] == 'X') {
failed++;
- } else {
+ } else if (lineBuffer[7] == '-' &&
+ !((lineBuffer[4] == '<' && lineBuffer[5] == '<') ||
+ (lineBuffer[4] == '>' && lineBuffer[5] == '>'))) {
stopped++;
}
}
fclose(commandOutput);
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (lineBuffer[2] == '+') { | |
| started++; | |
| } else if (lineBuffer[7] == 'X') { | |
| failed++; | |
| } else { | |
| stopped++; | |
| } | |
| if (lineBuffer[2] == '+' || lineBuffer[2] == 's') { | |
| started++; | |
| } else if (lineBuffer[7] == 'X') { | |
| failed++; | |
| } else if (lineBuffer[7] == '-' && | |
| !((lineBuffer[4] == '<' && lineBuffer[5] == '<') || | |
| (lineBuffer[4] == '>' && lineBuffer[5] == '>'))) { | |
| stopped++; | |
| } |
| while ((entry = readdir(dir)) != NULL) { | ||
| if (entry->d_name[0] == '.') | ||
| continue; | ||
|
|
||
| char servicePath[512]; | ||
| xSnprintf(servicePath, sizeof(servicePath), "%s/%s", scanDir, entry->d_name); | ||
|
|
||
| char supervisePath[540]; | ||
| xSnprintf(supervisePath, sizeof(supervisePath), "%s/supervise", servicePath); | ||
| if (access(supervisePath, F_OK) != 0) | ||
| continue; | ||
|
|
||
| char lineBuffer[256]; | ||
| if (RunitMeter_queryService(servicePath, lineBuffer, sizeof(lineBuffer)) < 0) | ||
| continue; | ||
|
|
||
| if (String_startsWith(lineBuffer, "run: ")) { | ||
| servicesUp++; | ||
| foundAny = true; | ||
| } else if (String_startsWith(lineBuffer, "down: ")) { | ||
| servicesDown++; | ||
| foundAny = true; | ||
| } else if (String_startsWith(lineBuffer, "finish: ")) { | ||
| servicesFinish++; | ||
| foundAny = true; | ||
| } | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
🔎 Supported by static analysis
🏁 Script executed:
sed -n '60,177p' RunitMeter.c
sed -n '60,174p' S6Meter.c
rg -n 'updateValues|delay.*Meter|Meter_updateValues' Meter.c ScreenManager.c 2>/dev/null | head -85Repository: htop-dev/htop
Length of output: 6171
🏁 Script executed:
set -o pipefail
printf '%s\n' '--- updateValues references and meter refresh dispatch ---'
rg -n -F --glob '*.{c,h}' -- 'updateValues' . || test "$?" -eq 1
printf '%s\n' '--- refresh loop and thread-related symbols ---'
rg -n -i --glob '*.{c,h}' 'refresh|meter.*update|update.*meter|MainLoop|pthread|thread|ScreenManager' . | head -240
printf '%s\n' '--- meter class and Runit/S6 declarations ---'
rg -n -F --glob '*.{c,h}' -- 'RunitMeter_class' .
rg -n -F --glob '*.{c,h}' -- 'S6Meter_class' .
rg -n -i --glob '*.{c,h,md,txt}' 'supervise/stat|supervise/status|s6-svstat|sv status|RunitMeter|S6Meter' . | head -240Repository: htop-dev/htop
Length of output: 31465
🏁 Script executed:
set -o pipefail
printf '%s\n' '--- Header refresh caller ---'
sed -n '210,270p' Header.c
printf '%s\n' '--- ScreenManager refresh/event loop ---'
sed -n '115,230p' ScreenManager.c
printf '%s\n' '--- enclosing main-loop callers of Header_update ---'
rg -n -i --glob '*.{c,h}' 'Header_[A-Za-z0-9_]+|updateValues\(.*meter|Meter_updateValues\(meter|lastUpdate|delay' . | head -220
printf '%s\n' '--- platform thread/event setup ---'
rg -n -i --glob '*.{c,h}' 'pthread_create|thrd_create|MainLoop|mainLoop|poll\(|select\(|usleep\(|nanosleep\(|timeout\(' . | head -220Repository: htop-dev/htop
Length of output: 18183
🌐 Web query:
official runit supervise/stat file format and s6 supervise/status file format s6-svstat documentation
💡 Result:
There are two different things here: **runit’s `supervise/stat`** and **s6’s `supervise/status`**. The official docs describe where these files are used, but don’t fully specify either on-disk format.
| File/tool | What the official documentation establishes |
|---|---|
| **runit `supervise/stat`** | `runsv` maintains human-readable status in `service/supervise/stat` (and the corresponding log-service path). Its binary status is a *different* file, `supervise/status`, documented as compatible with daemontools. The `runsv` man page does **not** define the exact text layout of `stat`. ([smarden.org](https://smarden.org/runit/runsv.8.html)) |
| **runit `sv status`** | This is the documented interface for reporting service status; it reads supervisor state and prints a report. It is not a specification of `supervise/stat`’s bytes. ([smarden.org](https://smarden.org/runit/sv.8.html)) |
| **s6 `supervise/status`** | s6 documents this as internal supervisor state, updated by `s6-supervise`; it does **not** publish a field-by-field byte layout on the `s6-svstat` page. ([skarnet.org](https://skarnet.org/software/s6/s6-supervise.html?utm_source=openai)) |
| **s6 `s6-svstat`** | The documented, supported programmatic interface is `s6-svstat -o fields servicedir`. Fields include `up`, `wantedup`, `normallyup`, `ready`, `paused`, `pid`, `pgid`, `exitcode`, `signal`/`signum`, and elapsed-time fields. Output is space-separated, one value per requested field. ([skarnet.org](https://www.skarnet.org/software/s6/s6-svstat.html?utm_source=openai)) |
**Compatibility warning:** don’t assume s6’s `supervise/status` can be parsed like runit’s. s6’s upgrade notes explicitly say its status-file ABI changed and is no longer compatible with daemontools/runit; the notes identify that change under s6 2.2.0.0. ([skarnet.org](https://skarnet.org/software/s6/upgrade.html?utm_source=openai))
**Bottom line:** use `sv status` for runit and `s6-svstat -o …` for s6 rather than treating the internal status files as a stable public format. The official runit `runsv` page confirms that `stat` is human-readable, but leaves its precise layout unspecified; the official s6 docs likewise describe status behavior and the command interface, not the raw `supervise/status` layout.
Citations:
- 1: https://smarden.org/runit/runsv.8.html
- 2: https://smarden.org/runit/sv.8.html
- 3: https://skarnet.org/software/s6/s6-supervise.html?utm_source=openai
- 4: https://www.skarnet.org/software/s6/s6-svstat.html?utm_source=openai
- 5: https://skarnet.org/software/s6/upgrade.html?utm_source=openai
🏁 Script executed:
set -o pipefail
printf '%s\n' '--- ScreenManager run owner and calls ---'
rg -n -F --glob 'ScreenManager.{c,h}' -- 'ScreenManager_run' .
sed -n '230,430p' ScreenManager.c
printf '%s\n' '--- delay declaration and initialization ---'
sed -n '1,135p' Settings.h
rg -n -i --glob '*.{c,h}' 'DEFAULT_DELAY|settings->delay|delay\s*=' . | head -120
printf '%s\n' '--- main entry and ScreenManager invocation ---'
rg -n -F --glob '*.{c,h}' -- 'ScreenManager_run(' .Repository: htop-dev/htop
Length of output: 12489
Move supervisor polling off the interactive refresh path.
ScreenManager_run calls Header_updateData after each configured update interval. The default interval is 15 tenths of a second. Both meters then fork, execute one command, and wait once per discovered service on the same interactive event-loop thread. A slow command or a large service set can therefore delay input and redraw, with cost scaling linearly with service count.
Do not read runit and s6 state files as interchangeable text formats. Their internal formats differ, and s6 does not provide a stable raw supervise/status ABI for this use. Cache the results at a longer interval or move the per-service queries to a shared asynchronous refresh mechanism. Changing only the shared refresh boundary does not fix the scaling unless it also avoids blocking the event loop.
| int wstatus; | ||
| if (parsed && xWaitpid(child, &wstatus, 0, false) >= 0 && WIFEXITED(wstatus) && WEXITSTATUS(wstatus) == 0) { | ||
| ctx_sysvInit.prevRunlevel = prev; | ||
| ctx_sysvInit.runlevel = cur; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
The child is not reaped when parsing fails.
&& short-circuits. If parsed is false, xWaitpid never runs. The runlevel child then stays a zombie. On every refresh with unparseable output, one more zombie accumulates. Reap the child unconditionally, then check the result.
Proposed fix
--- "a/SysVInitMeter.c"
+++ "b/SysVInitMeter.c"
@@ -85,8 +85,9 @@
bool parsed = (fscanf(commandOutput, " %c %c", &prev, &cur) == 2);
fclose(commandOutput);
int wstatus;
- if (parsed && xWaitpid(child, &wstatus, 0, false) >= 0 && WIFEXITED(wstatus) && WEXITSTATUS(wstatus) == 0) {
+ pid_t waited = xWaitpid(child, &wstatus, 0, false);
+ if (parsed && waited >= 0 && WIFEXITED(wstatus) && WEXITSTATUS(wstatus) == 0) {
ctx_sysvInit.prevRunlevel = prev;
ctx_sysvInit.runlevel = cur;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| int wstatus; | |
| if (parsed && xWaitpid(child, &wstatus, 0, false) >= 0 && WIFEXITED(wstatus) && WEXITSTATUS(wstatus) == 0) { | |
| ctx_sysvInit.prevRunlevel = prev; | |
| ctx_sysvInit.runlevel = cur; | |
| } | |
| int wstatus; | |
| pid_t waited = xWaitpid(child, &wstatus, 0, false); | |
| if (parsed && waited >= 0 && WIFEXITED(wstatus) && WEXITSTATUS(wstatus) == 0) { | |
| ctx_sysvInit.prevRunlevel = prev; | |
| ctx_sysvInit.runlevel = cur; | |
| } |
|
Please split the commit into one per init system. TIA. |
9062efc to
8baf21e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: ASSERTIVE
- Plan: Advanced
- Run ID:
3f2b3e2e-7540-4616-b777-c11170cba245
📒 Files selected for processing (2)
SysVInitMeter.cSysVInitMeter.h
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review.
| _exit(1); | ||
| dup2(fdnull, STDERR_FILENO); | ||
| close(fdnull); | ||
| execlp("runlevel", "runlevel", (char*)NULL); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Execute the path that passed the availability check.
Lines 42-48 can find runlevel outside PATH, but execlp searches only PATH. The meter then displays N/A despite finding an executable. If PATH contains a different runlevel first, the meter can also run a program it did not check. Save the selected absolute path and pass it to execl.
🧰 Tools
🪛 ast-grep (0.45.3)
[warning] 70-70: execlp/execvp/execvpe resolve the program name via ; if PATH is attacker-influenced this runs an unintended binary. Use execv/execve with an absolute path.
Context: execlp("runlevel", "runlevel", (char*)NULL)
Note: [CWE-426] Untrusted Search Path.
(untrusted-search-path-c)
Source: Linters/SAST tools
Fix it |
This affected popular Linux/BSD distrs: - Dinit (Artix Linux, Chimera Linux, KaOS, antiX, OpenBSD, FreeBSD)
This affected popular Linux/BSD distrs: - Runit (FreeBSD, OpenBSD, NetBSD, Void Linux, antiX, Dragora GNU/Linux-Libre, Artix Linux, Devuan, Gentoo Linux, Hyperbola GNU/Linux-libre, Debian, Ubuntu, Linux From Scratch)
This affected popular Linux/BSD distrs: - S6 (FreeBSD, OpenBSD, NetBSD, Gentoo Linux, Artix Linux, Alpine Linux, Devuan, Arch Linux, Debian, Ubuntu)
This affected popular Linux/BSD distrs: - SysVInit (Devuan, antiX, Slackware, Debian, Ubuntu, Linux From Scratch, OpenWrt, MX Linux)
8baf21e to
a011c16
Compare
This affected popular Linux/BSD distrs: