Re: [PATCH] libio: Implement internal function __libc_readline_unlocked

[Carlos O'Donell <carlos at redhat dot com>]

On 07/06/2018 08:31 AM, Florian Weimer wrote:
> On 07/06/2018 02:31 PM, Florian Weimer wrote:
>> On 11/22/2017 06:50 AM, Jonathan Nieder wrote:
>>> 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?
>>
>> If off_t and off64_t are the same type, __ftello and __ftello64 are the same function (at the same address).
>>
>>> [...]
>>>> +++ 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.
>>
>> The caller should not call the function if the stream is in an error condition I added a comment to the header file.
>>
>>>
>>>> +          *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).
>>
>> Thanks, fixed.
>>
>>> [...]
>>>> +  /* 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?
>>
>> The function is not meant to be used for such files.
>>
>> I've rebased the patch and fixed the internal alias generation logic.
>>
>> Carlos, is this and the fix for bug 18991 (which has already been
>> reviewed by DJ) still acceptable during the freeze?

Yes, they are still OK during the freeze. They are technically bug fixes
for a known issue, and the functions implemented are GLIBC_PRIVATE. I see little
risk in these functions causing a release regression.

>>
>> Thanks,
>> Florian
> 
> And now with the patch attached.
> 
> readline.patch
> 
> 
> Subject: [PATCH] libio: Implement internal function __libc_readline_unlocked
> To: libc-alpha@sourceware.org
> 
> This is a variant of fgets which fails with ERANGE if the
> buffer is too small, and the buffer length is given as an
> argument of type size_t.
> 
> This function will be useful for implementing NSS file reading
> operations.  Compared to a direct implementation using the public API,
> it avoids an lseek system call in case the line terminator can be
> found in the internal read buffer.
> 
> 2018-07-06  Florian Weimer  <fweimer@redhat.com>
> 
> 	* include/stdio.h (__libc_readline_unlocked): Declare.
> 	(__ftello64, __fseeko64): Declare aliases.
> 	* libio/readline.c: New file.
> 	* libio/tst-readline.c: Likewise.
> 	(routines): Add readline.
> 	(tests-internal): Add tst-readlime.
> 	* libio/Versions (GLIBC_PRIVATE): Export __fseeko64, __ftello64,
> 	__libc_readline_unlocked.
> 	* libio/fseeko.c (__fseeko): Rename from fseeko.
> 	(fseeko): Add alias.
> 	[__OFF_T_MATCHES_OFF64_T] (fseeko64, __fseeko64): Likewise.
> 	* libio/fseeko64.c (__fseeko64): Rename from fseeko64.
> 	(fseeko64): Add alias.
> 	* libio/ftello.c [__OFF_T_MATCHES_OFF64_T] (__ftello64): Add alias.
> 	* libio/ftello64.c (__ftello64): Rename from ftello64.
> 	(ftello64): Add alias.
> 

OK with comment tweak in readline_slow for assert.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>

> diff --git a/include/stdio.h b/include/stdio.h
> index 3ba0edc924..9162d4e247 100644
> --- a/include/stdio.h
> +++ b/include/stdio.h
> @@ -127,6 +127,19 @@ extern int __fxprintf (FILE *__fp, const char *__fmt, ...)
>  extern int __fxprintf_nocancel (FILE *__fp, const char *__fmt, ...)
>       __attribute__ ((__format__ (__printf__, 2, 3))) attribute_hidden;
>  
> +/* Read the next line from FP into BUFFER, of LENGTH bytes.  LINE will
> +   include the line terminator and a NUL terminator.  On success,
> +   return the length of the line, including the line terminator, but
> +   excluding the NUL termintor.  On EOF, return zero and write a NUL
> +   terminator.  On error, return -1 and set errno.  If the total byte
> +   count (line and both terminators) exceeds LENGTH, return -1 and set
> +   errno to ERANGE (but do not mark the stream as failed).

OK.

> +
> +   The behavior is undefined if FP is not seekable, or if the stream
> +   is already in an error state.  */

