Re: [PATCH 1/3] <fd_to_filename.h>: Add type safety and port to Hurd

[Florian Weimer <fweimer@redhat.com>]

* Paul Eggert:

> On 2/14/20 10:11 AM, Florian Weimer wrote:
>> +  /* A positive int value has at most 10 decimal digits.  */
>> +  char buffer[sizeof (FD_TO_FILENAME_PREFIX) + 10];
>
> This should use 'INT_STRLEN_BOUND (int)' rather than '10', where
> INT_STRLEN_BOUND is taken from intprops.h. Then you don't need the
> comment (and the code won't break on future ILP64 platforms :-).

But INT_STRLEN_BOUND is 11, right?

>> +char *
>> +__fd_to_filename (int descriptor, struct fd_to_filename *storage)
>> +{
>> +  char *p = mempcpy (storage->buffer, FD_TO_FILENAME_PREFIX,
>> +                     strlen (FD_TO_FILENAME_PREFIX));
>> +  if (__glibc_likely (descriptor >= 0))
>> +    {
>> +      if (descriptor < 1000)
>> +        p = digit123 (p, descriptor);
>> +      else if (descriptor < 1000 * 1000) ...
>
> It's not clear what that "descriptor >= 0" test is doing. I assume
> that a precondition is that DESCRIPTOR is nonnegative; if so, this
> should be mentioned and the test removed. If it's not a precondition,
> the code should do the right thing if DESCRIPTOR is negative (I
> suppose, return a filename that cannot be opened, though it doesn't do
> that currently) -- which should also be documented but I think this'd
> be overkill.

The problem is when an application passes an invalid descriptor to some
libc function and that ends up with __fd_to_filename.  We should not
make matters worse in that case.

Maybe we should use an assert?

Thanks,
Florian