posix: Make posix_spawn extensions available by default
Checks
Context |
Check |
Description |
dj/TryBot-apply_patch |
success
|
Patch applied to master at the time it was sent
|
dj/TryBot-32bit |
success
|
Build for i686
|
Commit Message
Some sources merely include <spawn.h> without -D_GNU_SOURCE and expect
declarations for posix_spawn_file_actions_addchdir_np to be available.
Tested on x86_64-linux-gnu.
---
posix/spawn.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
base-commit: faaf733f49211439475e50f06716b303ee2644bf
Comments
On 04/11/22 04:15, Florian Weimer via Libc-alpha wrote:
> Some sources merely include <spawn.h> without -D_GNU_SOURCE and expect
> declarations for posix_spawn_file_actions_addchdir_np to be available.
>
> Tested on x86_64-linux-gnu.
>
LGTM, although you reference posix_spawn_file_actions_addchdir_np on commit
message, but you export posix_spawn_file_actions_addtcsetpgrp_np.
> ---
> posix/spawn.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/posix/spawn.h b/posix/spawn.h
> index c4a81227b0..f3bcbc56be 100644
> --- a/posix/spawn.h
> +++ b/posix/spawn.h
> @@ -198,7 +198,7 @@ extern int posix_spawn_file_actions_adddup2 (posix_spawn_file_actions_t *
> int __fd, int __newfd)
> __THROW __nonnull ((1));
>
> -#ifdef __USE_GNU
> +#ifdef __USE_MISC
> /* Add an action changing the directory to PATH during spawn. This
> affects the subsequent file actions. */
> extern int posix_spawn_file_actions_addchdir_np (posix_spawn_file_actions_t *
> @@ -227,7 +227,7 @@ posix_spawn_file_actions_addtcsetpgrp_np (posix_spawn_file_actions_t *,
> int __tcfd)
> __THROW __nonnull ((1));
>
> -#endif
> +#endif /* __USE_MISC */
>
> __END_DECLS
>
>
> base-commit: faaf733f49211439475e50f06716b303ee2644bf
>
* Adhemerval Zanella Netto:
> On 04/11/22 04:15, Florian Weimer via Libc-alpha wrote:
>> Some sources merely include <spawn.h> without -D_GNU_SOURCE and expect
>> declarations for posix_spawn_file_actions_addchdir_np to be available.
>>
>> Tested on x86_64-linux-gnu.
>>
>
> LGTM, although you reference posix_spawn_file_actions_addchdir_np on commit
> message, but you export posix_spawn_file_actions_addtcsetpgrp_np.
Sorry, there are multiple functions that are now available by default.
I encountered the problem just with posix_spawn_file_actions_addchdir_np,
so I still think the commit message is correct.
Thanks,
Florian
On 04/11/22 09:10, Florian Weimer wrote:
> * Adhemerval Zanella Netto:
>
>> On 04/11/22 04:15, Florian Weimer via Libc-alpha wrote:
>>> Some sources merely include <spawn.h> without -D_GNU_SOURCE and expect
>>> declarations for posix_spawn_file_actions_addchdir_np to be available.
>>>
>>> Tested on x86_64-linux-gnu.
>>>
>>
>> LGTM, although you reference posix_spawn_file_actions_addchdir_np on commit
>> message, but you export posix_spawn_file_actions_addtcsetpgrp_np.
>
> Sorry, there are multiple functions that are now available by default.
> I encountered the problem just with posix_spawn_file_actions_addchdir_np,
> so I still think the commit message is correct.
I would advise cite all the function affected then, because with only
posix_spawn_file_actions_addchdir_np on commit message it might seems that
the patch is exporting more than intended.
Patch looks ok though.
Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>
> On 4 Nov 2022, at 12:26, Adhemerval Zanella Netto via Libc-alpha <libc-alpha@sourceware.org> wrote:
>
>
>
> On 04/11/22 09:10, Florian Weimer wrote:
>> * Adhemerval Zanella Netto:
>>
>>> On 04/11/22 04:15, Florian Weimer via Libc-alpha wrote:
>>>> Some sources merely include <spawn.h> without -D_GNU_SOURCE and expect
>>>> declarations for posix_spawn_file_actions_addchdir_np to be available.
>>>>
>>>> Tested on x86_64-linux-gnu.
>>>>
>>>
>>> LGTM, although you reference posix_spawn_file_actions_addchdir_np on commit
>>> message, but you export posix_spawn_file_actions_addtcsetpgrp_np.
>>
>> Sorry, there are multiple functions that are now available by default.
>> I encountered the problem just with posix_spawn_file_actions_addchdir_np,
>> so I still think the commit message is correct.
>
> I would advise cite all the function affected then, because with only
> posix_spawn_file_actions_addchdir_np on commit message it might seems that
> the patch is exporting more than intended.
>
Agreed. I'm a little bit uneasy given this ends up masking some problems
and then we're back to having to only fix them on musl systems, but if
this is the only function not exposed and the logic already applies to the others,
I guess it's fine.
(In particular, there's various functions from glibc where I have to go around
and spot missing _GNU_SOURCE right now for Clang 16/modern C porting,
but if we pursue more changes like this, we end up having to do more work
to find issues on other platforms like musl instead of getting it for free.)
thanks,
sam
* Sam James:
>> On 4 Nov 2022, at 12:26, Adhemerval Zanella Netto via Libc-alpha <libc-alpha@sourceware.org> wrote:
>>
>>
>>
>> On 04/11/22 09:10, Florian Weimer wrote:
>>> * Adhemerval Zanella Netto:
>>>
>>>> On 04/11/22 04:15, Florian Weimer via Libc-alpha wrote:
>>>>> Some sources merely include <spawn.h> without -D_GNU_SOURCE and expect
>>>>> declarations for posix_spawn_file_actions_addchdir_np to be available.
>>>>>
>>>>> Tested on x86_64-linux-gnu.
>>>>>
>>>>
>>>> LGTM, although you reference posix_spawn_file_actions_addchdir_np on commit
>>>> message, but you export posix_spawn_file_actions_addtcsetpgrp_np.
>>>
>>> Sorry, there are multiple functions that are now available by default.
>>> I encountered the problem just with posix_spawn_file_actions_addchdir_np,
>>> so I still think the commit message is correct.
>>
>> I would advise cite all the function affected then, because with only
>> posix_spawn_file_actions_addchdir_np on commit message it might seems that
>> the patch is exporting more than intended.
>>
>
> Agreed. I'm a little bit uneasy given this ends up masking some
> problems and then we're back to having to only fix them on musl
> systems, but if this is the only function not exposed and the logic
> already applies to the others, I guess it's fine.
I have submitted a fix to the application as well:
Build process.c with _GNU_SOURCE
<https://github.com/apple/swift-tools-support-core/pull/363>
But I think the glibc change makes sense independently of that.
Thanks,
Florian
> On 7 Nov 2022, at 16:10, Florian Weimer via Libc-alpha <libc-alpha@sourceware.org> wrote:
>
> * Sam James:
>
>>> On 4 Nov 2022, at 12:26, Adhemerval Zanella Netto via Libc-alpha <libc-alpha@sourceware.org> wrote:
>>>
>>>
>>>
>>> On 04/11/22 09:10, Florian Weimer wrote:
>>>> * Adhemerval Zanella Netto:
>>>>
>>>>> On 04/11/22 04:15, Florian Weimer via Libc-alpha wrote:
>>>>>> Some sources merely include <spawn.h> without -D_GNU_SOURCE and expect
>>>>>> declarations for posix_spawn_file_actions_addchdir_np to be available.
>>>>>>
>>>>>> Tested on x86_64-linux-gnu.
>>>>>>
>>>>>
>>>>> LGTM, although you reference posix_spawn_file_actions_addchdir_np on commit
>>>>> message, but you export posix_spawn_file_actions_addtcsetpgrp_np.
>>>>
>>>> Sorry, there are multiple functions that are now available by default.
>>>> I encountered the problem just with posix_spawn_file_actions_addchdir_np,
>>>> so I still think the commit message is correct.
>>>
>>> I would advise cite all the function affected then, because with only
>>> posix_spawn_file_actions_addchdir_np on commit message it might seems that
>>> the patch is exporting more than intended.
>>>
>>
>> Agreed. I'm a little bit uneasy given this ends up masking some
>> problems and then we're back to having to only fix them on musl
>> systems, but if this is the only function not exposed and the logic
>> already applies to the others, I guess it's fine.
>
> I have submitted a fix to the application as well:
>
> Build process.c with _GNU_SOURCE
> <https://github.com/apple/swift-tools-support-core/pull/363>
>
> But I think the glibc change makes sense independently of that.
Alright, WFM. I just ask we keep this consideration in mind
for future bits, but the boat has sailed here wrt pthreads.
>
> Thanks,
> Florian
best,
sam
@@ -198,7 +198,7 @@ extern int posix_spawn_file_actions_adddup2 (posix_spawn_file_actions_t *
int __fd, int __newfd)
__THROW __nonnull ((1));
-#ifdef __USE_GNU
+#ifdef __USE_MISC
/* Add an action changing the directory to PATH during spawn. This
affects the subsequent file actions. */
extern int posix_spawn_file_actions_addchdir_np (posix_spawn_file_actions_t *
@@ -227,7 +227,7 @@ posix_spawn_file_actions_addtcsetpgrp_np (posix_spawn_file_actions_t *,
int __tcfd)
__THROW __nonnull ((1));
-#endif
+#endif /* __USE_MISC */
__END_DECLS