OK.

> +ssize_t __libc_readline_unlocked (FILE *fp, char *buffer, size_t length);
> +libc_hidden_proto (__libc_readline_unlocked);
> +
>  extern const char *const _sys_errlist_internal[] attribute_hidden;
>  extern int _sys_nerr_internal attribute_hidden;
>  
> @@ -170,6 +183,10 @@ libc_hidden_proto (fwrite)
>  libc_hidden_proto (fseek)
>  extern __typeof (ftello) __ftello;
>  libc_hidden_proto (__ftello)
> +extern __typeof (fseeko64) __fseeko64;
> +libc_hidden_proto (__fseeko64)
> +extern __typeof (ftello64) __ftello64;
> +libc_hidden_proto (__ftello64)

OK.

>  libc_hidden_proto (fflush)
>  libc_hidden_proto (fflush_unlocked)
>  extern __typeof (fflush_unlocked) __fflush_unlocked;
> diff --git a/libio/Makefile b/libio/Makefile
> index 64d283e512..cab0eae946 100644
> --- a/libio/Makefile
> +++ b/libio/Makefile
> @@ -49,7 +49,7 @@ routines	:=							      \
>  	__fbufsize __freading __fwriting __freadable __fwritable __flbf	      \
>  	__fpurge __fpending __fsetlocking				      \
>  									      \
> -	libc_fatal fmemopen oldfmemopen vtables
> +	libc_fatal fmemopen oldfmemopen vtables readline

OK.

>  
>  tests = tst_swprintf tst_wprintf tst_swscanf tst_wscanf tst_getwc tst_putwc   \
>  	tst_wprintf2 tst-widetext test-fmemopen tst-ext tst-ext2 \
> @@ -66,7 +66,7 @@ tests = tst_swprintf tst_wprintf tst_swscanf tst_wscanf tst_getwc tst_putwc   \
>  	tst-fwrite-error tst-ftell-partial-wide tst-ftell-active-handler \
>  	tst-ftell-append tst-fputws tst-bz22415 tst-fgetc-after-eof
>  
> -tests-internal = tst-vtables tst-vtables-interposed
> +tests-internal = tst-vtables tst-vtables-interposed tst-readline

OK.

>  
>  ifeq (yes,$(build-shared))
>  # Add test-fopenloc only if shared library is enabled since it depends on
> diff --git a/libio/Versions b/libio/Versions
> index 77123347e3..acb896afa9 100644
> --- a/libio/Versions
> +++ b/libio/Versions
> @@ -158,5 +158,9 @@ libc {
>  
>      # Used by NPTL
>      _IO_enable_locks;
> +
> +    __fseeko64;
> +    __ftello64;
> +    __libc_readline_unlocked;

OK.

>    }
>  }
> diff --git a/libio/fseeko.c b/libio/fseeko.c
> index 2df0453d35..4d086c15d8 100644
> --- a/libio/fseeko.c
> +++ b/libio/fseeko.c
> @@ -24,11 +24,15 @@
>     This exception applies to code released by its copyright holders
>     in files containing the exception.  */
>  
> +/* We need to disable the redirect for __fseeko64 for the alias
> +   definitions below to work.  */
> +#define __fseeko64 __fseeko64_disable
> +
>  #include "libioP.h"
>  #include "stdio.h"
>  
>  int
> -fseeko (FILE *fp, off_t offset, int whence)
> +__fseeko (FILE *fp, off_t offset, int whence)

OK.

>  {
>    int result;
>    CHECK_FILE (fp, -1);
> @@ -37,7 +41,11 @@ fseeko (FILE *fp, off_t offset, int whence)
>    _IO_release_lock (fp);
>    return result;
>  }
> +weak_alias (__fseeko, fseeko)

OK.

