On 26/06/2025 21:09, Carlo Marcelo Arenas Belón wrote:
On Thu, Jun 26, 2025 at 04:19:11PM -0800, phillip.wood123@xxxxxxxxx wrote:
On 26/06/2025 15:58, Carlo Marcelo Arenas Belón wrote:
On Thu, Jun 26, 2025 at 02:56:22PM -0800, Phillip Wood wrote:
On 26/06/2025 14:15, Carlo Marcelo Arenas Belón wrote:
On Thu, Jun 26, 2025 at 01:52:47PM -0800, Phillip Wood wrote:
On 26/06/2025 09:53, Carlo Marcelo Arenas Belón via GitGitGadget wrote:
From: =?UTF-8?q?Carlo=20Marcelo=20Arenas=20Bel=C3=B3n?= <carenas@xxxxxxxxx>
A future change will start using sigaction to setup a SIGCHLD signal
handler.
The current code uses signal() which returns SIG_ERR (but doesn't
seem to set errno) so instruct sigaction() to do the same.
Why are we returning -1 below instead of SIG_ERR if we want the behavior to
match?
By "match", I mean that in both cases we will get an error return value
and errno won't be set to EINVAL (which is what POSIX requires)
In our codebase since we ignore the return code anyway, it wouldn't make
a difference, either way.
signal() returns a pointer, and sigaction() returns and int,
Oh right, I'd forgotten they have different return types. I think we should
probably be setting errno = EINVAL before returning -1 to match what this
function does with other signals it does not support - just because our
current callers ignore the return value doesn't mean that future callers
will and they might want check errno if they see the function fail.
I agree, and indeed had to triple check and change my implementation after I
confirmed that signal(SIGCHLD) does not change errno on Windows (not our
version, neither of the windows libc or mingw, even if it is documented[1] to
do so.
It might be because the signal number itself is bogus (there is none for
SIGCHLD in their headers, and git uses their own numbers in compat), but
either way, I would rather be consistent with signal() at least originally.
I'm not sure I understand - don't we want the sigaction() wrapper to behave
like sigaction() would?
for at least the first iteration, I would rather have sigaction() behave
like signal(), so that the change doesn't introduce any regressions.
What regressions are you worried about? We're talking about changing a
single call from signal() to sigaction(). I'd have thought we're far
more likely to introduce regressions if we change the behavior of the
windows implementation of sigaction() to behave like signal() as that
introduces more variation between different platforms.
eventually, sigaction() should behave like any other sigaction(), but to
do so, I suspect the windows emulation might need to change their SIGCHLD
to match.
just confirmed with MSVC that if I use 20 instead of 17, errno gets updated
just like the documentation says it should.
Oh not so setting errno as you want to do would not actually match
signal() on Windows in that case?
Thanks
Phillip
Carlo
PS. Maybe we should get dscho involved?
[1] https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/signal
