[PATCH/RFA] _fflush_r check seek result fix

[Corinna Vinschen]

Hi,

there was a short thread on the Cygwin list, in which it turned out that
fclose on certain Cygwin devices opened for readin could return an
error: http://cygwin.com/ml/cygwin/2009-10/msg00562.html

I tracked it down to the _fflush_r function.  _fflush_r calls fp->seek,
basically like this:

  curoff = fp->_seek (0, SEEK_CUR);
  curoff = -= fp->_r; // Take buffer position into account
  tmp = (fp->_seek (curoff, SEEK_SET) == curoff)
  if (tmp)
    // Success
  else
    // Failure

This code ignores the possibility that the offset returned by seek does
not exactly match the desired position.  This result is no error
condition, especially when taking devices into account.  It's especially
no error if lseek returns a negative offset on character special
devices.  
Please note that lseek on the Cygwin devices mentioned in the above
thread behave exactly like their Linux counterparts.  Thus, the same
_fseek_r would also treat the Linux behaviour as error.

Below is a patch which tries to fix this problem.  The idea is that only
a return code of -1 with errno set to some value != 0 is really an
error.  To accomplish this, the code stores the current errno, calls
fp->seek, and then checks for a return value of -1 and the errno.  If no
error has happened from the point of view of the underlying device, it
treats the result as success, sets the new offset, and restores the old
errno.  Otherwise, an error occured and the new errno is returned to the
calling code.

Is that ok?


Corinna


        * libc/stdio/fflush.c (_fflush_r): Don't use new position
        after seek as error condition, rather check for return value of
        -1 and errno.


