-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
Added std.os.linux.sigtimedwait #25946
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: master
Are you sure you want to change the base?
Conversation
| .SUCCESS => @enumFromInt(sig_num), | ||
| .AGAIN => error.TimeoutElapsed, | ||
| .INTR => error.OtherSignal, | ||
| .ENVAL => error.InvalidTimeout, |
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.
Isn't this a classic case of programmer error?
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.
According to #6389 this EINVAL should probably map to Unexpected? (I'm a fan of mapping the errnos 1:1 like the original PR here, but I believe 6389 is the current Zig plan of record.)
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.
Isn't this a classic case of programmer error?
That is more likely, but I'm not a fan of the current stdlib behavior of cutting it's own head off when something unexpected happens. I can replace this with invalidApiUsage() but I'd much rather expose the same API semantics that linux does. given this is a linux syscall.
Someone wanna make the call and I'll update the diff?
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.
An alternative would be using something like Io.Duration, and converting it internally. Then it's a prevented bug, instead of one that crashes the app... unless linux does something weird, and rejects a valid timeout?
| ) TimedWaitError!SIG { | ||
| const sig_num = syscall4(.rt_sigtimedwait, @intFromPtr(&sigset), @intFromPtr(siginfo), @intFromPtr(timeout), NSIG / 8); | ||
|
|
||
| return switch (E.init(sig_num)) { |
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.
Functions in std.os.linux do not to switch on errno values. This function should probably just return sig_num as an usize and another function in std.os.linux.wrapped or std.posix can then call this function and switch on the 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.
Line 1896 in 879ea27
| else => |err| return std.os.unexpectedErrno(err), |
Line 9908 in 73f863a
| else => |err| return unexpectedErrno(err), |
Line 9975 in 73f863a
| else => |err| return unexpectedErrno(err), |
I thought the exact same thing, but decided to keep it here mostly because posix as it is now isn't long for the repo.
andrewrk
left a comment
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.
please look around and follow the conventions from the surrounding code
|
@andrewrk can I get a bit more of a hint about which direction you'd like to see the changes? directly above the new fn is pub fn gettid() pid_t {
// Casts result to a pid_t, safety-checking >= 0, because gettid() cannot fail
return @intCast(@as(u32, @truncate(syscall0(.gettid))));
}and below is pub fn sigaction(sig: SIG, noalias act: ?*const Sigaction, noalias oact: ?*Sigaction) usize {
// ...
if (E.init(result) != .SUCCESS) return result;
if (oact) |old| {
old.handler.handler = oldksa.handler;
old.flags = oldksa.flags;
old.mask = oldksa.mask;
}
return 0;
}I believe that the shape of this function resembles the latter? Which convention would you prefer? |
I wanted to name this fn
sigTimedWaitto match zig style. But decided not to given all (most?) other functions here duplicate the C-API style.I'm not up to date on the current recommendations for how the stdlib should look. I considered introducing
SigSet, as a full zig struct similar to the packed struct instd.os.linux.MAPbut didn't here to keep this commit unobjectionable.