Utilities: Add timeout command #26209
Conversation
github-actions
bot
added
the
👀 pr-needs-review
JacobKnoth
force-pushed
the
add-timeout
branch
from
7bfbef3 to
a383fe5
Compare
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions! |
There was a problem hiding this comment.
Hello and welcome to the project!
And first of all, thanks for your contribution, and sorry for the wait.
My comments are mainly on the style rather than the logic itself. We have a lot of custom stuff in the codebase, so it's normal that it takes time to get used to it. I've left some specific comments but in general:
- Check if there is a
Core::Systemequivalent before using asyscall - Prefer the
Errortype to propagate errors. And I think that you will needMain::set_return_code_for_errors(125).
If you want, you can add your utility to Lagom (so it can run and be tested on the host). Also, we encourage the addition of a man page with new utilities.
Don't hesitate to ask questions on Discord if you need help with any of that.
| struct sigaction sa; | ||
| memset(&sa, 0, sizeof(struct sigaction)); | ||
| sa.sa_handler = handle_sigint; | ||
| sigaction(SIGINT, &sa, nullptr); |
There was a problem hiding this comment.
Let's try to use less C-ism.
| struct sigaction sa; | |
| memset(&sa, 0, sizeof(struct sigaction)); | |
| sa.sa_handler = handle_sigint; | |
| sigaction(SIGINT, &sa, nullptr); | |
| struct sigaction sa{}; // This will zero initialize the struct. | |
| sa.sa_handler = handle_sigint; | |
| TRY(Core::System::sigaction(SIGINT, &sa, nullptr)); // To get error handling. |
| perror("negative timeout"); | ||
| return 125; | ||
| } | ||
| TRY(Core::System::pledge("stdio proc exec sigaction")); |
There was a problem hiding this comment.
I haven't used pledge with sigaction but I would expect that it is only required for the syscall. And here you're already done with that so you could drop sigaction from the pledge line. Feel free to dismiss if my assumption is false.
| double whole_seconds = static_cast<time_t>(secs); | ||
| double fraction = secs - whole_seconds; | ||
| timespec requested_timeout { | ||
| .tv_sec = static_cast<time_t>(whole_seconds), | ||
| .tv_nsec = static_cast<long>(fraction * (double)1000000000), | ||
| }; | ||
| timespec tmp {}; | ||
| clock_gettime(CLOCK_MONOTONIC, &tmp); | ||
| timespec deadline = { | ||
| .tv_sec = tmp.tv_sec + requested_timeout.tv_sec, | ||
| .tv_nsec = tmp.tv_nsec + requested_timeout.tv_nsec, | ||
| }; | ||
| if (deadline.tv_nsec >= 1'000'000'000) { | ||
| deadline.tv_sec++; | ||
| deadline.tv_nsec -= 1'000'000'000; | ||
| } |
There was a problem hiding this comment.
We have some time utilities to help you here.
From AK/Time.h, you can invoke MonotonicTime::now() and then add a Duration that you created from secs (that can probably have a less abbreviated and more descriptive name, e.g. seconds, time_limit).
| pid_t child_pid = fork(); | ||
| if (child_pid < 0) { | ||
| perror("fork"); | ||
| return 125; | ||
| } | ||
| if (child_pid == 0) { | ||
| setpgid(0, 0); | ||
| execvp(argv_ptrs[0], argv_ptrs.data()); | ||
| perror("execvp"); | ||
| if (errno == ENOENT) { | ||
| _exit(127); | ||
| } | ||
| _exit(126); |
There was a problem hiding this comment.
Again, Core::System got your back here. And Core::System::exec will even take a Span<StringView> so you won't have to convert the Vector from ArgsParser.
| waitpid(child_pid, &status, 0); | ||
| TRY(Core::System::signal(SIGINT, SIG_DFL)); | ||
| raise(SIGINT); | ||
| __builtin_unreachable(); |
There was a problem hiding this comment.
We use VERIFY_NOT_REACHED(); for this.
spholz
added
⏳ pr-waiting-for-author
3 participants
Minimal GNU styled 'timeout',
runs a command with a time limit (monotonic clock) sends a SIGTERM to the child process group at deadline then SIGKILL after a grace period
Exit codes: 124(timeout), 125(internal/usage), 126/127(exec error) Otherwise child status