Re: supporting terminal ownership assignment (tcsetpgrp()) in posix_spawn

[Adhemerval Zanella <adhemerval.zanella@linaro.org>]

On 08/06/2021 19:11, Godmar Back wrote:
> On Tue, Jun 8, 2021 at 12:42 PM Adhemerval Zanella <
> adhemerval.zanella@linaro.org> wrote:
> 
>>>
>>>
>>>>
>>>> If the caller already has group it want to use, it can issue instead:
>>>>
>>>>   posix_spawnattr_t attr;
>>>>   posix_spawnattr_setpgroup (&attr, groupid);
>>>>   posix_spawnattr_tcsetpgrp_np (&attr, fd, groupid);
>>>>
>>>> Which in turn will make the created process to issue:
>>>>
>>>>   setpgid (0, groupid);
>>>>   tcsetpgrp (fd, groupid);
>>>>
>>>
>>> For this use case, as long as it supports groupid == 0, this should work
>> as
>>> it is what shells currently do.
>>
>> So the my question is whether providing the groupid as an argument is
>> really required (I would say yes so it can be combined with
>> posix_spawnattr_setpgroup).
>>
> 
> The Blackberry API does not pass an argument (as a point of reference).
> In practice, when spawning a pipeline like a | b | c ... we pass group id 0
> to the
> first child, and then the pid if of the first child as the pgrp id to the
> second and
> third. This means that if the groupid is omitted, it needs to refer to the
> group id that was given to POSIX_SPAWN_SETPGROUP, and thus
> POSIX_SPAWN_SETPGROUP is a prerequisite for it.
> 

The libc interface should be as generic possible to cover most users cases,
I really don't want to add a posix_spawnattr_tcsetpgrp_np that uses the 
groupid implicit from POSIX_SPAWN_SETPGROUP to someone ask for a
posix_spawnattr_tcsetpgrp_np_ex so one can set the groupid.

Also, requiring POSIX_SPAWN_SETPGROUP for posix_spawnattr_tcsetpgrp_np is
not really a good API, it adds subtle semantics (should it use a default
value set by posix_spawnattr_init or should we fail with EINVAL), and adds
complexity in the error path (we will need to either pre validate the
posix_spawnattr_t input before start the process creation or handle a
possible invalid combination on the helper process itself).

That's why I am more inclined to follow the tcsetpgrp on the posix_spawn
extension and let the caller set the required groups.

> 
>>
>> Another question is when to issue the tcsetpgrp related to
>> POSIX_SPAWN_SETSID. I would say tcsetpgrp should be issued *before*
>> setsid, so tcsetpgrp can return early if it fails. Otherwise tcsetpgrp
>> will always fail if POSIX_SPAWN_SETSID is set (it would be a caller
>> error, but I think from API viewpoint it should be better if we could
>> minimize the possible error scenarios).
>>
> 
> I haven't checked how the current implementation works. Do you use a pipe
> or something
> for the child to report back if something went wrong, or does the parent
> somehow check if the
> asked-for actions in the child will likely succeed?  If the former, doing
> setsid first, and then,
> if given, doing the tcsetpgrp call will fail as you say and this failure
> would be reported back
> to the caller. Then they know they've misused the API.
> 
> If you do tcsetpgrp first and then setsid, the tcsetpgrp will succeed, but
> will be ineffective, and
> the caller won't ever know.
> 

As Florian has said we use a helper process created with CLONE_VM and
CLONE_VFORK, meaning that it will share the memory with its parent and 
CLONE_VFORK avoids any potential race condition.

And you raised a good point, I agree we should not hide the possible misuse
from the caller.