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

[Florian Weimer <fweimer@redhat.com>]

* 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?

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-)

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.

> 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?

> 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.

> 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 ).

Thanks,
Florian