>  
>  #ifdef __OFF_T_MATCHES_OFF64_T
> -weak_alias (fseeko, fseeko64)
> +weak_alias (__fseeko, fseeko64)
> +# undef __fseeko64
> +strong_alias (__fseeko, __fseeko64)
> +libc_hidden_ver (__fseeko, __fseeko64)

OK.

>  #endif
> diff --git a/libio/fseeko64.c b/libio/fseeko64.c
> index eea6455bc3..1d9bb190a6 100644
> --- a/libio/fseeko64.c
> +++ b/libio/fseeko64.c
> @@ -32,7 +32,7 @@
>  #ifndef __OFF_T_MATCHES_OFF64_T
>  
>  int
> -fseeko64 (FILE *fp, off64_t offset, int whence)
> +__fseeko64 (FILE *fp, off64_t offset, int whence)

OK.

>  {
>    int result;
>    CHECK_FILE (fp, -1);
> @@ -41,5 +41,6 @@ fseeko64 (FILE *fp, off64_t offset, int whence)
>    _IO_release_lock (fp);
>    return result;
>  }
> -
> +libc_hidden_def (__fseeko64)
> +weak_alias (__fseeko64, fseeko64)

OK.

>  #endif
> diff --git a/libio/ftello.c b/libio/ftello.c
> index 5405821b45..5a3caf4ad9 100644
> --- a/libio/ftello.c
> +++ b/libio/ftello.c
> @@ -24,12 +24,15 @@
>     This exception applies to code released by its copyright holders
>     in files containing the exception.  */
>  
> +/* We need to disable the redirect for __ftello64 for the alias
> +   definitions below to work.  */
> +#define __ftello64 __ftello64_disable

OK.

> +
>  #include <stdio.h>
>  #include <stdlib.h>
>  #include <libioP.h>
>  #include <errno.h>
>  
> -
>  off_t
>  __ftello (FILE *fp)
>  {
> @@ -61,4 +64,7 @@ weak_alias (__ftello, ftello)
>  
>  #ifdef __OFF_T_MATCHES_OFF64_T
>  weak_alias (__ftello, ftello64)
> +# undef __ftello64
> +strong_alias (__ftello, __ftello64)
> +libc_hidden_ver (__ftello, __ftello64)

OK.

>  #endif
> diff --git a/libio/ftello64.c b/libio/ftello64.c
> index 281667fee2..d5546e1fb0 100644
> --- a/libio/ftello64.c
> +++ b/libio/ftello64.c
> @@ -32,7 +32,7 @@
>  #ifndef __OFF_T_MATCHES_OFF64_T
>  
>  off64_t
> -ftello64 (FILE *fp)
> +__ftello64 (FILE *fp)

OK.

>  {
>    off64_t pos;
>    CHECK_FILE (fp, -1L);
> @@ -52,5 +52,6 @@ ftello64 (FILE *fp)
>      }
>    return pos;
>  }
> -
> +libc_hidden_def (__ftello64)
> +weak_alias (__ftello64, ftello64)

OK.

>  #endif
> diff --git a/libio/readline.c b/libio/readline.c
> new file mode 100644
> index 0000000000..d5424b1c27
> --- /dev/null
> +++ b/libio/readline.c
> @@ -0,0 +1,169 @@
> +/* fgets with ERANGE error reporting and size_t buffer length.

OK.

> +   Copyright (C) 2017 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +#include <assert.h>
> +#include <errno.h>
> +#include <stdio.h>
> +#include <string.h>
> +
> +#include "libioP.h"
> +
> +/* Return -1 and set errno to EINVAL if it is ERANGE.  */
> +static ssize_t
> +fail_no_erange (void)
> +{
> +  if (errno == ERANGE)
> +    __set_errno (EINVAL);
> +  return -1;
> +}

OK.

> +
> +/* 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 (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 ();
> +          *buffer = '\0';
> +          /* Do not include the null terminator.  */
> +          return buffer - start;
> +        }

