Re: [PATCH v3 2/4] compat/mingw: allow sigaction(SIGCHLD)

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Jul 9, 2025, 14:13 UTC
Message-ID: <0931e1f2-6254-474f-be91-664cec9745f5@gmail.com>
In-Reply-To: <p6xegxqqq4wzi6gnokypy3k5auxk3d2wxmj4pj45ugfomace3q@y5q3e2al42oj>

On 26/06/2025 21:09, Carlo Marcelo Arenas Belón wrote:

Show 45 quoted lines

> On Thu, Jun 26, 2025 at 04:19:11PM -0800, phillip.wood123@gmail.com 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@gmail.com>
>>>>>>>
>>>>>>> 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.

Show 6 quoted lines

> 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

Show 5 quoted lines

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