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

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

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;
     }

> +      /* 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.

> +_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.


> +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 
:-).