OK.

> +
> +      /* __underflow has filled the buffer.  */
> +      char *readptr = fp->_IO_read_ptr;
> +      ssize_t readlen = fp->_IO_read_end - readptr;
> +      assert (readlen > 0);

Please add a comment explaining why readlen > 0, nominally
I expect it's because __underflow would have returned EOF
if it *was* 0 and we'd be in the above case. I think this needs
calling out with a comment here (or adjust the existing comment
a bit).

> +      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.  */
> +          return buffer - start + line_length + 1;

OK.

> +        }
> +
> +      if (readlen >= buffer_end - buffer)
> +        /* Not enough room in the caller-supplied buffer.  */
> +        break;

OK.

> +
> +      /* Save and consume the stream buffer.  */
> +      memcpy (buffer, readptr, readlen);
> +      fp->_IO_read_ptr = fp->_IO_read_end;
> +      buffer += readlen;

OK.

> +    }
> +
> +  /* The line does not fit into the buffer.  */
> +  __set_errno (ERANGE);
> +  return -1;

OK.

> +}
> +
> +ssize_t
> +__libc_readline_unlocked (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)
> +    {
> +      char *pnl = memchr (readptr, '\n', readlen);
> +      if (pnl != NULL)
> +        {
> +          size_t line_length = pnl - readptr;
> +          /* Account for line and null terminators.  */
> +          if (line_length + 2 > buffer_length)
> +            {
> +              __set_errno (ERANGE);
> +              return -1;

OK.

> +            }
> +          memcpy (buffer, readptr, line_length + 1);
> +          buffer[line_length + 1] = '\0';
> +          /* Consume the entire line.  */
> +          fp->_IO_read_ptr = pnl + 1;
> +          return line_length + 1;

OK.

> +        }
> +
> +      /* If the buffer does not have enough space for what is pending
> +         in the stream (plus a NUL terminator), the buffer is too
> +         small.  */
> +      if (readlen + 1 > buffer_length)
> +        {
> +          __set_errno (ERANGE);
> +          return -1;

OK.

> +        }
> +
> +      /* End of line not found.  We need all the buffered data.  Fall
> +         through to the slow path.  */
> +      memcpy (buffer, readptr, readlen);
> +      buffer += readlen;
> +      /* The original length is invalid after this point.  Use
> +         buffer_end instead.  */
> +#pragma GCC poison buffer_length
> +      /* Read the old offset before updating the read pointer.  */
> +      start_offset = __ftello64 (fp);
> +      fp->_IO_read_ptr = fp->_IO_read_end;
> +    }
> +  else
> +    {
> +      readlen = 0;
> +      start_offset = __ftello64 (fp);
> +    }
> +
> +  /* 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 ();

OK.

> +
> +  ssize_t result = readline_slow (fp, buffer, buffer_end);

OK, calling readline_slow from above.

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

OK. Allows caller to re-read the same line with a longer buffer.

> +            return fail_no_erange ();
> +          __set_errno (ERANGE);
> +        }
> +      /* Do not restore the file position on other errors; it is
> +         likely that the __fseeko64 call would fail, too.  */
> +      return -1;

Ok.

> +    }
> +  return readlen + result;
> +}
> +libc_hidden_def (__libc_readline_unlocked)
> diff --git a/libio/tst-readline.c b/libio/tst-readline.c
> new file mode 100644
> index 0000000000..79ed6ea699
> --- /dev/null
> +++ b/libio/tst-readline.c
> @@ -0,0 +1,235 @@
> +/* Test the __libc_readline_unlocked function.

OK.

> +   Copyright (C) 2017 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +/* Exercise __libc_readline_unlocked with various combinations of line
> +   lengths, stdio buffer sizes, and line read buffer sizes.  */
> +
> +#include <errno.h>
> +#include <stdbool.h>
> +#include <stdio.h>
> +#include <support/check.h>
> +#include <support/test-driver.h>
> +#include <support/support.h>
> +#include <support/xstdio.h>
> +#include <support/xmemstream.h>
> +#include <support/temp_file.h>
> +#include <support/xunistd.h>
> +
> +enum
> +  {
> +    maximum_line_length = 7,
> +    number_of_lines = 3,
> +  };

