From: phillip.wood123@gmail.com
Date: Thu, 26 Jun 2025 15:19:11 GMT
Subject: Re: [PATCH v3 2/4] compat/mingw: allow sigaction(SIGCHLD)
Message-ID: <0dd51eab-8869-46be-beca-238a616dd6f3@gmail.com>
In-Reply-To: <o6cihjnfj4q6uiks3syovjun3fcijvsqto444osw7tgtpkttvt@42r37athz2tw>

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?

Thanks

Phillip

> Carlo
> 
> [1] https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/signal