[02/10] linux: Simplify opendir buffer allocation

  THe fallback allocation is removed, so the possible size constraint
should be analized just once; __alloc_dir assumes that 'statp'
argument is non-null, and the max_buffer_size move to close its
used.

Checked on x86_64-linux-gnu and i686-linux-gnu.
---
 include/dirent.h                  |  3 +-
 sysdeps/unix/sysv/linux/opendir.c | 52 +++++++++++--------------------
 2 files changed, 21 insertions(+), 34 deletions(-)
  

* Adhemerval Zanella via Libc-alpha:

> THe fallback allocation is removed, so the possible size constraint
> should be analized just once; __alloc_dir assumes that 'statp'
> argument is non-null, and the max_buffer_size move to close its
> used.

Type: analized

> +    return NULL;
> +
> +  /* The st_blksize value of the directory is used as a hint for the
> +     size of the buffer which receives struct dirent values from the
> +     kernel.  st_blksize is limited to max_buffer_size, in case the
> +     file system provides a bogus value.  */
> +  enum { max_buffer_size = 1U << 20 };
> +
> +  const size_t allocation_size = 4 * BUFSIZ;
> +  _Static_assert (allocation_size >= sizeof (struct dirent64),
> +		  "allocation_size < sizeof (struct dirent64)");
> +
>    /* Increase allocation if requested, but not if the value appears to
> +     be bogus.  It will be between 32Kb (for blocksizes smaller than BUFSIZ)
> +     up to 1Mb.  */
> +  size_t allocation = MIN (MAX ((size_t) statp->st_blksize, allocation_size),
> +			   max_buffer_size);
>  
>    DIR *dirp = (DIR *) malloc (sizeof (DIR) + allocation);
>    if (dirp == NULL)
>      {
> +      if (close_fd)
> +	__close_nocancel_nostatus (fd);
> +      return NULL;
>      }
>  
>    dirp->fd = fd;

This looks okay to me.  I wonder if we should just use 32 KiB
uncodintionally, though?
  

21.04.2020 12:28 Florian Weimer <fw@deneb.enyo.de> wrote:
> 
> * Adhemerval Zanella via Libc-alpha:
> 
> > THe fallback allocation is removed, so the possible size constraint
> > should be analized just once; __alloc_dir assumes that 'statp'
> > argument is non-null, and the max_buffer_size move to close its
> > used.
> 
> Type: analized

True, also (which is maybe obvious but maybe not) "THe" -> "The".

Regards,

Rafal
  

On 21/04/2020 07:28, Florian Weimer wrote:
> * Adhemerval Zanella via Libc-alpha:
> 
>> THe fallback allocation is removed, so the possible size constraint
>> should be analized just once; __alloc_dir assumes that 'statp'
>> argument is non-null, and the max_buffer_size move to close its
>> used.
> 
> Type: analized
> 
>> +    return NULL;
>> +
>> +  /* The st_blksize value of the directory is used as a hint for the
>> +     size of the buffer which receives struct dirent values from the
>> +     kernel.  st_blksize is limited to max_buffer_size, in case the
>> +     file system provides a bogus value.  */
>> +  enum { max_buffer_size = 1U << 20 };
>> +
>> +  const size_t allocation_size = 4 * BUFSIZ;
>> +  _Static_assert (allocation_size >= sizeof (struct dirent64),
>> +		  "allocation_size < sizeof (struct dirent64)");
>> +
>>    /* Increase allocation if requested, but not if the value appears to
>> +     be bogus.  It will be between 32Kb (for blocksizes smaller than BUFSIZ)
>> +     up to 1Mb.  */
>> +  size_t allocation = MIN (MAX ((size_t) statp->st_blksize, allocation_size),
>> +			   max_buffer_size);
>>  
>>    DIR *dirp = (DIR *) malloc (sizeof (DIR) + allocation);
>>    if (dirp == NULL)
>>      {
>> +      if (close_fd)
>> +	__close_nocancel_nostatus (fd);
>> +      return NULL;
>>      }
>>  
>>    dirp->fd = fd;
> 
> This looks okay to me.  I wonder if we should just use 32 KiB
> uncodintionally, though?

Do you mean instead of '4 * BUFSIZ'?
  

* Adhemerval Zanella via Libc-alpha:

>> This looks okay to me.  I wonder if we should just use 32 KiB
>> uncodintionally, though?
>
> Do you mean instead of '4 * BUFSIZ'?

Yes.  I assumed BUFSIZ was 8192.
  

On 24/04/2020 07:11, Florian Weimer wrote:
> * Adhemerval Zanella via Libc-alpha:
> 
>>> This looks okay to me.  I wonder if we should just use 32 KiB
>>> uncodintionally, though?
>>
>> Do you mean instead of '4 * BUFSIZ'?
> 
> Yes.  I assumed BUFSIZ was 8192.
> 

Alright, I can make this change. Any opinion whether we should keep
32 KiB or increase/decrease it?
  

* Adhemerval Zanella:

> On 24/04/2020 07:11, Florian Weimer wrote:
>> * Adhemerval Zanella via Libc-alpha:
>> 
>>>> This looks okay to me.  I wonder if we should just use 32 KiB
>>>> uncodintionally, though?
>>>
>>> Do you mean instead of '4 * BUFSIZ'?
>> 
>> Yes.  I assumed BUFSIZ was 8192.
>> 
>
> Alright, I can make this change. Any opinion whether we should keep
> 32 KiB or increase/decrease it?

Sorry, I think it should be a separate change.  Lowering the buffer
size will unfortunately expose application bugs when applications
modify the directory while iterating through it using readdir.  (musl
sees more such bugs due to its smaller buffer size.)