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

[Jeff Johnston <jjohnstn at redhat dot com>]

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