Re: [PATCH v4 2/3] posix: Add pidfd_fork

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

On 22/05/23 07:12, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> +@deftypefun int pidfd_fork (unsigned int @var{flags})
>> +@standards{GNU, sys/pidfd.h}
>> +@safety{@prelim{}@mtsafe{}@assafe{}@acsafe{}}
>> +The @code{fork} function is similar to @code{fork} but return a file
>> +descriptor instead of process ID.
>> +
>> +If the operation is sucessful, there are both parent and child processes
>> +and both see @code{pidfd_fork} return, but with different values: it return
>> +a value of @code{0} in the child process and returns a file descriptor to
>> +child's process int the parent process.  Although the value of @code{0} clashes
>> +with @code{STDIN_FILENO}, it also assumed that stardand input is always
>> +available so a conformant application will never see a the @code{0} stream
>> +being returned to parent as a valid file descriptor.
> 
> I think this is a bit ugly.  Maybe pidfd_fork should return the PID/zero
> as fork, and store the FD through an int * argument?

Alright, I was not really sure about whether to expose the PID but I tend
to agree that for error code handling is not straightforward. I will change
to return the pid_t as fork and the pidfd as first argument.

> 
> The documentaiton should discuss if SIGCHLD is sent for such processes,
> and if the PID can be used in waitpid etc.  Ideally we'd have test cases
> for this behavior, too. 8-)

The default pidfd_fork should be similar semantically to fork, the first
version I make it similar to _Fork but I think this was a mistake.  This
included SIGCHLD handling as well (the flag is passed on clone syscall
as fork).  I will update the documentation and add extra tests.

> 
> I think it would be valuable to support creating subprocesses without
> ever sending SIGCHLD on their termination.  Not sure if this should be
> the default behavior, or something enabled with a flag.

I tend to agree with Christian and this should be behind a flags.  I
will add a flag for this.

> 
>> diff --git a/posix/fork-internal.c b/posix/fork-internal.c
>> new file mode 100644
>> index 0000000000..be0d3219af
>> --- /dev/null
>> +++ b/posix/fork-internal.c
> 
>> +void
>> +__fork_pos (int state, uint64_t lastrun, bool multiple_threads,
>> +	    struct nss_database_data *nss_database_data)
> 
> Maybe __fork_post, with a t?

Ack.

> 
>> diff --git a/posix/fork-internal.h b/posix/fork-internal.h
>> new file mode 100644
>> index 0000000000..3814d851c4
>> --- /dev/null
>> +++ b/posix/fork-internal.h
> 
>> +uint64_t __fork_pre (bool, struct nss_database_data *) attribute_hidden;
>> +void __fork_pos (int, uint64_t, bool, struct nss_database_data *)
>> +  attribute_hidden;
> 
> I suggest to put the arguments into a struct, with struct
> nss_database_data as a non-pointer member.  It makes it easier to
> document the meaning of the arguments.  And we may have to add more
> things in the future.

Ack.

> 
>> diff --git a/sysdeps/unix/sysv/linux/pidfd_fork.c b/sysdeps/unix/sysv/linux/pidfd_fork.c
>> new file mode 100644
>> index 0000000000..a3ef48ce77
>> --- /dev/null
>> +++ b/sysdeps/unix/sysv/linux/pidfd_fork.c
> 
>> +static int
>> +forkfd (void)
>> +{
>> +  /* The kernel should not change the pidfd for the child, and the returning
>> +     value on the parent is always larger than 0 (since a new file descriptor
>> +     is always opened with CLONE_PIDFD is the kernel supports it.  */
> 
> Missing ).

Ack.