Re: [PATCH] libio: Implement internal function __libc_readline_unlocked

[Jonathan Nieder <jrnieder at gmail dot com>]

Florian Weimer wrote:

> --- a/libio/ftello.c
> +++ b/libio/ftello.c
> @@ -61,4 +61,6 @@ weak_alias (__ftello, ftello)
> 
>  #ifdef __OFF_T_MATCHES_OFF64_T
>  weak_alias (__ftello, ftello64)
> +strong_alias (__ftello, __ftello64)

What does this part do?

[...]
> +++ b/libio/readline.c
> @@ -0,0 +1,169 @@
[...]
> +/* Slow path for reading the line.  Called with no data in the stream
> +   read buffer.  Write data to [BUFFER, BUFFER_END).  */
> +static ssize_t
> +readline_slow (_IO_FILE *fp, char *buffer, char *buffer_end)
> +{
> +  char *start = buffer;
> +
> +  while (buffer < buffer_end)
> +    {
> +      if (__underflow (fp) == EOF)
> +        {
> +          if (_IO_ferror_unlocked (fp))
> +            /* If the EOF was caused by a read error, report it.  */
> +            return fail_no_erange ();

Is readline_slow's caller responsible for ensuring !ferror(fp) before
the __underflow call?  Otherwise it's possible for this to return -1
without setting errno.

> +          *buffer = '\0';
> +          /* Do not include the NULL terminator.  */

nit: s/NULL/NUL/ or s/NULL/null/ (NULL is a pointer, NUL an ascii code
point).

> +          return buffer - start;
> +        }
[...]
> +      char *pnl = memchr (readptr, '\n', readlen);
> +      if (pnl != NULL)
> +        {
> +          /* We found the terminator.  */
> +          size_t line_length = pnl - readptr;
> +          if (line_length + 2 > buffer_end - buffer)
> +            /* Not enough room in the caller-supplied buffer.  */
> +            break;
> +          memcpy (buffer, readptr, line_length + 1);
> +          buffer[line_length + 1] = '\0';
> +          fp->_IO_read_ptr = pnl + 1;
> +          /* Do not include the NULL terminator.  */

Same nit about \0 vs NULL.

> +          return buffer - start + line_length + 1;
> +        }
[...]
> +ssize_t
> +__libc_readline_unlocked (_IO_FILE *fp, char *buffer, size_t buffer_length)
> +{
> +  char *buffer_end = buffer + buffer_length;
> +
> +  /* Orient the stream.  */
> +  if (__builtin_expect (fp->_mode, -1) == 0)
> +    _IO_fwide (fp, -1);
> +
> +  /* Fast path: The line terminator is found in the buffer.  */
> +  char *readptr = fp->_IO_read_ptr;
> +  ssize_t readlen = fp->_IO_read_end - readptr;
> +  off64_t start_offset;         /* File offset before reading anything.  */
> +  if (readlen > 0)
> +    {

optional: could handle the !readlen case first to avoid leaving the
reader in suspense.

> +      char *pnl = memchr (readptr, '\n', readlen);
[...]
> +      buffer += readlen;
> +      /* The original length is invalid after this point.  Use
> +         buffer_end instead.  */
> +#pragma GCC poison buffer_length

Neat!  I hadn't seen this trick before.

[...]
> +  /* Slow path: Read more data from the underlying file.  We need to
> +     restore the file pointer if the buffer is too small.  First,
> +     check if the __ftello64 call above failed.  */
> +  if (start_offset < 0)
> +    return fail_no_erange ();
> +
> +  ssize_t result = readline_slow (fp, buffer, buffer_end);
> +  if (result < 0)
> +    {
> +      if (errno == ERANGE)
> +        {
> +          /* Restore the file pointer so that the caller may read the
> +             same line again.  */
> +          if (__fseeko64 (fp, start_offset, SEEK_SET) < 0)
> +            return fail_no_erange ();

What happens if the file is unseekable?  Is this function not meant to
be used for such files, or is this seeking to reset the file pointer a
best-effort kind of thing?

[...]
> +  return readlen + result;
> +}

Looks good.  The test also looks good.

The only part that worries me is the value of errno if ferror(fp) was
already true.  The rest of my observations are cosmetic.

Thanks and hope that helps,
Jonathan