ziglang
Add 0 to SIG enum
Closes #26011
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.
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 like the idea of the fix, but the name
INVALfor signal 0 isn't great. My other ideas aren't better though:ZEROseems equally wrong,NONEmight work? (E.g.,kill(pid, SIG.NONE)I wonder if Zig should make the
SIGenums 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.
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)....
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?
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)
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...?)
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...?
@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.
