Re: [PATCH] libio: Implement internal function __libc_readline_unlocked

[Florian Weimer <fweimer at redhat dot com>]

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?

Thanks,
Florian