Re: [PATCH v2] libio: Flush stream at freopen (BZ#21037)

[Paul Eggert <eggert@cs.ucla.edu>]

On 06/14/2018 08:01 AM, Adhemerval Zanella wrote:
> + char fdfilename[30]; 

The magic number 30 should be turned into a named constant defined in 
fd_to_filename.h, to help prevent future mistakes. Once that is done, 
you can change the signature of fd_to_filename to not pass the size, and 
to require the caller to pass an array of at least size 30, so that 
fd_to_filename need not check for buffer overflow (see below for more on 
this).

> +  const char *gfilename;
> +  if (filename == NULL && fd >= 0)
> +    gfilename = fd_to_filename (fd, fdfilename, sizeof fdfilename)
> +		? fdfilename : NULL;
> +  else
> +    gfilename = filename;

Cleaner would be:

   const char *gfilename
     = filename != NULL ? filename : fd_to_filename (fd, fdfilename);

That is, let fd_to_filename worry about what to do with negative fd, and 
have it return fdfilename or NULL, and don't pass the size (which should 
be that magic number regardless).


> -static inline const char *
> -fd_to_filename (int fd)
> +static inline bool
> +fd_to_filename (int fd, char *buf, size_t len)
>   {
> -  char *ret = malloc (30);
> +  __snprintf (buf, len, "/proc/self/fd/%d", fd);
>   
> -  if (ret != NULL)
> -    {
> -      struct stat64 st;
> -
> -      *_fitoa_word (fd, __stpcpy (ret, "/proc/self/fd/"), 10, 0) = '\0';
> -
> -      /* We must make sure the file exists.  */
> -      if (__lxstat64 (_STAT_VER, ret, &st) < 0)
> -	{
> -	  /* /proc is not mounted or something else happened.  Don't
> -	     return the file name.  */
> -	  free (ret);
> -	  ret = NULL;
> -	}
> -    }
> -  return ret;
> +  /* We must make sure the file exists.  */
> +  if (__lxstat64 (_STAT_VER, buf, & (struct stat64) {}) < 0)
> +    /* /proc is not mounted or something else happened.  */
> +    return false;
> +  return true;
>   }

The __snprintf would be quite wrong if the string did not fit. Again, I 
suggest simply requiring the buffer to be long enough and not checking 
its length, and sticking with stpcpy + _fitoa_word which should be more 
efficient than __snprintf anyway (or if you prefer simplicity to speed, 
just use sprintf).

The '& (struct stat64) {}' construct looks pretty but is less efficient 
as it makes the compiler zero out the structure unnecessarily, so the 
code should keep doing that struct the old-fashioned way.