Re: [PATCH v2 4/9] linux: Use getdents64 on non-LFS readdir

[Adhemerval Zanella via Libc-alpha <libc-alpha@sourceware.org>]

On 20/10/2020 09:35, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> On 20/10/2020 04:38, Florian Weimer wrote:
>>> * Adhemerval Zanella via Libc-alpha:
>>>
>>>> ENAMETOOLONG does make more sense, I have fixed it locally.  I am not
>>>> sure I understood by 'delayed', it it report on the readdir call, but
>>>> the next entry will still be reported in the next readdir call.  The
>>>> construction such as won't work:
>>>>
>>>>   while (readdir (dp) != NULL)
>>>>     {
>>>>       [...]
>>>>     }
>>>>
>>>> But this will work as intended:
>>>>
>>>>   while (1)
>>>>     {
>>>>       errno = 0;
>>>>       struct dirent *entry = readdir (dp);
>>>>       if (entry == NULL)
>>>> 	{
>>>> 	  if (errno == ENAMETOOLONG)
>>>> 	    continue;
>>>> 	  break;
>>>> 	}
>>>>     }
>>>
>>> I think it's unreasonable to expect that this pattern will work.  Itg
>>> assumes that readdir updates dp despite the error.  In other
>>> implementations, this may produce an endless loop.  This is why for
>>> readdir_r, we record the ENAMETOOLONG error as pending, and report it
>>> only at the end.  (We should do the same for EOVERFLOW errors.)
>>
>> This is what POSIX states as the way to check for errors:
>>
>>   "Applications wishing to check for error situations should set errno 
>>    to 0 before calling readdir(). If errno is set to non-zero on return, 
>>    an error occurred."
>>
>> What the standard is not clear is whether the position as the directory 
>> stream is updated in a case of failure.
> 
> Yes, that was my concern.  I found this in POSIX (in 2.3 Error Numbers):
> 
> | If an error condition is detected, the action requested may have been
> | partially performed, unless otherwise stated.
> 
> But in general we would be rather concerned if a regular read or write
> on a file descriptor failed *and* still updated the file offset.  Not
> sure about directory streams.

I think the snippet you stated does not really prevent us to return an
empty entry with a transient error (ENAMETOOLONG), since it is clear
different than entries read error for getdents itself (EINVAL, ENOENT).

Former mean that only the referred entry could not be obtained, where
latter means that either there is a EOF (if the directory has been
removed) or the buffer is too small.  For latter we might still try to
resize the internal buffer to accommodate the required size, but this
incur in the memory failure possible error nonetheless.

> 
>> I think it is a fair assumption
>> since it also states that 'When the end of the directory is encountered, 
>> a null pointer shall be returned and errno is not changed.'.  So there
>> is a distinction between end of directory stream and a failure (in the
>> example above checking for 'errno == 0' should indicate the end of the
>> stream).
> 
> The question is whether you are supposed to be able to continue reading
> and encounter errno == 0 eventually.  In general, this is simply not
> true, consider EIO or EUCLEAN.  This is why I went with delayed error
> reporting for readdir_r: it is simply not reasonable to expect that
> applications continue calling readddir_r or readdir after the first
> error because it's not clear how to can avoid an endless loop (even
> assuming that the failing readdir call somehow advanced the read
> pointer).
> 
> I hope this explains my point of view.

I understood your point, however we are constraint by opendir interface.
Different than readdir_r, for readdir we can't signal that some error
has occurred in obtained the entry while returning the partial entry
information (for the case of ENAMETOOLONG for instance).

We can just skip the ENAMETOOLONG entries is set the errno only for EOF, 
but even  with this approach we might encounter different errors (due 
getdents failure for instance) which might shadow this specific interface
error glibc might trigger.

> 
>> And it also defines EOVERFLOW as possible transient error.  It
>> is possible to do for readdir_r because the function return is different
>> than the returned 'direct' itself, for readdir what we have is the
>> 'errno' to signal such failure.
> 
> I don't understand this point.  The approach to detect EOF vs error is
> different for readdir_r, but the question whether continue after the
> first error is equally relevant.

My point is if we decide to ignore entries with ENAMETOOLONG, how to signal 
the readdir caller this has happened?

> 
>>>> Even if go to buffer resize, application will still need to handle
>>>> ENOMEM so I am not sure if using separated buffer is really an
>>>> improvement here (at least on trying to adequate usual way to call
>>>> opendir functions).
>>>
>>> ENOMEM is a temporary error, ENAMETOOLONG depends on file system content
>>> and may be much more difficult to resolve.
>>
>> Even with ENOMEM, application will require to actually handle a possible
>> readdir failure and acts accordingly (either by retrying or skip the
>> file).  But I consider failing with ENOMEM in fact much more confusing,
>> it exposes internal implementation that needs to be handled by the
>> application (the internal memory reallocation), where ENAMETOOLONG is
>> indeed a system limitation that can be resolved by using LFS support.
> 
> I think readdir can already fail with ENOMEM if the error comes from the
> kernel, for example from ext4_ind_map_blocks and ext4_alloc_branch.  So
> adding another corner case where ENOMEM can result does not seem
> egregiously bad to me.

Is it a possible error for getdents? I couldn't really track it, but I
haven't dig into kernel code that deep. In any case it would be good
to update the getdents implementation.