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. 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. Carlo PS. Maybe we should get dscho involved? > > > > [1] https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/signal
