Re: [PATCH] io: Refactor close_range and closefrom

[Adhemerval Zanella <adhemerval.zanella@linaro.org>]

On 08/11/2021 11:13, Sergey Bugaev wrote:
> Hello,
> 
> I hope it's appropriate for me to leave some comments on the patch...
> 
> On Mon, Nov 8, 2021 at 4:58 PM Adhemerval Zanella
> <adhemerval.zanella@linaro.org> wrote:
>> +#if __ASSUME_CLOSE_RANGE
>> +static inline _Bool __closefrom_fallback (int __lowfd, _Bool dirfd_fallback)
>> +{
>> +  return true;
>> +}
> 
> My understanding is that this will get called if we assume close_range
> is available, but it still somehow fails. Should it maybe return false
> in this case, to cause closefrom to fail/abort, rather than silently
> succeeding?

Indeed it makes sense to return false and force the __fortify_fail() on
the generic implementation.

> 
>> +stub_warning (close_range)
> 
> Wouldn't this warn that "close_range is not implemented and will
> always fail"? I'd expect that warning for a stub that always fails
> with ENOSYS, but not for a working (if non-atomic etc) implementation.

Yeah, this does not make sense. I will remove it.

> 
>> --- a/posix/unistd.h
>> +++ b/posix/unistd.h
>> @@ -1199,6 +1199,17 @@ int getentropy (void *__buffer, size_t __length) __wur
>>      __attr_access ((__write_only__, 1, 2));
>>  #endif
>>
>> +#ifdef __USE_GNU
>> +/* Close all file descriptors in the range FD up to MAX_FD.  The flag FLAGS
>> +   are define by the CLOSE_RANGE prefix.  This function behaves like close
>> +   on the range, but in a fail-safe where it will either fail and not close
>> +   any file descriptor or close all of them.  Gaps where the file descriptor
> 
> Is this (either closes everything or nothing) an appropriate thing to
> promise in the common header? Similarly, if the default implementation
> accepts no flags, should the common description mention "the
> CLOSE_RANGE prefix"?

Well, that's the semantic of the both the syscall and the Linux fallback.
If Hurd does not provide such semantic I think you should work this out.

For CLOSE_RANGE I think it does make sense because it is not setting
any support flag, but rather the way it might be provided by the system.