[PATCH] make fflush_unlocked(NULL) work

[PATCH] make fflush_unlocked(NULL) work

[Denys Vlasenko]

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 */
-- 
2.9.2

Re: [PATCH] make fflush_unlocked(NULL) work

[Rich Felker]

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

Re: Re: [PATCH] make fflush_unlocked(NULL) work

[Denys Vlasenko]

On Sun, Sep 18, 2016 at 10:36 PM, Rich Felker <dalias@libc.org> wrote:
> 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).

fflush(NULL) does not _always_ perform syscalls.
It may well find that none of the streams need any work.

In fact, in some applications it may end it not doing any writes
most of the times it is called: for example, I call it before (v)fork()
(unless I know that I for sure do not have any buffered writes).

Sometimes I call it before vfork out of sheer paranoia,
even if it "looks like it's not needed":
having stdio buffering in vfork-shared mm is huge PITA
and a source of nasty bugs. Been there.

> 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.

Your call.