Re: [glibc] posix: Add terminal control setting support for posix_spawn

Re: [glibc] posix: Add terminal control setting support for posix_spawn

[Florian Weimer]

* Adhemerval Zanella via Glibc-cvs:

> commit 342cc934a3bf74ac618e2318d738f22ac93257ba
> Author: Adhemerval Zanella <adhemerval.zanella@linaro.org>
> Date:   Mon Jun 14 14:41:31 2021 -0300
>
>     posix: Add terminal control setting support for posix_spawn
>     
>     Currently there is no proper way to set the controlling terminal through
>     posix_spawn in race free manner [1].  This forces shell implementations
>     to keep using fork+exec when launching background process groups,
>     even when using posix_spawn yields better performance.
>     
>     This patch adds a new GNU extension so the creating process can
>     configure the created process terminal group.  This is done with a new
>     flag, POSIX_SPAWN_TCSETPGROUP, along with two new attribute functions:
>     posix_spawnattr_tcsetpgrp_np, and posix_spawnattr_tcgetpgrp_np.
>     The function sets a new attribute, spawn-tcgroupfd, that references to
>     the controlling terminal.
>     
>     The controlling terminal is set after the spawn-pgroup attribute, and
>     uses the spawn-tcgroupfd along with current creating process group
>     (so it is composable with POSIX_SPAWN_SETPGROUP).
>     
>     To create a process and set the controlling terminal, one can use the
>     following sequence:
>     
>         posix_spawnattr_t attr;
>         posix_spawnattr_init (&attr);
>         posix_spawnattr_setflags (&attr, POSIX_SPAWN_TCSETPGROUP);
>         posix_spawnattr_tcsetpgrp_np (&attr, tcfd);
>     
>     If the idea is also to create a new process groups:
>     
>         posix_spawnattr_t attr;
>         posix_spawnattr_init (&attr);
>         posix_spawnattr_setflags (&attr, POSIX_SPAWN_TCSETPGROUP
>                                          | POSIX_SPAWN_SETPGROUP);
>         posix_spawnattr_tcsetpgrp_np (&attr, tcfd);
>         posix_spawnattr_setpgroup (&attr, 0);

Sorry, I did not look at this in detail so far.  A file descriptor
outside the file actions API makes me quite nervous.

Advanced Programming in the UNIX Environment says (like POSIX), “If the
process has a controlling terminal, the process can call tcsetpgrp to
set the foreground process group ID […]”.  This suggests to me that a
file action may be required to establish a controlling terminal.
Callers of posix_spawn might restrict that to the spawned process.  As
far as I know, Linux follows SVR4, and opening the terminal device under
/dev/pts/ in a session leader that has no controlling terminal
establishes one.  This translate to a posix_spawn file action.  So the
action established by posix_spawnattr_tcsetpgrp_np must come after the
file actions.

But the descriptor used by posix_spawnattr_tcsetpgrp_np must still be
open.  So it would have to come before a closefrom file action (or a
regular close).

I think this means that the action established by
posix_spawnattr_tcsetpgrp_np needs to be sequenced among the file
actions, and needs to be a file action itself.

Thanks,
Florian

Re: [glibc] posix: Add terminal control setting support for posix_spawn

[Adhemerval Zanella]

On 27/01/2022 07:49, Florian Weimer via Libc-alpha wrote:
> * Adhemerval Zanella via Glibc-cvs:
> 
>> commit 342cc934a3bf74ac618e2318d738f22ac93257ba
>> Author: Adhemerval Zanella <adhemerval.zanella@linaro.org>
>> Date:   Mon Jun 14 14:41:31 2021 -0300
>>
>>     posix: Add terminal control setting support for posix_spawn
>>     
>>     Currently there is no proper way to set the controlling terminal through
>>     posix_spawn in race free manner [1].  This forces shell implementations
>>     to keep using fork+exec when launching background process groups,
>>     even when using posix_spawn yields better performance.
>>     
>>     This patch adds a new GNU extension so the creating process can
>>     configure the created process terminal group.  This is done with a new
>>     flag, POSIX_SPAWN_TCSETPGROUP, along with two new attribute functions:
>>     posix_spawnattr_tcsetpgrp_np, and posix_spawnattr_tcgetpgrp_np.
>>     The function sets a new attribute, spawn-tcgroupfd, that references to
>>     the controlling terminal.
>>     
>>     The controlling terminal is set after the spawn-pgroup attribute, and
>>     uses the spawn-tcgroupfd along with current creating process group
>>     (so it is composable with POSIX_SPAWN_SETPGROUP).
>>     
>>     To create a process and set the controlling terminal, one can use the
>>     following sequence:
>>     
>>         posix_spawnattr_t attr;
>>         posix_spawnattr_init (&attr);
>>         posix_spawnattr_setflags (&attr, POSIX_SPAWN_TCSETPGROUP);
>>         posix_spawnattr_tcsetpgrp_np (&attr, tcfd);
>>     
>>     If the idea is also to create a new process groups:
>>     
>>         posix_spawnattr_t attr;
>>         posix_spawnattr_init (&attr);
>>         posix_spawnattr_setflags (&attr, POSIX_SPAWN_TCSETPGROUP
>>                                          | POSIX_SPAWN_SETPGROUP);
>>         posix_spawnattr_tcsetpgrp_np (&attr, tcfd);
>>         posix_spawnattr_setpgroup (&attr, 0);
> 
> Sorry, I did not look at this in detail so far.  A file descriptor
> outside the file actions API makes me quite nervous.
> 
> Advanced Programming in the UNIX Environment says (like POSIX), “If the
> process has a controlling terminal, the process can call tcsetpgrp to
> set the foreground process group ID […]”.  This suggests to me that a
> file action may be required to establish a controlling terminal.
> Callers of posix_spawn might restrict that to the spawned process.  As
> far as I know, Linux follows SVR4, and opening the terminal device under
> /dev/pts/ in a session leader that has no controlling terminal
> establishes one.  This translate to a posix_spawn file action.  So the
> action established by posix_spawnattr_tcsetpgrp_np must come after the
> file actions.
> 
> But the descriptor used by posix_spawnattr_tcsetpgrp_np must still be
> open.  So it would have to come before a closefrom file action (or a
> regular close).
> 
> I think this means that the action established by
> posix_spawnattr_tcsetpgrp_np needs to be sequenced among the file
> actions, and needs to be a file action itself.
> 
> Thanks,
> Florian
> 