Index: libc/stdio/fflush.c
===================================================================
RCS file: /cvs/src/src/newlib/libc/stdio/fflush.c,v
retrieving revision 1.14
diff -u -p -r1.14 fflush.c
--- libc/stdio/fflush.c 22 Jul 2009 02:17:12 -0000 1.14
+++ libc/stdio/fflush.c 23 Oct 2009 13:05:23 -0000
@@ -115,7 +115,7 @@ _DEFUN(_fflush_r, (ptr, fp),
          to miss a code scenario.  */
       if ((fp->_r > 0 || fp->_ur > 0) && fp->_seek != NULL)
  {
-  int tmp;
+  int tmp_errno;
 #ifdef __LARGE64_FILES
   _fpos64_t curoff;
 #else
@@ -155,13 +155,15 @@ _DEFUN(_fflush_r, (ptr, fp),
                 curoff -= fp->_ur;
             }
   /* Now physically seek to after byte last read.  */
+  tmp_errno = ptr->_errno;
+  ptr->_errno = 0;
 #ifdef __LARGE64_FILES
   if (fp->_flags & __SL64)
-    tmp = (fp->_seek64 (ptr, fp->_cookie, curoff, SEEK_SET) == curoff);
+    curoff = fp->_seek64 (ptr, fp->_cookie, curoff, SEEK_SET);
   else
 #endif
-    tmp = (fp->_seek (ptr, fp->_cookie, curoff, SEEK_SET) == curoff);
-  if (tmp)
+    curoff = fp->_seek (ptr, fp->_cookie, curoff, SEEK_SET);
+  if (curoff != -1 || ptr->_errno == 0)
     {
       /* Seek successful.  We can clear read buffer now.  */
       fp->_flags &= ~__SNPT;
@@ -169,6 +171,7 @@ _DEFUN(_fflush_r, (ptr, fp),
       fp->_p = fp->_bf._base;
       if (fp->_flags & __SOFF)
  fp->_offset = curoff;
+      ptr->_errno = tmp_errno;
       if (HASUB (fp))
  FREEUB (ptr, fp);
     }



--
Corinna Vinschen
Cygwin Project Co-Leader
Red Hat

[Corinna Vinschen]

Jeff,

could you please have a look into this patch?


Thanks,
Corinna

On Oct 23 15:08, Corinna Vinschen wrote:

> Hi,
>
> there was a short thread on the Cygwin list, in which it turned out that
> fclose on certain Cygwin devices opened for readin could return an
> error: http://cygwin.com/ml/cygwin/2009-10/msg00562.html
>
> I tracked it down to the _fflush_r function.  _fflush_r calls fp->seek,
> basically like this:
>
>   curoff = fp->_seek (0, SEEK_CUR);
>   curoff = -= fp->_r; // Take buffer position into account
>   tmp = (fp->_seek (curoff, SEEK_SET) == curoff)
>   if (tmp)
>     // Success
>   else
>     // Failure
>
> This code ignores the possibility that the offset returned by seek does
> not exactly match the desired position.  This result is no error
> condition, especially when taking devices into account.  It's especially
> no error if lseek returns a negative offset on character special
> devices.  
> Please note that lseek on the Cygwin devices mentioned in the above
> thread behave exactly like their Linux counterparts.  Thus, the same
> _fseek_r would also treat the Linux behaviour as error.
>
> Below is a patch which tries to fix this problem.  The idea is that only
> a return code of -1 with errno set to some value != 0 is really an
> error.  To accomplish this, the code stores the current errno, calls
> fp->seek, and then checks for a return value of -1 and the errno.  If no
> error has happened from the point of view of the underlying device, it
> treats the result as success, sets the new offset, and restores the old
> errno.  Otherwise, an error occured and the new errno is returned to the
> calling code.
>
> Is that ok?
>
>
> Corinna
>
>
> * libc/stdio/fflush.c (_fflush_r): Don't use new position
> after seek as error condition, rather check for return value of
> -1 and errno.
>
>
> Index: libc/stdio/fflush.c
> ===================================================================
> RCS file: /cvs/src/src/newlib/libc/stdio/fflush.c,v
> retrieving revision 1.14
> diff -u -p -r1.14 fflush.c
> --- libc/stdio/fflush.c 22 Jul 2009 02:17:12 -0000 1.14
> +++ libc/stdio/fflush.c 23 Oct 2009 13:05:23 -0000
> @@ -115,7 +115,7 @@ _DEFUN(_fflush_r, (ptr, fp),
>           to miss a code scenario.  */
>        if ((fp->_r > 0 || fp->_ur > 0) && fp->_seek != NULL)
>   {
> -  int tmp;
> +  int tmp_errno;
>  #ifdef __LARGE64_FILES
>    _fpos64_t curoff;
>  #else
> @@ -155,13 +155,15 @@ _DEFUN(_fflush_r, (ptr, fp),
>                  curoff -= fp->_ur;
>              }
>    /* Now physically seek to after byte last read.  */
> +  tmp_errno = ptr->_errno;
> +  ptr->_errno = 0;
>  #ifdef __LARGE64_FILES
>    if (fp->_flags & __SL64)
> -    tmp = (fp->_seek64 (ptr, fp->_cookie, curoff, SEEK_SET) == curoff);
> +    curoff = fp->_seek64 (ptr, fp->_cookie, curoff, SEEK_SET);
>    else
>  #endif
> -    tmp = (fp->_seek (ptr, fp->_cookie, curoff, SEEK_SET) == curoff);
> -  if (tmp)
> +    curoff = fp->_seek (ptr, fp->_cookie, curoff, SEEK_SET);
> +  if (curoff != -1 || ptr->_errno == 0)
>      {
>        /* Seek successful.  We can clear read buffer now.  */
>        fp->_flags &= ~__SNPT;
> @@ -169,6 +171,7 @@ _DEFUN(_fflush_r, (ptr, fp),
>        fp->_p = fp->_bf._base;
>        if (fp->_flags & __SOFF)
>   fp->_offset = curoff;
> +      ptr->_errno = tmp_errno;
>        if (HASUB (fp))
>   FREEUB (ptr, fp);
>      }
>
>
>
> --
> Corinna Vinschen
> Cygwin Project Co-Leader
> Red Hat

--
Corinna Vinschen
Cygwin Project Co-Leader
Red Hat

[Jeff Johnston]

On 23/10/09 09:08 AM, Corinna Vinschen wrote:

> Hi,
>
> there was a short thread on the Cygwin list, in which it turned out that
> fclose on certain Cygwin devices opened for readin could return an
> error: http://cygwin.com/ml/cygwin/2009-10/msg00562.html
>
> I tracked it down to the _fflush_r function.  _fflush_r calls fp->seek,
> basically like this:
>
>    curoff = fp->_seek (0, SEEK_CUR);
>    curoff = -= fp->_r; // Take buffer position into account
>    tmp = (fp->_seek (curoff, SEEK_SET) == curoff)
>    if (tmp)
>      // Success
>    else
>      // Failure
>
> This code ignores the possibility that the offset returned by seek does
> not exactly match the desired position.  This result is no error
> condition, especially when taking devices into account.  It's especially
> no error if lseek returns a negative offset on character special
> devices.
> Please note that lseek on the Cygwin devices mentioned in the above
> thread behave exactly like their Linux counterparts.  Thus, the same
> _fseek_r would also treat the Linux behaviour as error.
>
> Below is a patch which tries to fix this problem.  The idea is that only
> a return code of -1 with errno set to some value != 0 is really an
> error.  To accomplish this, the code stores the current errno, calls
> fp->seek, and then checks for a return value of -1 and the errno.  If no
> error has happened from the point of view of the underlying device, it
> treats the result as success, sets the new offset, and restores the old
> errno.  Otherwise, an error occured and the new errno is returned to the
> calling code.
>
> Is that ok?
>

So for these devices, you're saying seek is supported, but they cannot
return their position (correct)?  Otherwise, if seek isn't supported,
why don't they set ESPIPE?

Above the code you are changing is a check that either takes the file
offset that has been tracked or tries to calculate it.  If it tries to
calculate and -1 is returned, it returns 0 if ESPIPE, otherwise, it
returns failure (it fails to reset errno in the case of the ignored ESPIPE).

Should that logic be changed to match yours or should your new logic do
the same? (i.e. treat ESPIPE as ok and otherwise -1 is failure).

-- Jeff J.

>
> Corinna
>
>
> * libc/stdio/fflush.c (_fflush_r): Don't use new position
> after seek as error condition, rather check for return value of
> -1 and errno.
>
>
> Index: libc/stdio/fflush.c
> ===================================================================
> RCS file: /cvs/src/src/newlib/libc/stdio/fflush.c,v
> retrieving revision 1.14
> diff -u -p -r1.14 fflush.c
> --- libc/stdio/fflush.c 22 Jul 2009 02:17:12 -0000 1.14
> +++ libc/stdio/fflush.c 23 Oct 2009 13:05:23 -0000
> @@ -115,7 +115,7 @@ _DEFUN(_fflush_r, (ptr, fp),
>            to miss a code scenario.  */
>         if ((fp->_r>  0 || fp->_ur>  0)&&  fp->_seek != NULL)
>   {
> -  int tmp;
> +  int tmp_errno;
>   #ifdef __LARGE64_FILES
>    _fpos64_t curoff;
>   #else
> @@ -155,13 +155,15 @@ _DEFUN(_fflush_r, (ptr, fp),
>                   curoff -= fp->_ur;
>               }
>    /* Now physically seek to after byte last read.  */
> +  tmp_errno = ptr->_errno;
> +  ptr->_errno = 0;
>   #ifdef __LARGE64_FILES
>    if (fp->_flags&  __SL64)
> -    tmp = (fp->_seek64 (ptr, fp->_cookie, curoff, SEEK_SET) == curoff);
> +    curoff = fp->_seek64 (ptr, fp->_cookie, curoff, SEEK_SET);
>    else
>   #endif
> -    tmp = (fp->_seek (ptr, fp->_cookie, curoff, SEEK_SET) == curoff);
> -  if (tmp)
> +    curoff = fp->_seek (ptr, fp->_cookie, curoff, SEEK_SET);
> +  if (curoff != -1 || ptr->_errno == 0)
>      {
>        /* Seek successful.  We can clear read buffer now.  */
>        fp->_flags&= ~__SNPT;
> @@ -169,6 +171,7 @@ _DEFUN(_fflush_r, (ptr, fp),
>        fp->_p = fp->_bf._base;
>        if (fp->_flags&  __SOFF)
>   fp->_offset = curoff;
> +      ptr->_errno = tmp_errno;
>        if (HASUB (fp))
>   FREEUB (ptr, fp);
>      }
>
>
>

[Corinna Vinschen]

On Oct 27 15:14, Jeff Johnston wrote:

> On 23/10/09 09:08 AM, Corinna Vinschen wrote:
> >Hi,
> >
> >there was a short thread on the Cygwin list, in which it turned out that
> >fclose on certain Cygwin devices opened for readin could return an
> >error: http://cygwin.com/ml/cygwin/2009-10/msg00562.html
> >
> >I tracked it down to the _fflush_r function.  _fflush_r calls fp->seek,
> >basically like this:
> >
> >   curoff = fp->_seek (0, SEEK_CUR);
> >   curoff = -= fp->_r; // Take buffer position into account
> >   tmp = (fp->_seek (curoff, SEEK_SET) == curoff)
> >   if (tmp)
> >     // Success
> >   else
> >     // Failure
> >
> >This code ignores the possibility that the offset returned by seek does
> >not exactly match the desired position.  This result is no error
> >condition, especially when taking devices into account.  It's especially
> >no error if lseek returns a negative offset on character special
> >devices.
> >Please note that lseek on the Cygwin devices mentioned in the above
> >thread behave exactly like their Linux counterparts.  Thus, the same
> >_fseek_r would also treat the Linux behaviour as error.
> >[...]
>
> So for these devices, you're saying seek is supported, but they
> cannot return their position (correct)?  Otherwise, if seek isn't
> supported, why don't they set ESPIPE?

Per POSIX, ESPIPE has to be returned if the descriptor references
a pipe, a fifo, or a socket.  POSIX does not require this for block
or character special devices.  Quote:

  The behavior of lseek() on devices which are incapable of seeking is
  implementation-defined. The value of the file offset associated with
  such a device is undefined.

Please note that Cygwin tries to emulate Linux behaviour in the first
place.  So the Cygwin devices behave just like the same Linux devices.

Try this code on Linux...

  #include <stdio.h>
  #include <sys/fcntl.h>
  #include <string.h>
  #include <errno.h>

  int
  main (int argc, char **argv)
  {
    int fd = open (argv[1], O_RDONLY);
    if (fd >= 0)
      {
        char buf[65536];
        off_t pos = lseek (fd, 0, SEEK_CUR);
        printf ("pos(0,CUR): %lld\n", (long long) pos);
        pos = read (fd, buf, 65536);
        printf ("pos(read): %lld\n", (long long) pos);
        pos = lseek (fd, 0, SEEK_CUR);
        printf ("pos(0, CUR): %lld\n", (long long) pos);
        pos = lseek (fd, -65528, SEEK_CUR);
        printf ("pos(-65528,CUR): %lld\n", (long long) pos);
        pos = lseek (fd, 0, SEEK_CUR);
        printf ("pos(0, CUR): %lld\n", (long long) pos);
        close (fd);
      }
    else
      printf ("open: %d <%s>\n", errno, strerror (errno));
    return 0;
  }

...with the following devices:

  $ gcc -o seektest seektest.c

  $ ./seektest /dev/urandom
  pos(0,CUR): 0
  pos(read): 65536
  pos(0, CUR): 0
  pos(-65528,CUR): -65528
  pos(0, CUR): -65528

  $ ./seektest /dev/zero
  pos(0,CUR): 0
  pos(read): 65536
  pos(0, CUR): 0
  pos(-65528,CUR): 0
  pos(0, CUR): 0

  $ ./seektest /dev/full
  pos(0,CUR): 0
  pos(read): 65536
  pos(0, CUR): 0
  pos(-65528,CUR): 0
  pos(0, CUR): 0

As you can see, lseek() on these devices does not return an error.
However, the position returned to the calling function doesn't match the
position you'd expect if you made the same operation on a file.

> Above the code you are changing is a check that either takes the
> file offset that has been tracked or tries to calculate it.  If it
> tries to calculate and -1 is returned, it returns 0 if ESPIPE,
> otherwise, it returns failure (it fails to reset errno in the case
> of the ignored ESPIPE).
>
> Should that logic be changed to match yours or should your new logic
> do the same? (i.e. treat ESPIPE as ok and otherwise -1 is failure).

I'm not sure I can follow.  How are you going to change that code if
the curoff value doesn't reflect an error condition at all?!?


Corinna

--
Corinna Vinschen
Cygwin Project Co-Leader
Red Hat

[Jeff Johnston]

On 27/10/09 03:49 PM, Corinna Vinschen wrote:

> On Oct 27 15:14, Jeff Johnston wrote:
>> On 23/10/09 09:08 AM, Corinna Vinschen wrote:
>>> Hi,
>>>
>>> there was a short thread on the Cygwin list, in which it turned out that
>>> fclose on certain Cygwin devices opened for readin could return an
>>> error: http://cygwin.com/ml/cygwin/2009-10/msg00562.html
>>>
>>> I tracked it down to the _fflush_r function.  _fflush_r calls fp->seek,
>>> basically like this:
>>>
>>>    curoff = fp->_seek (0, SEEK_CUR);
>>>    curoff = -= fp->_r; // Take buffer position into account
>>>    tmp = (fp->_seek (curoff, SEEK_SET) == curoff)
>>>    if (tmp)
>>>      // Success
>>>    else
>>>      // Failure
>>>
>>> This code ignores the possibility that the offset returned by seek does
>>> not exactly match the desired position.  This result is no error
>>> condition, especially when taking devices into account.  It's especially
>>> no error if lseek returns a negative offset on character special
>>> devices.
>>> Please note that lseek on the Cygwin devices mentioned in the above
>>> thread behave exactly like their Linux counterparts.  Thus, the same
>>> _fseek_r would also treat the Linux behaviour as error.
>>> [...]
>>
>> So for these devices, you're saying seek is supported, but they
>> cannot return their position (correct)?  Otherwise, if seek isn't
>> supported, why don't they set ESPIPE?
>
> Per POSIX, ESPIPE has to be returned if the descriptor references
> a pipe, a fifo, or a socket.  POSIX does not require this for block
> or character special devices.  Quote:
>
>    The behavior of lseek() on devices which are incapable of seeking is
>    implementation-defined. The value of the file offset associated with
>    such a device is undefined.
>

Glibc adds to this under info lseek:

     `ESPIPE'
           The FILEDES corresponds to an object that cannot be
           positioned, such as a pipe, FIFO or terminal device.
           (POSIX.1 specifies this error only for pipes and FIFOs, but
           in the GNU system, you always get `ESPIPE' if the object is
           not seekable.)

I interpret this that Linux always return ESPIPE for unseekable devices.

> Please note that Cygwin tries to emulate Linux behaviour in the first
> place.  So the Cygwin devices behave just like the same Linux devices.
>
> Try this code on Linux...
>
>    #include<stdio.h>
>    #include<sys/fcntl.h>
>    #include<string.h>
>    #include<errno.h>
>
>    int
>    main (int argc, char **argv)
>    {
>      int fd = open (argv[1], O_RDONLY);
>      if (fd>= 0)
>        {
> char buf[65536];
> off_t pos = lseek (fd, 0, SEEK_CUR);
> printf ("pos(0,CUR): %lld\n", (long long) pos);
> pos = read (fd, buf, 65536);
> printf ("pos(read): %lld\n", (long long) pos);
> pos = lseek (fd, 0, SEEK_CUR);
> printf ("pos(0, CUR): %lld\n", (long long) pos);
> pos = lseek (fd, -65528, SEEK_CUR);
> printf ("pos(-65528,CUR): %lld\n", (long long) pos);
> pos = lseek (fd, 0, SEEK_CUR);
> printf ("pos(0, CUR): %lld\n", (long long) pos);
> close (fd);
>        }
>      else
>        printf ("open: %d<%s>\n", errno, strerror (errno));
>      return 0;
>    }
>
> ...with the following devices:
>
>    $ gcc -o seektest seektest.c
>
>    $ ./seektest /dev/urandom
>    pos(0,CUR): 0
>    pos(read): 65536
>    pos(0, CUR): 0
>    pos(-65528,CUR): -65528
>    pos(0, CUR): -65528
>
>    $ ./seektest /dev/zero
>    pos(0,CUR): 0
>    pos(read): 65536
>    pos(0, CUR): 0
>    pos(-65528,CUR): 0
>    pos(0, CUR): 0
>
>    $ ./seektest /dev/full
>    pos(0,CUR): 0
>    pos(read): 65536
>    pos(0, CUR): 0
>    pos(-65528,CUR): 0
>    pos(0, CUR): 0
>
> As you can see, lseek() on these devices does not return an error.
> However, the position returned to the calling function doesn't match the
> position you'd expect if you made the same operation on a file.
>

I have no issue with removing the check for position equality.  I get a
slightly different result on Fedora Linux with /dev/urandom.  I get back
-1 and errno set to 22 (Invalid argument) for the seek of -65528.

>> Above the code you are changing is a check that either takes the
>> file offset that has been tracked or tries to calculate it.  If it
>> tries to calculate and -1 is returned, it returns 0 if ESPIPE,
>> otherwise, it returns failure (it fails to reset errno in the case
>> of the ignored ESPIPE).
>>
>> Should that logic be changed to match yours or should your new logic
>> do the same? (i.e. treat ESPIPE as ok and otherwise -1 is failure).
>
> I'm not sure I can follow.  How are you going to change that code if
> the curoff value doesn't reflect an error condition at all?!?
>

Your change allows -1 to be returned if no errno is set.  None of the
above tests exhibit this, but you may know differently.  If -1 and
errno-not-set is possible, then the earlier check in fflush where -1
returned from seek causes failure unless ESPIPE is set, might require
similar checking to what you are proposing.

There might be an issue if any libgloss implementation does not properly
set errno on an lseek failure, but I know none off-hand and have only
checked a handful of platforms.

If an unseekable device returns -1 and sets ESPIPE, shouldn't you ignore
this situation as is done above in the same code? (i.e. there is no
requirement to do a seek once the read buffer is cleared).

So one possible solution would be to change the added check to be (rc !=
-1 || ptr->errno == ESPIPE || ptr->errno == 0), with the last check for
errno being removed if it is not feasible that -1 will be returned with
errno 0.

-- Jeff J.

[Corinna Vinschen]

On Oct 27 17:09, Jeff Johnston wrote:

> On 27/10/09 03:49 PM, Corinna Vinschen wrote:
> >   The behavior of lseek() on devices which are incapable of seeking is
> >   implementation-defined. The value of the file offset associated with
> >   such a device is undefined.
>
> Glibc adds to this under info lseek:
>
>     `ESPIPE'
>           The FILEDES corresponds to an object that cannot be
>           positioned, such as a pipe, FIFO or terminal device.
>           (POSIX.1 specifies this error only for pipes and FIFOs, but
>           in the GNU system, you always get `ESPIPE' if the object is
>           not seekable.)
>
> I interpret this that Linux always return ESPIPE for unseekable devices.

Yeah, it seems so, but testing the actual behaviour leads to a
surprising result.  See below.

> >Please note that Cygwin tries to emulate Linux behaviour in the first
> >place.  So the Cygwin devices behave just like the same Linux devices.
> >
> >Try this code on Linux...
> >[...]
> >As you can see, lseek() on these devices does not return an error.
> >However, the position returned to the calling function doesn't match the
> >position you'd expect if you made the same operation on a file.
> >
> I have no issue with removing the check for position equality.  I
> get a slightly different result on Fedora Linux with /dev/urandom.
> I get back -1 and errno set to 22 (Invalid argument) for the seek of
> -65528.

Indeed.  My tests were performed on a 2.6.27.x kernel.  On a 2.6.30.x
kernel I get the same result as you.  And the errno returned is not
ESPIPE, but EINVAL.  The joke is, this was the original behaviour of
Cygwin's /dev/urandom as well, until I changed it a couple of days ago.

EINVAL is a problem for newlib's fflush, no matter if with or without
my patch.  Here's why:

Imagine this code:

  fp = fopen ("/dev/urandom", "r");
  read (fp, buf, 100);
  fclose (fp);

The result is that fclose returns EOF with errno set to EINVAL.  That's
what happens in _fflush_r, assuming the default buffer size is 4096.

  curoff = fp->_seek (ptr, fp->_cookie, 0, SEEK_CUR);
  ==> curoff = 0   since that's what gets returned by /dev/urandom

  curoff -= fp->_r;
  ==> curoff = -3996

  Without my patch:

  tmp = (fp->_seek (ptr, fp->_cookie, curoff, SEEK_SET) == curoff);
  ==> tmp = 0 ==> return EOF;

  With my patch:

  curoff = fp->_seek (ptr, fp->_cookie, curoff, SEEK_SET);
  ==> curoff == -1, errno = EINVAL ==> return EOF;

If this problem gets fixed as well, I can revert my change in Cygwin and
Cygwin's /dev/urandom can behave as in the newer Linux kernels again.

> >>Should that logic be changed to match yours or should your new logic
> >>do the same? (i.e. treat ESPIPE as ok and otherwise -1 is failure).
> >
> >I'm not sure I can follow.  How are you going to change that code if
> >the curoff value doesn't reflect an error condition at all?!?
> >
>
> Your change allows -1 to be returned if no errno is set.  None of
> the above tests exhibit this, but you may know differently.

Yes, it is possible for lseek to return -1 with errno set to 0.
If you try this:

  errno = 0;
  pos = lseek (fd, -1, SEEK_CUR);
  printf ("pos(-1, CUR): %lld (errno = %d)\n", (long long) pos, errno);
  errno = 0;
  pos = lseek (fd, 0, SEEK_CUR);
  printf ("pos(0, CUR): %lld (errno = %d)\n", (long long) pos, errno);

on /dev/zero, you get this output:

  pos(-1, CUR): 0 (errno = 0)
  pos(0, CUR): 0 (errno = 0)

and on /dev/urandom with a 2.6.27.x kernel:

  pos(-1, CUR): -1 (errno = 0)
  pos(0, CUR): -1 (errno = 0)

Quote from POSIX-1.2008:

  The POSIX.1-1990 standard did not specifically prohibit lseek() from
  returning a negative offset. Therefore, an application was required to
  clear errno prior to the call and check errno upon return to determine
  whether a return value of ( off_t)-1 is a negative offset or an
  indication of an error condition. The standard developers did not wish
  to require this action on the part of a conforming application, and
  chose to require that errno be set to [EINVAL] when the resulting file
  offset would be negative for a regular file, block special file, or
  directory.

So POSIX takes character special files still out of the picture.

> If -1
> and errno-not-set is possible, then the earlier check in fflush
> where -1 returned from seek causes failure unless ESPIPE is set,
> might require similar checking to what you are proposing.

Yes, you're right.

> There might be an issue if any libgloss implementation does not
> properly set errno on an lseek failure, but I know none off-hand and
> have only checked a handful of platforms.

Not setting errno on an error condition would be a bug, though.

> If an unseekable device returns -1 and sets ESPIPE, shouldn't you
> ignore this situation as is done above in the same code? (i.e. there
> is no requirement to do a seek once the read buffer is cleared).

Yes, good idea.

However, this doesn't help in the case in which the device returns
EINVAL on negative offsets, as outlined above.  Wouldn't it make sense
to check curoff for being negative before trying to seek to this
position?  I can only imagine two situations in which this occurs:

- A non-seekable device, so seeking makes no sense anyway, or

- The application has called lseek on the file at one point, so the
  seek in fflush would lead to wrong results anyway.  It's sufficient
  to flush the FILE content without seeking in this case.

I created a new patch based on this discussion.  See below.


Corinna


        * libc/stdio/fflush.c (_fflush_r): Store old errno to check for
        low-level seek error condition.  Restore old errno in case of
        success.  Don't use new position after seek as error condition,
        rather check for return value of -1 and errno.  Only seek to
        positive offsets, explain why.  Only set fp->_offset if errno
        is not ESPIPE.


Index: libc/stdio/fflush.c
===================================================================
RCS file: /cvs/src/src/newlib/libc/stdio/fflush.c,v
retrieving revision 1.14
diff -u -p -r1.14 fflush.c
--- libc/stdio/fflush.c 22 Jul 2009 02:17:12 -0000 1.14
+++ libc/stdio/fflush.c 28 Oct 2009 10:00:20 -0000
@@ -115,13 +115,18 @@ _DEFUN(_fflush_r, (ptr, fp),
          to miss a code scenario.  */
       if ((fp->_r > 0 || fp->_ur > 0) && fp->_seek != NULL)
  {
-  int tmp;
+  int tmp_errno;
 #ifdef __LARGE64_FILES
   _fpos64_t curoff;
 #else
   _fpos_t curoff;
 #endif
 
+  /* Save last errno and set errno to 0, so we can check if a device
+     returns with a valid position -1.  We restore the last errno if
+     no other error condition has been encountered. */
+  tmp_errno = ptr->_errno;
+  ptr->_errno = 0;
   /* Get the physical position we are at in the file.  */
   if (fp->_flags & __SOFF)
     curoff = fp->_offset;
@@ -135,11 +140,14 @@ _DEFUN(_fflush_r, (ptr, fp),
       else
 #endif
  curoff = fp->_seek (ptr, fp->_cookie, 0, SEEK_CUR);
-      if (curoff == -1L)
+      if (curoff == -1L && ptr->_errno != 0)
  {
   int result = EOF;
   if (ptr->_errno == ESPIPE)
-    result = 0;
+    {
+      result = 0;
+      ptr->_errno = tmp_errno;
+    }
   else
     fp->_flags |= __SERR;
   _funlockfile (fp);
@@ -154,21 +162,29 @@ _DEFUN(_fflush_r, (ptr, fp),
               if (HASUB (fp))
                 curoff -= fp->_ur;
             }
-  /* Now physically seek to after byte last read.  */
+  /* Now physically seek to after byte last read.
+     If curoff is < 0, fp is either pointing to a non-seekable device,
+     or the application already called lseek and invalidated the
+     information in fp.  Seeking doesn't make sense in this case.  */
+  if (curoff >= 0)
+    {
 #ifdef __LARGE64_FILES
-  if (fp->_flags & __SL64)
-    tmp = (fp->_seek64 (ptr, fp->_cookie, curoff, SEEK_SET) == curoff);
-  else
+      if (fp->_flags & __SL64)
+ curoff = fp->_seek64 (ptr, fp->_cookie, curoff, SEEK_SET);
+      else
 #endif
-    tmp = (fp->_seek (ptr, fp->_cookie, curoff, SEEK_SET) == curoff);
-  if (tmp)
+ curoff = fp->_seek (ptr, fp->_cookie, curoff, SEEK_SET);
+    }
+  if (curoff != -1 || ptr->_errno == 0 || ptr->_errno == ESPIPE)
     {
       /* Seek successful.  We can clear read buffer now.  */
       fp->_flags &= ~__SNPT;
       fp->_r = 0;
       fp->_p = fp->_bf._base;
-      if (fp->_flags & __SOFF)
+      if ((fp->_flags & __SOFF)
+  && (curoff != -1 || ptr->_errno != ESPIPE))
  fp->_offset = curoff;
+      ptr->_errno = tmp_errno;
       if (HASUB (fp))
  FREEUB (ptr, fp);
     }


--
Corinna Vinschen
Cygwin Project Co-Leader
Red Hat

[Corinna Vinschen]

On Oct 28 11:06, Corinna Vinschen wrote:
> * libc/stdio/fflush.c (_fflush_r): Store old errno to check for
> low-level seek error condition.  Restore old errno in case of
> success.  Don't use new position after seek as error condition,
> rather check for return value of -1 and errno.  Only seek to
> positive offsets, explain why.  Only set fp->_offset if errno
> is not ESPIPE.

There's a still a problem with this patch.  Assuming the offset returned
by the first seek on a regular file would result in a negative offset
after fp->_r and fp->_ur have been subtracted, then the new position
stored in fp->_offset will be incorrect.  Therefore, the code must check
for this condition before changing curoff.  The below patch does this,
so fp->_offset will not be set to an incorrect value.

Alternatively, I'm wondering if it isn't easier to ignore the EINVAL
condition in the second invocation of seek just like ESPIPE.


Corinna


Index: libc/stdio/fflush.c
===================================================================
RCS file: /cvs/src/src/newlib/libc/stdio/fflush.c,v
retrieving revision 1.14
diff -u -p -r1.14 fflush.c
--- libc/stdio/fflush.c 22 Jul 2009 02:17:12 -0000 1.14
+++ libc/stdio/fflush.c 28 Oct 2009 10:44:48 -0000
@@ -115,13 +115,18 @@ _DEFUN(_fflush_r, (ptr, fp),
          to miss a code scenario.  */
       if ((fp->_r > 0 || fp->_ur > 0) && fp->_seek != NULL)
  {
-  int tmp;
+  int tmp_errno;
 #ifdef __LARGE64_FILES
   _fpos64_t curoff;
 #else
   _fpos_t curoff;
 #endif
 
+  /* Save last errno and set errno to 0, so we can check if a device
+     returns with a valid position -1.  We restore the last errno if
+     no other error condition has been encountered. */
+  tmp_errno = ptr->_errno;
+  ptr->_errno = 0;
   /* Get the physical position we are at in the file.  */
   if (fp->_flags & __SOFF)
     curoff = fp->_offset;
@@ -135,11 +140,14 @@ _DEFUN(_fflush_r, (ptr, fp),
       else
 #endif
  curoff = fp->_seek (ptr, fp->_cookie, 0, SEEK_CUR);
-      if (curoff == -1L)
+      if (curoff == -1L && ptr->_errno != 0)
  {
   int result = EOF;
   if (ptr->_errno == ESPIPE)
-    result = 0;
+    {
+      result = 0;
+      ptr->_errno = tmp_errno;
+    }
   else
     fp->_flags |= __SERR;
   _funlockfile (fp);
@@ -148,27 +156,44 @@ _DEFUN(_fflush_r, (ptr, fp),
             }
           if (fp->_flags & __SRD)
             {
+#ifdef __LARGE64_FILES
+      _fpos64_t newoff = curoff;
+#else
+      _fpos_t newoff = curoff;
+#endif
               /* Current offset is at end of buffer.  Compensate for
                  characters not yet read.  */
-              curoff -= fp->_r;
+              newoff -= fp->_r;
               if (HASUB (fp))
-                curoff -= fp->_ur;
+                newoff -= fp->_ur;
+      /* Make sure that curoff still reflects the current file
+         position if we don't seek.  */
+      if (newoff >= 0)
+ curoff = newoff;
             }
-  /* Now physically seek to after byte last read.  */
+  /* Now physically seek to after byte last read.
+     If curoff is < 0, fp is either pointing to a non-seekable device,
+     or the application already called lseek and invalidated the
+     information in fp.  Seeking doesn't make sense in this case.  */
+  if (curoff >= 0)
+    {
 #ifdef __LARGE64_FILES
-  if (fp->_flags & __SL64)
-    tmp = (fp->_seek64 (ptr, fp->_cookie, curoff, SEEK_SET) == curoff);
-  else
+      if (fp->_flags & __SL64)
+ curoff = fp->_seek64 (ptr, fp->_cookie, curoff, SEEK_SET);
+      else
 #endif
-    tmp = (fp->_seek (ptr, fp->_cookie, curoff, SEEK_SET) == curoff);
-  if (tmp)
+ curoff = fp->_seek (ptr, fp->_cookie, curoff, SEEK_SET);
+    }
+  if (curoff != -1 || ptr->_errno == 0 || ptr->_errno == ESPIPE)
     {
       /* Seek successful.  We can clear read buffer now.  */
       fp->_flags &= ~__SNPT;
       fp->_r = 0;
       fp->_p = fp->_bf._base;
-      if (fp->_flags & __SOFF)
+      if ((fp->_flags & __SOFF)
+  && (curoff != -1 || ptr->_errno != ESPIPE))
  fp->_offset = curoff;
+      ptr->_errno = tmp_errno;
       if (HASUB (fp))
  FREEUB (ptr, fp);
     }

--
Corinna Vinschen
Cygwin Project Co-Leader
Red Hat

[Jeff Johnston]

On 28/10/09 06:47 AM, Corinna Vinschen wrote:

> On Oct 28 11:06, Corinna Vinschen wrote:
>> * libc/stdio/fflush.c (_fflush_r): Store old errno to check for
>> low-level seek error condition.  Restore old errno in case of
>> success.  Don't use new position after seek as error condition,
>> rather check for return value of -1 and errno.  Only seek to
>> positive offsets, explain why.  Only set fp->_offset if errno
>> is not ESPIPE.
>
> There's a still a problem with this patch.  Assuming the offset returned
> by the first seek on a regular file would result in a negative offset
> after fp->_r and fp->_ur have been subtracted, then the new position
> stored in fp->_offset will be incorrect.  Therefore, the code must check
> for this condition before changing curoff.  The below patch does this,
> so fp->_offset will not be set to an incorrect value.
>
> Alternatively, I'm wondering if it isn't easier to ignore the EINVAL
> condition in the second invocation of seek just like ESPIPE.
>

I think you need to ignore the EINVAL regardless as it doesn't indicate
the seek had a failure as much as ESPIPE doesn't.

-- Jeff J.

>
> Corinna
>
>
> Index: libc/stdio/fflush.c
> ===================================================================
> RCS file: /cvs/src/src/newlib/libc/stdio/fflush.c,v
> retrieving revision 1.14
> diff -u -p -r1.14 fflush.c
> --- libc/stdio/fflush.c 22 Jul 2009 02:17:12 -0000 1.14
> +++ libc/stdio/fflush.c 28 Oct 2009 10:44:48 -0000
> @@ -115,13 +115,18 @@ _DEFUN(_fflush_r, (ptr, fp),
>            to miss a code scenario.  */
>         if ((fp->_r>  0 || fp->_ur>  0)&&  fp->_seek != NULL)
>   {
> -  int tmp;
> +  int tmp_errno;
>   #ifdef __LARGE64_FILES
>    _fpos64_t curoff;
>   #else
>    _fpos_t curoff;
>   #endif
>
> +  /* Save last errno and set errno to 0, so we can check if a device
> +     returns with a valid position -1.  We restore the last errno if
> +     no other error condition has been encountered. */
> +  tmp_errno = ptr->_errno;
> +  ptr->_errno = 0;
>    /* Get the physical position we are at in the file.  */
>    if (fp->_flags&  __SOFF)
>      curoff = fp->_offset;
> @@ -135,11 +140,14 @@ _DEFUN(_fflush_r, (ptr, fp),
>        else
>   #endif
>   curoff = fp->_seek (ptr, fp->_cookie, 0, SEEK_CUR);
> -      if (curoff == -1L)
> +      if (curoff == -1L&&  ptr->_errno != 0)
>   {
>    int result = EOF;
>    if (ptr->_errno == ESPIPE)
> -    result = 0;
> +    {
> +      result = 0;
> +      ptr->_errno = tmp_errno;
> +    }
>    else
>      fp->_flags |= __SERR;
>    _funlockfile (fp);
> @@ -148,27 +156,44 @@ _DEFUN(_fflush_r, (ptr, fp),
>               }
>             if (fp->_flags&  __SRD)
>               {
> +#ifdef __LARGE64_FILES
> +      _fpos64_t newoff = curoff;
> +#else
> +      _fpos_t newoff = curoff;
> +#endif
>                 /* Current offset is at end of buffer.  Compensate for
>                    characters not yet read.  */
> -              curoff -= fp->_r;
> +              newoff -= fp->_r;
>                 if (HASUB (fp))
> -                curoff -= fp->_ur;
> +                newoff -= fp->_ur;
> +      /* Make sure that curoff still reflects the current file
> +         position if we don't seek.  */
> +      if (newoff>= 0)
> + curoff = newoff;
>               }
> -  /* Now physically seek to after byte last read.  */
> +  /* Now physically seek to after byte last read.
> +     If curoff is<  0, fp is either pointing to a non-seekable device,
> +     or the application already called lseek and invalidated the
> +     information in fp.  Seeking doesn't make sense in this case.  */
> +  if (curoff>= 0)
> +    {
>   #ifdef __LARGE64_FILES
> -  if (fp->_flags&  __SL64)
> -    tmp = (fp->_seek64 (ptr, fp->_cookie, curoff, SEEK_SET) == curoff);
> -  else
> +      if (fp->_flags&  __SL64)
> + curoff = fp->_seek64 (ptr, fp->_cookie, curoff, SEEK_SET);
> +      else
>   #endif
> -    tmp = (fp->_seek (ptr, fp->_cookie, curoff, SEEK_SET) == curoff);
> -  if (tmp)
> + curoff = fp->_seek (ptr, fp->_cookie, curoff, SEEK_SET);
> +    }
> +  if (curoff != -1 || ptr->_errno == 0 || ptr->_errno == ESPIPE)
>      {
>        /* Seek successful.  We can clear read buffer now.  */
>        fp->_flags&= ~__SNPT;
>        fp->_r = 0;
>        fp->_p = fp->_bf._base;
> -      if (fp->_flags&  __SOFF)
> +      if ((fp->_flags&  __SOFF)
> + &&  (curoff != -1 || ptr->_errno != ESPIPE))
>   fp->_offset = curoff;
> +      ptr->_errno = tmp_errno;
>        if (HASUB (fp))
>   FREEUB (ptr, fp);
>      }
>

[Corinna Vinschen]

On Oct 28 11:56, Jeff Johnston wrote:

> On 28/10/09 06:47 AM, Corinna Vinschen wrote:
> >On Oct 28 11:06, Corinna Vinschen wrote:
> >> * libc/stdio/fflush.c (_fflush_r): Store old errno to check for
> >> low-level seek error condition.  Restore old errno in case of
> >> success.  Don't use new position after seek as error condition,
> >> rather check for return value of -1 and errno.  Only seek to
> >> positive offsets, explain why.  Only set fp->_offset if errno
> >> is not ESPIPE.
> >
> >There's a still a problem with this patch.  Assuming the offset returned
> >by the first seek on a regular file would result in a negative offset
> >after fp->_r and fp->_ur have been subtracted, then the new position
> >stored in fp->_offset will be incorrect.  Therefore, the code must check
> >for this condition before changing curoff.  The below patch does this,
> >so fp->_offset will not be set to an incorrect value.
> >
> >Alternatively, I'm wondering if it isn't easier to ignore the EINVAL
> >condition in the second invocation of seek just like ESPIPE.
> >
>
> I think you need to ignore the EINVAL regardless as it doesn't
> indicate the seek had a failure as much as ESPIPE doesn't.

Ok, no worries, but in that case it doesn't make sense to avoid offsets
< 0.  This case is sufficiently covered by checking for EINVAL.  Patch
below.


Corinna

        * libc/stdio/fflush.c (_fflush_r): Store old errno to check for
        low-level seek error condition.  Restore old errno in case of
        success.  Don't use new position after seek as error condition,
        rather check for return value of -1 and errno.  Handle EINVAL
        just like ESPIPE.  Only set fp->_offset if errno is 0.


Index: libc/stdio/fflush.c
===================================================================
RCS file: /cvs/src/src/newlib/libc/stdio/fflush.c,v
retrieving revision 1.14
diff -u -p -r1.14 fflush.c
--- libc/stdio/fflush.c 22 Jul 2009 02:17:12 -0000 1.14
+++ libc/stdio/fflush.c 29 Oct 2009 10:23:28 -0000
@@ -115,31 +115,39 @@ _DEFUN(_fflush_r, (ptr, fp),
          to miss a code scenario.  */
       if ((fp->_r > 0 || fp->_ur > 0) && fp->_seek != NULL)
  {
-  int tmp;
+  int tmp_errno;
 #ifdef __LARGE64_FILES
   _fpos64_t curoff;
 #else
   _fpos_t curoff;
 #endif
 
+  /* Save last errno and set errno to 0, so we can check if a device
+     returns with a valid position -1.  We restore the last errno if
+     no other error condition has been encountered. */
+  tmp_errno = ptr->_errno;
+  ptr->_errno = 0;
   /* Get the physical position we are at in the file.  */
   if (fp->_flags & __SOFF)
     curoff = fp->_offset;
   else
     {
       /* We don't know current physical offset, so ask for it.
- Only ESPIPE is ignorable.  */
+ Only ESPIPE and EINVAL are ignorable.  */
 #ifdef __LARGE64_FILES
       if (fp->_flags & __SL64)
  curoff = fp->_seek64 (ptr, fp->_cookie, 0, SEEK_CUR);
       else
 #endif
  curoff = fp->_seek (ptr, fp->_cookie, 0, SEEK_CUR);
-      if (curoff == -1L)
+      if (curoff == -1L && ptr->_errno != 0)
  {
   int result = EOF;
-  if (ptr->_errno == ESPIPE)
-    result = 0;
+  if (ptr->_errno == ESPIPE || ptr->_errno == EINVAL)
+    {
+      result = 0;
+      ptr->_errno = tmp_errno;
+    }
   else
     fp->_flags |= __SERR;
   _funlockfile (fp);
@@ -157,18 +165,20 @@ _DEFUN(_fflush_r, (ptr, fp),
   /* Now physically seek to after byte last read.  */
 #ifdef __LARGE64_FILES
   if (fp->_flags & __SL64)
-    tmp = (fp->_seek64 (ptr, fp->_cookie, curoff, SEEK_SET) == curoff);
+    curoff = fp->_seek64 (ptr, fp->_cookie, curoff, SEEK_SET);
   else
 #endif
-    tmp = (fp->_seek (ptr, fp->_cookie, curoff, SEEK_SET) == curoff);
-  if (tmp)
+    curoff = fp->_seek (ptr, fp->_cookie, curoff, SEEK_SET);
+  if (curoff != -1 || ptr->_errno == 0
+      || ptr->_errno == ESPIPE || ptr->_errno == EINVAL)
     {
       /* Seek successful.  We can clear read buffer now.  */
       fp->_flags &= ~__SNPT;
       fp->_r = 0;
       fp->_p = fp->_bf._base;
-      if (fp->_flags & __SOFF)
+      if ((fp->_flags & __SOFF) && (curoff != -1 || ptr->_errno == 0))
  fp->_offset = curoff;
+      ptr->_errno = tmp_errno;
       if (HASUB (fp))
  FREEUB (ptr, fp);
     }


--
Corinna Vinschen
Cygwin Project Co-Leader
Red Hat

[Jeff Johnston]

On 29/10/09 06:24 AM, Corinna Vinschen wrote:

> On Oct 28 11:56, Jeff Johnston wrote:
>> On 28/10/09 06:47 AM, Corinna Vinschen wrote:
>>> On Oct 28 11:06, Corinna Vinschen wrote:
>>>> * libc/stdio/fflush.c (_fflush_r): Store old errno to check for
>>>> low-level seek error condition.  Restore old errno in case of
>>>> success.  Don't use new position after seek as error condition,
>>>> rather check for return value of -1 and errno.  Only seek to
>>>> positive offsets, explain why.  Only set fp->_offset if errno
>>>> is not ESPIPE.
>>>
>>> There's a still a problem with this patch.  Assuming the offset returned
>>> by the first seek on a regular file would result in a negative offset
>>> after fp->_r and fp->_ur have been subtracted, then the new position
>>> stored in fp->_offset will be incorrect.  Therefore, the code must check
>>> for this condition before changing curoff.  The below patch does this,
>>> so fp->_offset will not be set to an incorrect value.
>>>
>>> Alternatively, I'm wondering if it isn't easier to ignore the EINVAL
>>> condition in the second invocation of seek just like ESPIPE.
>>>
>>
>> I think you need to ignore the EINVAL regardless as it doesn't
>> indicate the seek had a failure as much as ESPIPE doesn't.
>
> Ok, no worries, but in that case it doesn't make sense to avoid offsets
> <  0.  This case is sufficiently covered by checking for EINVAL.  Patch
> below.
>

Looks fine.  Feel free to commit.

-- Jeff J.

>
> Corinna
>
> * libc/stdio/fflush.c (_fflush_r): Store old errno to check for
> low-level seek error condition.  Restore old errno in case of
> success.  Don't use new position after seek as error condition,
> rather check for return value of -1 and errno.  Handle EINVAL
> just like ESPIPE.  Only set fp->_offset if errno is 0.
>
>
> Index: libc/stdio/fflush.c
> ===================================================================
> RCS file: /cvs/src/src/newlib/libc/stdio/fflush.c,v
> retrieving revision 1.14
> diff -u -p -r1.14 fflush.c
> --- libc/stdio/fflush.c 22 Jul 2009 02:17:12 -0000 1.14
> +++ libc/stdio/fflush.c 29 Oct 2009 10:23:28 -0000
> @@ -115,31 +115,39 @@ _DEFUN(_fflush_r, (ptr, fp),
>            to miss a code scenario.  */
>         if ((fp->_r>  0 || fp->_ur>  0)&&  fp->_seek != NULL)
>   {
> -  int tmp;
> +  int tmp_errno;
>   #ifdef __LARGE64_FILES
>    _fpos64_t curoff;
>   #else
>    _fpos_t curoff;
>   #endif
>
> +  /* Save last errno and set errno to 0, so we can check if a device
> +     returns with a valid position -1.  We restore the last errno if
> +     no other error condition has been encountered. */
> +  tmp_errno = ptr->_errno;
> +  ptr->_errno = 0;
>    /* Get the physical position we are at in the file.  */
>    if (fp->_flags&  __SOFF)
>      curoff = fp->_offset;
>    else
>      {
>        /* We don't know current physical offset, so ask for it.
> - Only ESPIPE is ignorable.  */
> + Only ESPIPE and EINVAL are ignorable.  */
>   #ifdef __LARGE64_FILES
>        if (fp->_flags&  __SL64)
>   curoff = fp->_seek64 (ptr, fp->_cookie, 0, SEEK_CUR);
>        else
>   #endif
>   curoff = fp->_seek (ptr, fp->_cookie, 0, SEEK_CUR);
> -      if (curoff == -1L)
> +      if (curoff == -1L&&  ptr->_errno != 0)
>   {
>    int result = EOF;
> -  if (ptr->_errno == ESPIPE)
> -    result = 0;
> +  if (ptr->_errno == ESPIPE || ptr->_errno == EINVAL)
> +    {
> +      result = 0;
> +      ptr->_errno = tmp_errno;
> +    }
>    else
>      fp->_flags |= __SERR;
>    _funlockfile (fp);
> @@ -157,18 +165,20 @@ _DEFUN(_fflush_r, (ptr, fp),
>    /* Now physically seek to after byte last read.  */
>   #ifdef __LARGE64_FILES
>    if (fp->_flags&  __SL64)
> -    tmp = (fp->_seek64 (ptr, fp->_cookie, curoff, SEEK_SET) == curoff);
> +    curoff = fp->_seek64 (ptr, fp->_cookie, curoff, SEEK_SET);
>    else
>   #endif
> -    tmp = (fp->_seek (ptr, fp->_cookie, curoff, SEEK_SET) == curoff);
> -  if (tmp)
> +    curoff = fp->_seek (ptr, fp->_cookie, curoff, SEEK_SET);
> +  if (curoff != -1 || ptr->_errno == 0
> +      || ptr->_errno == ESPIPE || ptr->_errno == EINVAL)
>      {
>        /* Seek successful.  We can clear read buffer now.  */
>        fp->_flags&= ~__SNPT;
>        fp->_r = 0;
>        fp->_p = fp->_bf._base;
> -      if (fp->_flags&  __SOFF)
> +      if ((fp->_flags&  __SOFF)&&  (curoff != -1 || ptr->_errno == 0))
>   fp->_offset = curoff;
> +      ptr->_errno = tmp_errno;
>        if (HASUB (fp))
>   FREEUB (ptr, fp);
>      }
>
>

[Corinna Vinschen]

On Oct 29 15:59, Jeff Johnston wrote:

> On 29/10/09 06:24 AM, Corinna Vinschen wrote:
> >On Oct 28 11:56, Jeff Johnston wrote:
> >>I think you need to ignore the EINVAL regardless as it doesn't
> >>indicate the seek had a failure as much as ESPIPE doesn't.
> >
> >Ok, no worries, but in that case it doesn't make sense to avoid offsets
> ><  0.  This case is sufficiently covered by checking for EINVAL.  Patch
> >below.
> >
>
> Looks fine.  Feel free to commit.

Done, thank you.  Additionally I expanded the comment

  /* Seek successful.  We can clear read buffer now.  */

to

  /* Seek successful or ignorable error condition.
     We can clear read buffer now.  */

to be more clear.


Corinna

--
Corinna Vinschen
Cygwin Project Co-Leader
Red Hat