Show error message when trace feature fails on fork#2013
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughSummaryTwo small, focused changes fix issue
Implementation qualityClean, minimal, and idiomatic: preserves errno with the standard pattern, avoids clobbering errors during cleanup, and ensures the UI always presents immediate feedback. Changes are narrowly scoped, no public API changes, and follow conventional C error-handling. Commit split is logical and in-scope (UI/path handling in Action.c; error-preservation and robustness in TraceScreen.c). TestsBuild and unit checks pass (./autogen.sh && ./configure && make; make check). Manual test guidance included (e.g., ulimit -u 0 or seccomp to simulate fork failure) to verify the visible error message. Fixes WalkthroughTraceScreen_forkTracer() now preserves errno during its error-cleanup. actionStrace includes <errno.h>, captures strerror(errno) on fork failure, appends that message to the TraceScreen/InfoScreen, and runs InfoScreen regardless of success. TraceScreen_updateTrace() now initializes fd_strace to -1, only calls fileno(this->strace) when non-NULL, and gates readiness/FD_ISSET checks on fd_strace >= 0 and this->strace_alive. Assessment against linked issues
Out-of-scope changes
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Infer (1.2.0)TraceScreen.cTraceScreen.c:8:10: fatal error: 'config.h' file not found ... [truncated 689 characters] ... Action.cAction.c:8:10: fatal error: 'config.h' file not found ... [truncated 670 characters] ... tem" 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
In the second commit you accidentally deleted |
When TraceScreen_forkTracer() fails (e.g. OOM, PID quota exceeded, seccomp/AppArmor restrictions), the TraceScreen was not displayed and users received no feedback. The UI appeared unresponsive. - Show error message in TraceScreen when forkTracer fails - Preserve errno across cleanup in error path for accurate reporting - Use plain snprintf (not xSnprintf) to avoid fail() on truncation - Guard NULL strace before fileno() to prevent crash - Guard all fd_strace usage with fd_strace >= 0 checks Fixes htop-dev#1991 Assisted-by: Claude (Anthropic)
41e7977 to
d66df1f
Compare
|
Hi, is the issue resolved? if not yet please assign here i have got an idea to resolve it minimal fix. |
|
@Fakekeymaster The PR from @re2zero looks ok and ready to merge. If you feel you can implement it better, do so a separate PR and we can compare. Be sure to have your AI credited in Assisted-by: lines as per our AI contributions policy. |
|
@fasterit Looks good overall. Two things worth flagging:
|
|
@Fakekeymaster Both sound reasonable. Please post a patch here (git format-patch) or clone this PR and add your fix commit on top. |
I suggested the use of raw Since this can draw confusion to you, I think a line of code comment can be added there, saying "
The existing function bar is already able to tell how the user can exit the screen. |
|
Thanks for the context, @Explorer09. Both points are fair. On snprintf: I hadn't considered that xSnprintf could crash in an error path — that's a good reason to keep it as-is. A short comment there would definitely help future contributors understand the intent. On the UX hint: You're right, the function bar already handles it. My oversight. However, if any other feature or fix is required in other parts of the codebase, I will always be happy and available to help. Thanks |
|
Sounds like we should merge as it is... |
5 participants
Summary
TraceScreen_forkTracer()failsProblem
When the trace feature (strace/truss) fails during the fork phase (e.g., out of memory, PID quota exceeded, seccomp/AppArmor restrictions), the TraceScreen was not displayed and users received no feedback. The UI appeared unresponsive even when pressing the
skey multiple times.Solution
actionStrace()to always run the InfoScreen, adding an error message line whenforkTracerfailsTraceScreen_forkTracer()to preserve errno across cleanup operations for accurate error reportingChanges
Action.cTraceScreen.cTest Plan
./autogen.sh && ./configure && makemake checkpassesulimit -u 0or use seccomp) and verify error message appears in TraceScreenAI disclosure
Assisted-by: Claude (Anthropic)
Fixes #1991