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

[Jeff Johnston <jjohnstn at redhat dot com>]

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);
>   	    }