From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/14523 Path: news.gmane.org!.POSTED.blaine.gmane.org!not-for-mail From: Rich Felker Newsgroups: gmane.linux.lib.musl.general Subject: Re: error.h implementation Date: Tue, 6 Aug 2019 18:50:54 -0400 Message-ID: <20190806225054.GA9017@brightrain.aerifal.cx> References: <20190723041615.GB15665@brightrain.aerifal.cx> <20190806161331.GX9017@brightrain.aerifal.cx> Reply-To: musl@lists.openwall.com Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Injection-Info: blaine.gmane.org; posting-host="blaine.gmane.org:195.159.176.226"; logging-data="109523"; mail-complaints-to="usenet@blaine.gmane.org" User-Agent: Mutt/1.5.21 (2010-09-15) To: musl@lists.openwall.com Original-X-From: musl-return-14539-gllmg-musl=m.gmane.org@lists.openwall.com Wed Aug 07 00:51:10 2019 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.89) (envelope-from ) id 1hv8IQ-000SPB-Jp for gllmg-musl@m.gmane.org; Wed, 07 Aug 2019 00:51:10 +0200 Original-Received: (qmail 7360 invoked by uid 550); 6 Aug 2019 22:51:06 -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 7341 invoked from network); 6 Aug 2019 22:51:06 -0000 Content-Disposition: inline In-Reply-To: Original-Sender: Rich Felker Xref: news.gmane.org gmane.linux.lib.musl.general:14523 Archived-At: On Tue, Aug 06, 2019 at 06:06:12PM -0400, James Y Knight wrote: > On Tue, Aug 6, 2019, 12:13 PM Rich Felker wrote: > > > > diff --git a/src/legacy/error.c b/src/legacy/error.c > > > new file mode 100644 > > > index 00000000..c5000fa4 > > > --- /dev/null > > > +++ b/src/legacy/error.c > > > @@ -0,0 +1,69 @@ > > > +#include > > > +#include > > > +#include > > > +#include > > > +#include > > > +#include "libc.h" > > > + > > > +void (*error_print_progname) (void) = 0; > > > +unsigned int error_message_count = 0; > > > +int error_one_per_line = 0; > > > + > > > +static unsigned int saved_linenum = 0; > > > +static const char *saved_file = 0; > > > + > > > +static void errorv(int status, int errnum, const char *file, unsigned > > int linenum, const char *fmt, va_list ap) > > > +{ > > > + ++error_message_count; > > > + > > > + fflush(stdout); > > > + flockfile(stderr); > > > + > > > + if (error_print_progname) > > > + error_print_progname(); > > > + else { > > > + fprintf(stderr, "%s:", __progname_full); > > > + if (!file) > > > + fputc(' ', stderr); > > > + } > > > + > > > + if (file) > > > + fprintf(stderr, "%s:%u: ", file, linenum); > > > + > > > + vfprintf(stderr, fmt, ap); > > > + if (errnum) > > > + fprintf(stderr, ": %s", strerror(errnum)); > > > + fputc('\n', stderr); > > > + > > > + funlockfile(stderr); > > > + > > > + if (status) > > > + exit(status); > > > +} > > > + > > > +void error(int status, int errnum, const char *fmt, ...) > > > +{ > > > + va_list ap; > > > + va_start(ap, fmt); > > > + errorv(status, errnum, NULL, 0, fmt, ap); > > > + va_end(ap); > > > +} > > > + > > > +void error_at_line(int status, int errnum, const char *file, unsigned > > int linenum, const char *fmt, ...) > > > +{ > > > + if (error_one_per_line) { > > > + if(saved_linenum == linenum && file != NULL && > > > + saved_file != NULL && !strcmp(file, saved_file)) > > > + return; > > > + saved_linenum = linenum; > > > + // Assuming that the lifetime of the passed in file name > > extends > > > + // until the next call is rather questionable, but appears > > to be > > > + // the expected semantics. > > > + saved_file = file; > > > + } > > > + > > > + va_list ap; > > > + va_start(ap, fmt); > > > + errorv(status, errnum, file, linenum, fmt, ap); > > > + va_end(ap); > > > +} > > > -- > > > 2.22.0.709.g102302147b-goog > > > > One other question: what is the thread-safety of these interfaces > > supposed to be? The globals are accessed without any synchronization, > > but flockfile is used to synchronize with concurrent access to stderr. > > This is plausibly correct (if the error.h functions themselves can't > > be used from multiple threads, but can be used in a multithreaded > > application), but at least questionable. > > > > The error_at_line stuff is probably not possible to make thread-safe > > without thread-local state for saved_file/linenum, which would be a > > disproportionate cost for these interfaces. > > > > I'm not objecting to anything with these comments, just wanting to > > confirm that you think it's reasonable as-is. > > > > Hmm, good question! I expect most uses are single threaded but it's better > not to be thread-unsafe. > > So, I think it's probably best to move everything within the stderr lock > (which means moving the flockfile out to the error and error_at_line > functions). That at least makes these safe w.r.t. themselves. Users > accessing the public globals will still have to take care, which is > unavoidable and fine. I don't think the error_one_per_line stuff can be made safe at all, without TLS, due to the issue about the lifetime of the string passed to it. So it's probably ok declaring this one as hard thread-unsafe (and so broken it shouldn't be used). The rest of the cases then should be safe if you just move the increment inside the lock, I think. > I just noticed this is still using '__progname_full' instead of the > public 'program_invocation_name', > which I should also fix. > > (But I'm out and won't be able to update anything for a couple weeks, > sorry!) Rather than leave things in limbo, I'm happy to polish up and commit patches you have pending. I'll probably be making another release in roughly this timeframe and would like to get these things (especially the glob fix) in. Rich