OK.

> +
> +/* -1: Do not set buffer size.  0: unbuffered.  Otherwise, use this as
> +   the size of the buffer.  */
> +static int buffer_size;
> +
> +/* These size of the buffer used for reading.  Must be at least 2.  */
> +static int read_size;
> +
> +/* If a read files with ERANGE, increase the buffer size by this
> +   amount.  Must be positive.  */
> +static int read_size_increment;
> +
> +/* If non-zero, do not reset the read size after an ERANGE error.  */
> +static int read_size_preserve;
> +
> +/* If non-zero, no '\n' at the end of the file.  */
> +static int no_newline_at_eof;
> +
> +/* Length of the line, or -1 if the line is not present.  */
> +static int line_lengths[number_of_lines];
> +
> +/* The name of the test file.  */
> +static char *test_file_path;
> +
> +/* The contents of the test file.  */
> +static char expected_contents[(maximum_line_length + 2) * number_of_lines + 1];
> +static size_t expected_length;
> +
> +/* Returns a random byte which is not zero or the line terminator.  */
> +static char
> +random_char (void)
> +{
> +  static unsigned int rand_state = 1;
> +  while (true)
> +    {
> +      char result = rand_r (&rand_state) >> 16;
> +      if (result != 0 && result != '\n')
> +        return result;
> +    }
> +}

OK.

> +
> +/* Create the test file.  */
> +static void
> +prepare (int argc, char **argv)
> +{
> +  int fd = create_temp_file ("tst-readline-", &test_file_path);
> +  TEST_VERIFY_EXIT (fd >= 0);
> +  xclose (fd);
> +}

OK.

> +
> +/* Prepare the test file.  Return false if the test parameters are
> +   incongruent and the test should be skipped.  */
> +static bool
> +write_test_file (void)
> +{
> +  expected_length = 0;
> +  char *p = expected_contents;
> +  for (int lineno = 0; lineno < number_of_lines; ++lineno)
> +    for (int i = 0; i < line_lengths[lineno]; ++i)
> +      *p++ = random_char ();
> +  expected_length = p - &expected_contents[0];
> +  if (no_newline_at_eof)
> +    {
> +      if (expected_length == 0)
> +        return false;
> +      --expected_length;
> +      --p;
> +    }
> +  if (test_verbose > 0)
> +    {
> +      printf ("info: writing test file of %zu bytes:\n", expected_length);
> +      for (int i = 0; i < number_of_lines; ++i)
> +        printf (" line %d: %d\n", i, line_lengths[i]);
> +      if (no_newline_at_eof)
> +        puts ("  (no newline at EOF)");
> +    }
> +  TEST_VERIFY_EXIT (expected_length < sizeof (expected_contents));
> +  *p++ = '\0';
> +  support_write_file_string (test_file_path, expected_contents);
> +  return true;
> +}

OK.

> +
> +/* Run a single test (a combination of a test file and read
> +   parameters).  */
> +static void
> +run_test (void)
> +{
> +  TEST_VERIFY_EXIT (read_size_increment > 0);
> +  if (test_verbose > 0)
> +    {
> +      printf ("info: running test: buffer_size=%d read_size=%d\n"
> +              "  read_size_increment=%d read_size_preserve=%d\n",
> +              buffer_size, read_size, read_size_increment, read_size_preserve);
> +    }
> +
> +  struct xmemstream result;
> +  xopen_memstream (&result);
> +
> +  FILE *fp = xfopen (test_file_path, "rce");
> +  char *fp_buffer = NULL;
> +  if (buffer_size == 0)
> +    TEST_VERIFY_EXIT (setvbuf (fp, NULL, _IONBF, 0) == 0);
> +  if (buffer_size > 0)
> +    {
> +      fp_buffer = xmalloc (buffer_size);
> +      TEST_VERIFY_EXIT (setvbuf (fp, fp_buffer, _IOFBF, buffer_size) == 0);
> +    }
> +
> +  char *line_buffer = xmalloc (read_size);
> +  size_t line_buffer_size = read_size;
> +
> +  while (true)
> +    {
> +      ssize_t ret = __libc_readline_unlocked
> +        (fp, line_buffer, line_buffer_size);

OK.

> +      if (ret < 0)
> +        {
> +          TEST_VERIFY (ret == -1);
> +          if (errno != ERANGE)
> +            FAIL_EXIT1 ("__libc_readline_unlocked: %m");
> +          line_buffer_size += read_size_increment;
> +          free (line_buffer);
> +          line_buffer = xmalloc (line_buffer_size);
> +          /* Try reading this line again.  */

OK.

> +        }
> +      else if (ret == 0)
> +        break;
> +      else
> +        {
> +          /* A line has been read.  Save it.  */
> +          TEST_VERIFY (ret == strlen (line_buffer));
> +          const char *pnl = strchr (line_buffer, '\n');
> +          /* If there is a \n, it must be at the end.  */
> +          TEST_VERIFY (pnl == NULL || pnl == line_buffer + ret - 1);
> +          fputs (line_buffer, result.out);
> +
> +          /* Restore the original read size if required.  */
> +          if (line_buffer_size > read_size && !read_size_preserve)
> +            {
> +              line_buffer_size = read_size;
> +              free (line_buffer);
> +              line_buffer = xmalloc (line_buffer_size);
> +            }
> +        }
> +    }
> +

OK.

> +  xfclose (fp);
> +  free (fp_buffer);
> +  free (line_buffer);
> +
> +  xfclose_memstream (&result);
> +  TEST_VERIFY (result.length == expected_length);
> +  TEST_VERIFY (strcmp (result.buffer, expected_contents) == 0);
> +  if (test_verbose > 0)
> +    {
> +      printf ("info: expected (%zu): [[%s]]\n",
> +              expected_length, expected_contents);
> +      printf ("info:   actual (%zu): [[%s]]\n", result.length, result.buffer);
> +    }
> +  free (result.buffer);

OK.

> +}
> +
> +/* Test one test file with multiple read parameters.  */
> +static void
> +test_one_file (void)
> +{
> +  for (buffer_size = -1; buffer_size <= maximum_line_length + 1; ++buffer_size)
> +    for (read_size = 2; read_size <= maximum_line_length + 2; ++read_size)
> +      for (read_size_increment = 1; read_size_increment <= 4;
> +           ++read_size_increment)
> +        for (read_size_preserve = 0; read_size_preserve < 2;
> +             ++read_size_preserve)
> +          run_test ();

OK.

> +}
> +
> +
> +static int
> +do_test (void)
> +{
> +  /* Set up the test file contents.  */
> +  for (line_lengths[0] = -1; line_lengths[0] <= maximum_line_length;
> +       ++line_lengths[0])
> +    for (line_lengths[1] = -1; line_lengths[1] <= maximum_line_length;
> +         ++line_lengths[1])
> +      for (line_lengths[2] = -1; line_lengths[2] <= maximum_line_length;
> +           ++line_lengths[2])
> +        for (no_newline_at_eof = 0; no_newline_at_eof < 2; ++no_newline_at_eof)
> +          {
> +            if (!write_test_file ())
> +              continue;
> +            test_one_file ();
> +          }
> +  free (test_file_path);
> +  return 0;

OK. Permute over the lines from min to max length and EOF or no EOF at end of file.

Looks good.

> +}
> +
> +#define PREPARE prepare
> +#include <support/test-driver.c>

-- 
Cheers,
Carlos.