I did not considered the closefrom in conjunction with closefrom file
action and making a file action does make sense indeed.  Maybe a better
approach would be to remove posix_spawnattr_tcsetpgrp_np and
posix_spawnattr_tcsetpgrp_np and add:

  int posix_spawn_file_actions_tcsetgrp_np (posix_spawn_file_actions_t *,
					    int fd);

Similar to posix_spawn_file_actions_addfchdir_np.

So to create a process and set the controlling terminal, one can use the
following sequence:

  int tcfd = open (_PATH_TTY, O_RDONLY, 0600);

  posix_spawnattr_t attr;
  posix_spawnattr_init (&attr);
  posix_spawnattr_setflags (&attr, POSIX_SPAWN_TCSETPGROUP);
  
  posix_spawn_file_actions_t actions;
  posix_spawn_file_actions_init (&actions);
  posix_spawn_file_actions_tcsetgrp_np (&actions, tcfd);

So users would be able to add a posix_spawn_file_actions_addclose or
posix_spawn_file_actions_addclosefrom_np. 

Re: [glibc] posix: Add terminal control setting support for posix_spawn

[Florian Weimer]

* Adhemerval Zanella:

> I did not considered the closefrom in conjunction with closefrom file
> action and making a file action does make sense indeed.  Maybe a better
> approach would be to remove posix_spawnattr_tcsetpgrp_np and
> posix_spawnattr_tcsetpgrp_np and add:
>
>   int posix_spawn_file_actions_tcsetgrp_np (posix_spawn_file_actions_t *,
> 					    int fd);
>
> Similar to posix_spawn_file_actions_addfchdir_np.
>
> So to create a process and set the controlling terminal, one can use the
> following sequence:
>
>   int tcfd = open (_PATH_TTY, O_RDONLY, 0600);
>
>   posix_spawnattr_t attr;
>   posix_spawnattr_init (&attr);
>   posix_spawnattr_setflags (&attr, POSIX_SPAWN_TCSETPGROUP);

Why is POSIX_SPAWN_TCSETPGROUP needed?

>   posix_spawn_file_actions_t actions;
>   posix_spawn_file_actions_init (&actions);
>   posix_spawn_file_actions_tcsetgrp_np (&actions, tcfd);
>
> So users would be able to add a posix_spawn_file_actions_addclose or
> posix_spawn_file_actions_addclosefrom_np. 

This would address my concern, yes.

Thanks,
Florian

Re: [glibc] posix: Add terminal control setting support for posix_spawn

[Adhemerval Zanella]

On 27/01/2022 08:57, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> I did not considered the closefrom in conjunction with closefrom file
>> action and making a file action does make sense indeed.  Maybe a better
>> approach would be to remove posix_spawnattr_tcsetpgrp_np and
>> posix_spawnattr_tcsetpgrp_np and add:
>>
>>   int posix_spawn_file_actions_tcsetgrp_np (posix_spawn_file_actions_t *,
>> 					    int fd);
>>
>> Similar to posix_spawn_file_actions_addfchdir_np.
>>
>> So to create a process and set the controlling terminal, one can use the
>> following sequence:
>>
>>   int tcfd = open (_PATH_TTY, O_RDONLY, 0600);
>>
>>   posix_spawnattr_t attr;
>>   posix_spawnattr_init (&attr);
>>   posix_spawnattr_setflags (&attr, POSIX_SPAWN_TCSETPGROUP);
> 
> Why is POSIX_SPAWN_TCSETPGROUP needed?
The idea would so caller can check if the action would take, but it does not
make sense with a file action.

> 
>>   posix_spawn_file_actions_t actions;
>>   posix_spawn_file_actions_init (&actions);
>>   posix_spawn_file_actions_tcsetgrp_np (&actions, tcfd);

In fact I think it should be posix_spawn_file_actions_addtcsetgrp_np
to follow other file action naming convention.

>>
>> So users would be able to add a posix_spawn_file_actions_addclose or
>> posix_spawn_file_actions_addclosefrom_np. 
> 
> This would address my concern, yes.
> 
> Thanks,
> Florian
> 

Re: [glibc] posix: Add terminal control setting support for posix_spawn

[Florian Weimer]

* Adhemerval Zanella:

>>>   posix_spawn_file_actions_t actions;
>>>   posix_spawn_file_actions_init (&actions);
>>>   posix_spawn_file_actions_tcsetgrp_np (&actions, tcfd);
>
> In fact I think it should be posix_spawn_file_actions_addtcsetgrp_np
> to follow other file action naming convention.

Yes, that name is better.

Thanks,
Florian