[PATCH v6 3/3] linux: Set internal DIR filepos as off64_t (BZ #23960, BZ #24050)

[Adhemerval Zanella Netto]

On 10/03/23 18:41, Paul Eggert wrote:
> On 2023-03-02 06:57, Adhemerval Zanella wrote:
>> +      for (i = 0; i < dirstream_loc_size (&dirp->locs); i++)
>> +    if (*dirstream_loc_at (&dirp->locs, i) == dirp->filepos)
>> +      break;
>> +      /* It should be pre-allocated on readdir.  */
>> +      assert (i != dirstream_loc_size (&dirp->locs));
> 
> This should be something like the following, to avoid unnecessary work when assertions are disabled:
> 
>   for (long int i = 0; ; i++)
>     {
>       assert (i < dirstream_loc_size (&dirp->locs));
>       if (*dirstream_loc_at (&dirp->locs, i) == dirp->filepos)
>     break;
>     }
> 

Ack, it works for me.


>> +      /* This assignment might overflow, however most likely ENOME would
>> +     happen long before.  */
>> +      dsp.p.info = i;
> 
> This doesn't sound right. The allocator should never create a table with more than LONG_MAX entries because the upper part of any such table would be useless. If that is done right, the assignment cannot overflow.

Indeed it does not make sense since it is based on malloc anyway. I will remove
the comment.

> 
>> +_Static_assert (sizeof (long int) == sizeof (off64_t),
>> +        "sizeof (long int) != sizeof (off64_t)");
> 
> This is confusing. First, we need require only that long int be at least as wide as off64_t; it doesn't have to be exactly the same width. Second, why both "==" and "!="? Third, why not use plain "static_assert" with one arg instead of the old-fashioned "_Static_assert" with two? We can support this form of static_assert on older compilers - see how Gnulib does it.

Indeed, we already set _DIRENT_OFFSET_TRANSLATION for (LONG_WIDTH < 64)
so maybe I think we should replace with 64 with sizeof (off_64) and
remove the static assert. 

> 
> 
>> +static __always_inline bool
>> +telldir_need_dirstream (__off64_t d_off)
>> +{
>> +  return d_off >= 1UL << 31;
>> +}
> 
> Safer would be '! (TYPE_MINIMUM (off_t) <= d_off && d_off <= TYPE_MAXIMUM (off_t))', in case d_off is negative (or off_t isn't 32-bit :-).

Ack.