-
-
Notifications
You must be signed in to change notification settings - Fork 555
Feature/status line errors #1879
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Feature/status line errors #1879
Conversation
|
Did you test building and running the program with this thing? Is there any screenshot to show what it looks like? It seems you're throwing in a lot of patches to the htop PR list without any verification of the code quality. While I don't suggest there are slop, I have to admit: that reviewing LLM outputs is tiring. Therefore I expect some code quality guarantees for people who use LLMs in their works. |
|
I hear the concern. I locally clean build and test every PR. I attempt to test every aspect of code I touched. In this case I used kill on process I owned and those I didn't, including mixed multiselect. I ensured warning dismissal behavior, and FunctionBar behavior described as desirable in other PR. I'd like to clarify what LLM means in my PR. I do not use LLM to free write code nor do I paste LLM output. I use the LLM more like a "rubber ducky" that I can reason out code flow, check for obvious errors, and attempt to match unstated htop code preferences / designs. I am learning C and having a tool just generate the code doesn't lead to me learning anything. I will make mistakes as I learn, but every PR is written, built, and tested locally before I submit a PR. I own the PR, not the LLM. |
f73edff to
12fbcd2
Compare
| static bool Process_sendSignal(Process* this, sendSignalContext* ctx) { | ||
| if (kill(Process_getPid(this), ctx->sgn ) != 0) { | ||
| int e = errno; | ||
| if (e != ESRCH) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why filter this specific error code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ESRCH is often a silent failure in other projects/management scripting. Since no process found means the process exited before the kill command sent, the process ended, just not from kill. Signaling an error could cause confusion to users and add noise that overshadows more useful errors. This is a UX decision for usability and clarity as ESRCH errors do not facilitate any user actions, and processes exiting is normal on Unix systems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But you stop the loop instead of continue. This doesn't look like you are ignoring the error, but like you are hiding the error deliberately from user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To the end user the ESRCH/successful kill are the same, so I break the loop to return true to the caller. If it is any other error then it gets displayed on the error bar.
Edit: I misspoke. We treat ESRCH as failure, but treat it as one not worth having an error displayed.
If htop wants all errors surfaced with no filtering I can do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
htop can kill multiple processes in a loop. There's a tag feature (Space key) to select multiple processes, then press the k key, and it can kill multiple processes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DeltachangeOG I admit I didn't test myself, but, since htop can kill multiple processes at once, shouldn't the status bar messages show a list of processes that failed to kill? The message could look like:
Failed to kill process 1234: Operation not permitted
Failed to kill process 1240: Operation not permitted
Failed to kill process 5678: Operation not permitted
Failed to kill process <PID>: <message from strerror(errno)>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ESRCH currently returns false, that behavior is maintained. We could not filter out ESRCH, and maintain the return of False on any failure, but with an error message
or
Treat ESRCH as success due to the end user goal being completed either by kill or the process exiting for other reasons.
I lean toward treating ESRCH as success here since from the user’s perspective the process is already gone (same end state as a successful kill). Please let me know if you have additional thoughts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The return value for a loop callback function should indicate whether the loop should continue. The general idea (if the return type wasn't boolean) is that no error = continre loop, and error = break loop and return error.
That's what I interpret this function. It's used as a callback in a loop and so should have this semantic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't want to alter the structure that was already in place, so I maintained current behavior of the call, and just added error details when there was a failure.
I can queue errors, or display multiple errors, or we can have a hierarchy of errors and only display the "top" error.
If we want failures to break the loop and display an error then I would vote we treat ESRCH as success for those semantics.
I tried to implement this feature with as little impact on current behavior as possible, however I am happy to implement whatever direction the maintainers want for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know what the maintainers would decide, but I personally don't like having a sendSignalContext structure in a loop just to output error codes. I didn't have a good suggestion for an alternative yet.

References #1231
Closes #1863
Beginning framework for error display similar to naon. Currently only wired to sendsignal for proof of concept and discussion.
LLM used for code syntax checking.