Skip to content

Conversation

@GrayHatter
Copy link
Contributor

I wanted to name this fn sigTimedWait to 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 in std.os.linux.MAP but didn't here to keep this commit unobjectionable.

.SUCCESS => @enumFromInt(sig_num),
.AGAIN => error.TimeoutElapsed,
.INTR => error.OtherSignal,
.ENVAL => error.InvalidTimeout,
Copy link
Member

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?

Copy link
Contributor

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.)

Copy link
Contributor Author

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?

Copy link
Contributor Author

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)) {
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

else => |err| return std.os.unexpectedErrno(err),

else => |err| return unexpectedErrno(err),

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.

Copy link
Member

@andrewrk andrewrk left a 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

@GrayHatter
Copy link
Contributor Author

@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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants