Add 0 to SIG enum #26012

psznm · 2025-11-22T18:43:57Z

I checked for manpages of kill on linux, freebsd, macos, openbsd, illumos, netbsd, openbsd. They all stated 0 can be used. I did not find manpages for others, but I assume it is all the same, so i added 0 to all of the enums.

rootbeer · 2025-11-22T20:15:09Z

I like the idea of the fix, but the name INVAL for signal 0 isn't great. My other ideas aren't better though: ZERO seems equally wrong, NONE might work? (E.g., kill(pid, SIG.NONE)

I wonder if Zig should make the SIG enums non-exhaustive? If some kernel adds support for new signals (which is unlikely but is entirely its prerogative?), then users should be able to pass other integers.

Zig core team folks might have better ideas or stronger opinions. This patch does unblock the kill-0 case, so I think it is an improvement over the status quo.

psznm · 2025-11-22T21:07:58Z

I like the idea of the fix, but the name INVAL for signal 0 isn't great. My other ideas aren't better though: ZERO seems equally wrong, NONE might work? (E.g., kill(pid, SIG.NONE)

I wonder if Zig should make the SIG enums non-exhaustive? If some kernel adds support for new signals (which is unlikely but is entirely its prerogative?), then users should be able to pass other integers.

Zig core team folks might have better ideas or stronger opinions. This patch does unblock the kill-0 case, so I think it is an improvement over the status quo.

I thought of making it NONE at first, but then I noticed one of the SIG implementations already had 0 named INVAL so that is what I went with. When you think about it, it makes sense. It is not a signal at all, so signal 0 is "invalid". But yes, in the case of kill, NONE makes more sense.

nektro · 2025-11-22T21:22:48Z

i would propose ZERO, STATUS, or making it non-exhaustive

rootbeer · 2025-11-23T01:20:55Z

I thought of making it NONE at first, but then I noticed one of the SIG implementations already had 0 named INVAL so that is what I went with.

Ah, looks like this is set on the Serenity SIG enum, and it probably was inherited from their header file: https://github.com/SerenityOS/serenity/blob/d0e77ec377b0a2b8f70e19050bb90586bae2ef8a/Kernel/API/POSIX/signal_numbers.h#L9 So there is some precedent for this name, but I'm not sure where they got it from (that header is part of Serenity's POSIX compatibility, but I don't think SIGINVAL is a POSIX thing ...). Overall, I don't find this case too compelling (but I can see why you're deferring to the existing name)....

rootbeer · 2025-11-23T01:25:38Z

Oh, one useful suggestion I have is to add a simple test case for this so it doesn't get elided in the future (just ensure that you can send signal 0 to the current pid and get a non-error result?). You can probably put it in lib/std/posix/test.zig?

psznm · 2025-11-23T10:13:04Z

Oh, one useful suggestion I have is to add a simple test case for this so it doesn't get elided in the future (just ensure that you can send signal 0 to the current pid and get a non-error result?). You can probably put it in lib/std/posix/test.zig?

I have added the test.

I am pretty confident about it, but I am struggling to make the tests run in a way that I can see that it is passing. Figured I could used -Dtest-filter, but I didn't figure out how, as no matter what I put there, it seems the test output runs way too much and always is the same (and I couldn't see my new test neither passing or failing)

squeek502 · 2025-11-24T07:55:50Z

lib/std/posix/test.zig

+test "kill 0 can be used to perform checks of sending signal" {
+    if (native_os == .wasi) return error.SkipZigTest;
+    if (native_os == .windows) return error.SkipZigTest;
+    posix.kill(posix.getpid(), .INVAL) catch |err| switch (err) {
+        posix.KillError.PermissionDenied => return,
+        else => return err,
+    };
+}


This should probably be moved into a standalone test. See https://github.com/ziglang/zig/tree/master/test/standalone/posix for a potential standalone test that it'd make sense to move it to.

(note: for the purposes of locally testing a standalone test, you can just do e.g. cd test/standalone/posix and zig build test)

Given its not really sending a signal (just testing if the existence/permissions are fine), I think its okay to keep here in the normal unit tests. But it is a bit of a grey area. (For everyone else: signals and signal handling can interfere with the Zig unit test framework, so some Posix test cases are over in test/standalone/posix/ if they need to be isolated.)

Ah, I see now that this is part of the POSIX spec:

If sig is 0 (the null signal), error checking is performed but no signal is actually sent. The null signal can be used to check the validity of pid.

https://pubs.opengroup.org/onlinepubs/9799919799/functions/kill.html

It still kinda seems like it might be better in a standalone test, though, just to ensure that if the system doesn't properly implement that part of the spec it doesn't have a chance of messing with the unit test runner.

rootbeer · 2025-11-24T08:26:12Z

I am pretty confident about it, but I am struggling to make the tests run in a way that I can see that it is passing. Figured I could used -Dtest-filter, but I didn't figure out how, as no matter what I put there, it seems the test output runs way too much and always is the same (and I couldn't see my new test neither passing or failing)

The posix tests get run in a bunch of configurations, so you should see this tests execute a couple times. I recommend putting a compile time or run-time error in the test, and making sure you can trigger that. Just to be sure you're running the right target (which I believe should be zig build test-std). It may take a while on the first run, but subsequent runs are quick if little has changed.

Looking at the test more closely, its not clear to me why PermissionDenied error is being ignored. I think sending a signal to the current pid should always be allowed? (In which case this could just be a try posix.kill...?)

psznm · 2025-11-24T12:34:35Z

Looking at the test more closely, its not clear to me why PermissionDenied error is being ignored. I think sending a signal to the current pid should always be allowed? (In which case this could just be a try posix.kill...?)

I will be changing this later to try current process and also non existent process. One of my unit tests detected that error checking of kill is not actually working, see #26034, so I am not sure if I should add failing test in this PR...?

psznm · 2025-11-24T17:29:44Z

@rootbeer @squeek502 I made the test standalone and also added test case for missing process. I have also sucessfully ran the tests - I had some issues with using zig binary built from wrong version.

alexrp · 2025-11-24T18:36:58Z

test/standalone/posix/kill.zig

+fn test_kill_nonexistent() !void {
+    const impossible_pid: posix.pid_t = 1_999_999_999;
+    posix.kill(impossible_pid, .INVAL) catch |err| switch (err) {
+        posix.KillError.ProcessNotFound => return,
+        else => return err,
+    };
+    return error.ProcessShouldHaveNotBeenFound;
+}


All it takes is a write to /proc/sys/kernel/pid_max on Linux to make this test unreliable.

Unless you can come up with a non-flaky and portable way of obtaining a truly "impossible" PID, this part of the test can't be merged.

All it takes is a write to /proc/sys/kernel/pid_max on Linux to make this test unreliable.

Unless you can come up with a non-flaky and portable way of obtaining a truly "impossible" PID, this part of the test can't be merged.

I think you might be wrong. There appears to be a constant in linux PID_MAX_LIMIT which limits that number to somewhere around 4 million. https://github.com/torvalds/linux/blob/master/include/linux/threads.h#L34

Attempting # echo "4194305" > /proc/sys/kernel/pid_max confirms that, at least for linux. I am not knowledgable of other posix systems, but even then it seems better to have this test that is extremely unlikely to ever be flakey, than to not have the test at all?

We can have the test if it's only enabled for operating systems on which it can be verified that the PID is in fact impossible. We have a pretty strict "no flaky tests" policy.

I have limited the test to linux (limited by the constant) and macos (results of searching generally agree on a non-configurable and verifiable number)

I have also excluded windows from the test because it apparently does not have kill -0 equivalent

Given this is testing Zig's handling of the error path (vs. testing the OS's error path), it seems sufficient (and safe) to make this a (native_os == .linux) only test. Then you can be confident in the PID_MAX_LIMIT, and we exercise the Zig code well enough?

Apologies, github didn't show me your most recent update, which basically does what I suggested, plus MacOS.

Given this is testing Zig's handling of the error path (vs. testing the OS's error path), it seems sufficient (and safe) to make this a (native_os == .linux) only test. Then you can be confident in the PID_MAX_LIMIT, and we exercise the Zig code well enough?

Thanks for the suggestion, I have already limited to linux and macos. both of which I am confident of never being able to hit the PID limit, if that is fine?

Apologies, github didn't show me your most recent update, which basically does what I suggested, plus MacOS.

Thanks for the suggestion, I have already limited to linux and macos. both of which I am confident of never being able to hit the PID limit, if that is fine?

Same, same :D That is resolved than.

rootbeer

LGTM, thanks for adding the tests.

rootbeer · 2025-11-24T19:35:00Z

test/standalone/posix/kill.zig

+fn test_kill_nonexistent() !void {
+    const impossible_pid: posix.pid_t = 1_999_999_999;
+    posix.kill(impossible_pid, .INVAL) catch |err| switch (err) {
+        posix.KillError.ProcessNotFound => return,
+        else => return err,
+    };
+    return error.ProcessShouldHaveNotBeenFound;
+}


Given this is testing Zig's handling of the error path (vs. testing the OS's error path), it seems sufficient (and safe) to make this a (native_os == .linux) only test. Then you can be confident in the PID_MAX_LIMIT, and we exercise the Zig code well enough?

rootbeer · 2025-11-24T19:40:24Z

test/standalone/posix/kill.zig

+    // MacOS maximum pid appears to be 99999 and not configurable.
+    // Others are unknown thus not tested.
+    const impossible_pid: posix.pid_t = 1_999_999_999;
+    posix.kill(impossible_pid, .INVAL) catch |err| switch (err) {


I think you could use std.testing.expectError() here to simplify a bit:

std.testing.expectError(posix.KillError.ProcessNotFound, posix.kill(impossible_pid, .INVAL));

Thanks! I'll have to inspect std.testing more.

Add 0 to SIG enum

9781e02

Add test for kill 0

5aff6cd

squeek502 reviewed Nov 24, 2025

View reviewed changes

psznm mentioned this pull request Nov 24, 2025

kill error checking does not work #26034

Closed

Standalone kill test

2f738eb

psznm force-pushed the signal-0 branch from 2a59503 to 2f738eb Compare November 24, 2025 17:34

psznm requested review from rootbeer and squeek502 November 24, 2025 17:55

alexrp reviewed Nov 24, 2025

View reviewed changes

Limit test to OS with known maximum PID. Omit windows from kill 0 test

cdb85b2

rootbeer reviewed Nov 24, 2025

View reviewed changes

use testing.expectError

a79ccf9

psznm requested a review from alexrp November 24, 2025 21:52

Uh oh!

Add 0 to SIG enum #26012

Are you sure you want to change the base?

Add 0 to SIG enum #26012

Conversation

psznm commented Nov 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rootbeer commented Nov 22, 2025

Uh oh!

psznm commented Nov 22, 2025

Uh oh!

nektro commented Nov 22, 2025

Uh oh!

rootbeer commented Nov 23, 2025

Uh oh!

rootbeer commented Nov 23, 2025

Uh oh!

psznm commented Nov 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

squeek502 Nov 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rootbeer commented Nov 24, 2025

Uh oh!

psznm commented Nov 24, 2025

Uh oh!

psznm commented Nov 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

psznm Nov 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rootbeer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

psznm commented Nov 22, 2025 •

edited

Loading

psznm commented Nov 23, 2025 •

edited

Loading

squeek502 Nov 24, 2025 •

edited

Loading

psznm commented Nov 24, 2025 •

edited

Loading

psznm Nov 24, 2025 •

edited

Loading