[PATCH] posix: Make posix_spawn extensions available by default

[PATCH] posix: Make posix_spawn extensions available by default

[Florian Weimer]

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(-)

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

Re: [PATCH] posix: Make posix_spawn extensions available by default

[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.

> ---
>  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
> 

Re: [PATCH] posix: Make posix_spawn extensions available by default

[Florian Weimer]

* 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

Re: [PATCH] posix: Make posix_spawn extensions available by default

[Adhemerval Zanella Netto]

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>

Re: [PATCH] posix: Make posix_spawn extensions available by default

[Sam James]

[-- Attachment #1: Type: text/plain, Size: 1610 bytes --]



> 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

[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 358 bytes --]

Re: [PATCH] posix: Make posix_spawn extensions available by default

[Florian Weimer]

* 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

Re: [PATCH] posix: Make posix_spawn extensions available by default

[Sam James]

[-- Attachment #1: Type: text/plain, Size: 1875 bytes --]



> 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



[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 358 bytes --]