Re: [PATCH 1/2] util: Introduce set_cloexec()

[Stefano Brivio <sbrivio@redhat.com>]

On Tue,  6 Jun 2023 13:41:29 +0200
Michal Privoznik <mprivozn@redhat.com> wrote:

> This function is going to be used on FDs inherited from parent.
> Parent can't set O_CLOEXEC obviously, so we have to set it
> ourselves.
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  util.c | 17 +++++++++++++++++
>  util.h |  1 +
>  2 files changed, 18 insertions(+)
> 
> diff --git a/util.c b/util.c
> index 1d00404..9817c74 100644
> --- a/util.c
> +++ b/util.c
> @@ -526,6 +526,23 @@ int write_file(const char *path, const char *buf)
>  	return len == 0 ? 0 : -1;
>  }
>  
> +/**
> + * set_cloexec() - Set Close-on-exec flag on given FD
> + * @fd:		FD to set the flag on

Nits: "close-on-exec", "file descriptor".

> + *
> + * Return: 0 on success, -1 on any error

I'd rather return errno as set by fcntl(), for consistency.

> + */
> +int set_cloexec(int fd)
> +{
> +	int fflags;
> +	if ((fflags = fcntl(fd, F_GETFD)) < 0)
> +		return -1;
> +	fflags |= FD_CLOEXEC;
> +	if ((fcntl(fd, F_SETFD, fflags)) < 0)
> +		return -1;
> +	return 0;
> +}

...but in general, I'm not convinced this is really needed. O_CLOEXEC
has been the only file _descriptor_ flag for 16 years, and I have some
doubts another one will ever come up (and at that point, we wouldn't
probably want to have it set by default anyway).

I'd just set it with a direct call. This is not a strong preference
though.

-- 
Stefano