Re: atomic Xprintf

[Jeff Johnston <jjohnstn at redhat dot com>]

Jeff Johnston wrote:
> Brian Ford wrote:
>
> > This Cygwin bug:
> >
> > http://www.cygwin.com/ml/cygwin/2004-05/msg00765.html
> >
> > appears to be caused by these changes:
> >
> > 2003-08-22  Jeff Johnston  <jjohnstn at redhat dot com>
> >
> >         * libc/stdio/vasprintf.c: Ditto.  Also call _vfprintf_r directly.
> >         * libc/stdio/vsnprintf.c: Ditto.
> >         * libc/stdio/vsprintf.c: Ditto.
> >         * libc/stdio/snprintf.c: Call _vfprintf_r directly.
> >         * libc/stdio/sprintf.c: Ditto.
> >         * libc/stdio/vprintf.c: Ditto.  Also add _REENT_ONLY check.
> >
> > Calling _vfprintf_r directly avoids the f[lock|unlock]file calls that
> > would happen if just _vfprintf were called.
> >
> > I don't understand the reasoning for this change.  Could someone please
> > explain?  Thanks.
>
> It was a case of eliminating an extraneous call as the _r routines in 
> the same files call _vfprintf_r directly.  It was obviously an oversight 
> on my part regarding the locking aspsect, however, it is equally 
> obvious, the locking is being performed in the wrong place.  While the 
> old call handled the locking for the regular I/O printf functions, the 
> fact remains that the _r versions of _vasprintf_r, etc.. all call 
> _vfprintf_r directly and they are not protected. Pushing the locking 
> down into _vfprintf_r should fix the problems.
>
> -- Jeff J.

Brian,

  Can you try out the attached patch and verify if it solves the problem?

-- Jeff J.
Index: vfprintf.c
===================================================================
RCS file: /cvs/src/src/newlib/libc/stdio/vfprintf.c,v
retrieving revision 1.35
diff -u -p -r1.35 vfprintf.c
--- vfprintf.c	26 May 2004 00:19:14 -0000	1.35
+++ vfprintf.c	26 May 2004 22:46:04 -0000
@@ -381,10 +381,7 @@ _DEFUN(VFPRINTF, (fp, fmt0, ap),
        va_list ap)
 {
   int result;
-  _flockfile (fp);
-  CHECK_INIT (fp);
   result = _VFPRINTF_R (_REENT, fp, fmt0, ap);
-  _funlockfile (fp);
   return result;
 }
 
@@ -536,14 +533,21 @@ _DEFUN(_VFPRINTF_R, (data, fp, fmt0, ap)
 	    (u_long)GET_ARG (N, ap, u_int))
 #endif
 
+	_flockfile (fp);
+	CHECK_INIT (fp);
+
 	/* sorry, fprintf(read_only_file, "") returns EOF, not 0 */
-	if (cantwrite (fp))
+	if (cantwrite (fp)) {
+		_funlockfile (fp);	
 		return (EOF);
+	}
 
 	/* optimise fprintf(stderr) (and other unbuffered Unix files) */
 	if ((fp->_flags & (__SNBF|__SWR|__SRW)) == (__SNBF|__SWR) &&
-	    fp->_file >= 0)
+	    fp->_file >= 0) {
+		_funlockfile (fp);
 		return (__sbprintf (data, fp, fmt0, ap));
+	}
 
 	fmt = (char *)fmt0;
 	uio.uio_iov = iovp = iov;
@@ -1211,6 +1215,7 @@ done:
 error:
 	if (malloc_buf != NULL)
 		_free_r (data, malloc_buf);
+	_funlockfile (fp);
 	return (__sferror (fp) ? EOF : ret);
 	/* NOTREACHED */
 }