Re: [PATCH] linux: Implement tmpfile with O_TMPFILE (BZ#21530)

[Carlos O'Donell <carlos at redhat dot com>]

On 08/31/2017 03:34 PM, Adhemerval Zanella wrote:
> This patch adds support to use O_TMPFILE on tmpfile on Linux.  This is
> similar previous suggestion by Andreas Schwab [1] with the difference
> the file descriptor creation is parameterized to compartmentalize Linux
> only open flags (O_TMPFILE) on sysdep folder.
> 
> Checked on x86_64-linux-gnu.
> 
> 	Adhemerval Zanella  <adhemerval.zanella@linaro.org>
> 	Andreas Schwab  <schwab@suse.de>
> 
> 	[BZ #21530]
> 	* include/stdio.h (__gen_tempfd): New function.
> 	* stdio-common/Makefile (routines): Add gentempfd.
> 	* stdio-common/gentempfd.c: New file.
> 	* sysdeps/unix/sysv/linux/gentempfd.c: Likewise.
> 	* stdio-common/tmpfile.c (tmpfile): First try to use a system specific
> 	unnamed file first.

Looks good to me.

> [1] https://sourceware.org/ml/libc-alpha/2017-06/msg01293.html
> ---
>  ChangeLog                           | 11 +++++++++++
>  include/stdio.h                     |  3 +++
>  stdio-common/Makefile               |  2 +-
>  stdio-common/gentempfd.c            | 26 ++++++++++++++++++++++++++
>  stdio-common/tmpfile.c              | 29 ++++++++++++++++++++---------
>  sysdeps/unix/sysv/linux/gentempfd.c | 34 ++++++++++++++++++++++++++++++++++
>  6 files changed, 95 insertions(+), 10 deletions(-)
>  create mode 100644 stdio-common/gentempfd.c
>  create mode 100644 sysdeps/unix/sysv/linux/gentempfd.c
> 
> diff --git a/include/stdio.h b/include/stdio.h
> index 509447c..87e0e10 100644
> --- a/include/stdio.h
> +++ b/include/stdio.h
> @@ -191,5 +191,8 @@ libc_hidden_proto (__obstack_vprintf_chk)
>  extern FILE * __fmemopen (void *buf, size_t len, const char *mode);
>  libc_hidden_proto (__fmemopen)
>  
> +extern int __gen_tempfd (int flags);
> +libc_hidden_proto (__gen_tempfd)

OK.

> +
>  # endif /* not _ISOMAC */
>  #endif /* stdio.h */
> diff --git a/stdio-common/Makefile b/stdio-common/Makefile
> index 397e0c2..2c3c2e5 100644
> --- a/stdio-common/Makefile
> +++ b/stdio-common/Makefile
> @@ -39,7 +39,7 @@ routines	:=							      \
>  	flockfile ftrylockfile funlockfile				      \
>  	isoc99_scanf isoc99_vscanf isoc99_fscanf isoc99_vfscanf isoc99_sscanf \
>  	isoc99_vsscanf							      \
> -	psiginfo
> +	psiginfo gentempfd

OK.

>  
>  aux	:= errlist siglist printf-parsemb printf-parsewc fxprintf
>  
> diff --git a/stdio-common/gentempfd.c b/stdio-common/gentempfd.c
> new file mode 100644
> index 0000000..d40c57d
> --- /dev/null
> +++ b/stdio-common/gentempfd.c
> @@ -0,0 +1,26 @@
> +/* Generate a temporary file descriptor.  Generic/POSIX version.
> +   Copyright (C) 2017 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +#include <stdio.h>
> +
> +int
> +__gen_tempfd (int flags)
> +{
> +  return -1;
> +}

OK.

> +libc_hidden_def (__gen_tempfd)
> diff --git a/stdio-common/tmpfile.c b/stdio-common/tmpfile.c
> index e6030be..3e35345 100644
> --- a/stdio-common/tmpfile.c
> +++ b/stdio-common/tmpfile.c
> @@ -34,23 +34,34 @@
>  FILE *
>  tmpfile (void)
>  {
> -  char buf[FILENAME_MAX];
>    int fd;
>    FILE *f;
> -
> -  if (__path_search (buf, FILENAME_MAX, NULL, "tmpf", 0))
> -    return NULL;
>    int flags = 0;
>  #ifdef FLAGS
>    flags = FLAGS;
>  #endif
> -  fd = __gen_tempname (buf, 0, flags, __GT_FILE);
> +
> +  /* First try a system specific method.  */
> +  fd = __gen_tempfd (flags);
> +

OK.

>    if (fd < 0)
> -    return NULL;
> +    {
> +      char buf[FILENAME_MAX];
>  
> -  /* Note that this relies on the Unix semantics that
> -     a file is not really removed until it is closed.  */
> -  (void) __unlink (buf);
> +      if (__path_search (buf, sizeof buf, NULL, "tmpf", 0))
> +	return NULL;
> +
> +      fd = __gen_tempname (buf, 0, flags, __GT_FILE);
> +      if (fd < 0)
> +	return NULL;
> +
> +      /* Note that this relies on the Unix semantics that
> +	 a file is not really removed until it is closed.  */
> +      (void) __unlink (buf);

OK.

> +    }
> +
> +  if (fd < 0)
> +    return NULL;
>  
>    if ((f = __fdopen (fd, "w+b")) == NULL)
>      __close (fd);
> diff --git a/sysdeps/unix/sysv/linux/gentempfd.c b/sysdeps/unix/sysv/linux/gentempfd.c
> new file mode 100644
> index 0000000..902cbe2
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/gentempfd.c
> @@ -0,0 +1,34 @@
> +/* Generate a temporary file descriptor.  Linux version.
> +   Copyright (C) 2017 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +#include <stdio.h>
> +#include <fcntl.h>
> +#include <errno.h>
> +
> +int
> +__gen_tempfd (int flags)
> +{
> +  int fd = __open (P_tmpdir, O_RDWR | O_TMPFILE | O_EXCL | flags,

OK.

I assume we don't care that on < 3.11 this will consistently fail,
or < 3.15 (XFS) or < 3.16 (BTRFS).

We might have remembered that failure and avoided the open(), but
then again one open() is not that costly in a long sequence of syscalls
that are going to create a temporary file.

> +		   S_IRUSR | S_IWUSR);
> +  if (fd < 0 && errno == ENOENT && strcmp (P_tmpdir, "/tmp") != 0)
> +    fd = __open ("/tmp", O_RDWR | O_TMPFILE | O_EXCL | flags,
> +		 S_IRUSR | S_IWUSR);
> +
> +  return fd;
> +}
> +libc_hidden_def (__gen_tempfd)
> 


-- 
Cheers,
Carlos.