Re: [PATCH] make fflush_unlocked(NULL) work

[Rich Felker <dalias@libc.org>]

[-- Attachment #1: Type: text/plain, Size: 2194 bytes --]

On Sun, Sep 18, 2016 at 09:44:55PM +0200, Denys Vlasenko wrote:
> In glibc, fflush_unlocked(NULL) works.
> Before this patch, musl was segfaulting.
> 
> Signed-off-by: Denys Vlasenko <vda.linux@googlemail.com>
> CC: musl <musl@lists.openwall.com>
> ---
>  src/stdio/fflush.c | 35 +++++++++++++++++++++++++----------
>  1 file changed, 25 insertions(+), 10 deletions(-)
> 
> diff --git a/src/stdio/fflush.c b/src/stdio/fflush.c
> index 3f462c8..06d7a56 100644
> --- a/src/stdio/fflush.c
> +++ b/src/stdio/fflush.c
> @@ -2,20 +2,35 @@
>  
>  static int __fflush_unlocked(FILE *f)
>  {
> -	/* If writing, flush output */
> -	if (f->wpos > f->wbase) {
> -		f->write(f, 0, 0);
> -		if (!f->wpos) return EOF;
> +	int r;
> +
> +	if (f) {
> +		/* If writing, flush output */
> +		if (f->wpos > f->wbase) {
> +			f->write(f, 0, 0);
> +			if (!f->wpos) return EOF;
> +		}
> +
> +		/* If reading, sync position, per POSIX */
> +		if (f->rpos < f->rend) f->seek(f, f->rpos-f->rend, SEEK_CUR);
> +
> +		/* Clear read and write modes */
> +		f->wpos = f->wbase = f->wend = 0;
> +		f->rpos = f->rend = 0;
> +
> +		return 0;
>  	}
>  
> -	/* If reading, sync position, per POSIX */
> -	if (f->rpos < f->rend) f->seek(f, f->rpos-f->rend, SEEK_CUR);
> +	/* fflush_unlocked(NULL) is supported by glibc, mimic that */
>  
> -	/* Clear read and write modes */
> -	f->wpos = f->wbase = f->wend = 0;
> -	f->rpos = f->rend = 0;
> +	r = __stdout_used ? __fflush_unlocked(__stdout_used) : 0;
>  
> -	return 0;
> +	for (f=*__ofl_lock(); f; f=f->next) {
> +		if (f->wpos > f->wbase) r |= __fflush_unlocked(f);
> +	}
> +	__ofl_unlock();
> +
> +	return r;
>  }
>  
>  /* stdout.c will override this if linked */

This patch introduces significant code duplication and complexity for
the sake of saving something like 10 cycles in an operation that makes
syscalls (i.e. takes thousands if not tens of thousands of cycles). As
mentioned on the bb list, the right fix is just making fflush_unlocked
an alias for fflush. Then __fflush_unlocked can be eliminated
completely and the file simplified rather than increased in
complexity. See the attached patch which I'll apply if there are no
obvious mistakes or objections.

Rich

[-- Attachment #2: fflush_unlocked.diff --]
[-- Type: text/plain, Size: 1472 bytes --]

diff --git a/src/stdio/fflush.c b/src/stdio/fflush.c
index 3f462c8..c288106 100644
--- a/src/stdio/fflush.c
+++ b/src/stdio/fflush.c
@@ -1,11 +1,30 @@
 #include "stdio_impl.h"
 
-static int __fflush_unlocked(FILE *f)
+/* stdout.c will override this if linked */
+static FILE *volatile dummy = 0;
+weak_alias(dummy, __stdout_used);
+
+int fflush(FILE *f)
 {
+	if (!f) {
+		int r = __stdout_used ? fflush(__stdout_used) : 0;
+
+		for (f=*__ofl_lock(); f; f=f->next)
+			if (f->wpos > f->wbase) r |= fflush(f);
+		__ofl_unlock();
+
+		return r;
+	}
+
+	FLOCK(f);
+
 	/* If writing, flush output */
 	if (f->wpos > f->wbase) {
 		f->write(f, 0, 0);
-		if (!f->wpos) return EOF;
+		if (!f->wpos) {
+			FUNLOCK(f);
+			return EOF;
+		}
 	}
 
 	/* If reading, sync position, per POSIX */
@@ -15,34 +34,8 @@ static int __fflush_unlocked(FILE *f)
 	f->wpos = f->wbase = f->wend = 0;
 	f->rpos = f->rend = 0;
 
+	FUNLOCK(f);
 	return 0;
 }
 
-/* stdout.c will override this if linked */
-static FILE *volatile dummy = 0;
-weak_alias(dummy, __stdout_used);
-
-int fflush(FILE *f)
-{
-	int r;
-
-	if (f) {
-		FLOCK(f);
-		r = __fflush_unlocked(f);
-		FUNLOCK(f);
-		return r;
-	}
-
-	r = __stdout_used ? fflush(__stdout_used) : 0;
-
-	for (f=*__ofl_lock(); f; f=f->next) {
-		FLOCK(f);
-		if (f->wpos > f->wbase) r |= __fflush_unlocked(f);
-		FUNLOCK(f);
-	}
-	__ofl_unlock();
-	
-	return r;
-}
-
-weak_alias(__fflush_unlocked, fflush_unlocked);
+weak_alias(fflush, fflush_unlocked);