This is the mail archive of the
newlib@sourceware.org
mailing list for the newlib project.
Re: [PATCH/RFA] _fflush_r check seek result fix
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