Re: [PATCH/RFA] _fflush_r check seek result fix

[Corinna Vinschen <vinschen at redhat dot com>]

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