From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/10497 Path: news.gmane.org!.POSTED!not-for-mail From: Rich Felker Newsgroups: gmane.linux.lib.musl.general Subject: Re: [PATCH] make fflush_unlocked(NULL) work Date: Sun, 18 Sep 2016 16:36:08 -0400 Message-ID: <20160918203608.GB15995@brightrain.aerifal.cx> References: <20160918194455.21981-1-vda.linux@googlemail.com> Reply-To: musl@lists.openwall.com NNTP-Posting-Host: blaine.gmane.org Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="8jNwmpfkpox/fiJK" X-Trace: blaine.gmane.org 1474231008 21685 195.159.176.226 (18 Sep 2016 20:36:48 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Sun, 18 Sep 2016 20:36:48 +0000 (UTC) User-Agent: Mutt/1.5.21 (2010-09-15) To: musl Original-X-From: musl-return-10510-gllmg-musl=m.gmane.org@lists.openwall.com Sun Sep 18 22:36:43 2016 Return-path: Envelope-to: gllmg-musl@m.gmane.org Original-Received: from mother.openwall.net ([195.42.179.200]) by blaine.gmane.org with smtp (Exim 4.84_2) (envelope-from ) id 1blip7-00036i-0q for gllmg-musl@m.gmane.org; Sun, 18 Sep 2016 22:36:25 +0200 Original-Received: (qmail 18129 invoked by uid 550); 18 Sep 2016 20:36:24 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Original-Received: (qmail 18084 invoked from network); 18 Sep 2016 20:36:22 -0000 Content-Disposition: inline In-Reply-To: <20160918194455.21981-1-vda.linux@googlemail.com> Original-Sender: Rich Felker Xref: news.gmane.org gmane.linux.lib.musl.general:10497 Archived-At: --8jNwmpfkpox/fiJK Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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 > CC: musl > --- > 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 --8jNwmpfkpox/fiJK Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="fflush_unlocked.diff" 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); --8jNwmpfkpox/fiJK--