[PATCH 3/3] posix: Add posix_spawn_file_actions_closefrom

[Adhemerval Zanella]

On 22/12/2020 09:00, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>>>> diff --git a/posix/spawn.h b/posix/spawn.h
>>>> index be6bd591a3..21ea563425 100644
>>>> --- a/posix/spawn.h
>>>> +++ b/posix/spawn.h
>>>> @@ -213,6 +213,13 @@ extern int posix_spawn_file_actions_addchdir_np (posix_spawn_file_actions_t *
>>>>  extern int posix_spawn_file_actions_addfchdir_np (posix_spawn_file_actions_t *,
>>>>  						  int __fd)
>>>>       __THROW __nonnull ((1));
>>>> +
>>>> +/* Add an action to close all file descriptor greater than FROM during
>>>> +   spawn.  This affects the subsequent file actions.  */
>>>> +extern int posix_spawn_file_actions_addclosefrom_np (posix_spawn_file_actions_t *,
>>>> +						     int __from)
>>>> +     __THROW __nonnull ((1));
>>>> +
>>>>  #endif
>>>
>>> Line length issue (but I'm not sure how to avoid that).
>>
>> Maybe move the argument to next line aligning with __THROW?
>>
>> extern int posix_spawn_file_actions_addclosefrom_np (
>>      posix_spawn_file_actions_t *, int __from)
>>      __THROW __nonnull ((1));
>>
>> ?
> 
> I think it's not GNU style.  You could drop the extern, though.

Ack.

> 
>>> You could simplify the reinvocation logic by making this a static or a
>>> container test.
>>
>> I prefer to keep this exercises this test on some platform where container
>> tests are not really supported.
> 
> Then perhaps make it a static test, or only make it a container test for
> the non-hard-coded-paths case?

Maybe another option would to simplify this setup and add a libsupport
to handle the argument and re-invocation logic.  Meanwhile, I think
issuing the shared case still stresses the most common usage for glibc.

> 
>>>> diff --git a/sysdeps/unix/sysv/linux/spawni.c b/sysdeps/unix/sysv/linux/spawni.c
>>>> index f157bfffd2..f496578d19 100644
>>>> --- a/sysdeps/unix/sysv/linux/spawni.c
>>>> +++ b/sysdeps/unix/sysv/linux/spawni.c
>>>> @@ -16,22 +16,17 @@
>>>>     License along with the GNU C Library; if not, see
>>>>     <https://www.gnu.org/licenses/>.  */
>>>>  
>>>> -#include <spawn.h>
>>>> -#include <fcntl.h>
>>>> -#include <paths.h>
>>>> -#include <string.h>
>>>> -#include <sys/resource.h>
>>>> -#include <sys/wait.h>
>>>> -#include <sys/param.h>
>>>> -#include <sys/mman.h>
>>>> -#include <not-cancel.h>
>>>> +#include <arch-fd_to_filename.h>
>>>> +#include <internal-signals.h>
>>>> +#include <ldsodefs.h>
>>>>  #include <local-setxid.h>
>>>> +#include <not-cancel.h>
>>>> +#include <paths.h>
>>>>  #include <shlib-compat.h>
>>>> -#include <nptl/pthreadP.h>
>>>> -#include <dl-sysdep.h>
>>>> -#include <libc-pointer-arith.h>
>>>> -#include <ldsodefs.h>
>>>> -#include "spawn_int.h"
>>>> +#include <spawn.h>
>>>> +#include <spawn_int.h>
>>>> +#include <sysdep.h>
>>>> +#include <sys/resource.h>
>>>
>>> Suprious changes?
>>
>> In fact I cleanup the include range a bit, if you prefer I can remove this
>> change from this set.
> 
> Hmm.  I think <arch-fd_to_filename.h> is not used here?

Not anymore indeed, I will remove it.