posix: Make posix_spawn extensions available by default

Message ID 875yfv2nsa.fsf@oldenburg.str.redhat.com
State Committed
Commit 2ff48a4025515e93d722947a9eabb114f4a65b22
Headers
Series 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

Florian Weimer Nov. 4, 2022, 7:15 a.m. UTC
  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

Adhemerval Zanella Netto Nov. 4, 2022, 12:04 p.m. UTC | #1
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
>
  
Florian Weimer Nov. 4, 2022, 12:10 p.m. UTC | #2
* 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
  
Adhemerval Zanella Netto Nov. 4, 2022, 12:26 p.m. UTC | #3
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>
  
Sam James Nov. 6, 2022, 9:26 p.m. UTC | #4
> 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
  
Florian Weimer Nov. 7, 2022, 4:10 p.m. UTC | #5
* 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
  
Sam James Nov. 8, 2022, 12:05 a.m. UTC | #6
> 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
  

Patch

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