From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/14522 Path: news.gmane.org!.POSTED.blaine.gmane.org!not-for-mail From: James Y Knight Newsgroups: gmane.linux.lib.musl.general Subject: Re: error.h implementation Date: Tue, 6 Aug 2019 18:06:12 -0400 Message-ID: References: <20190723041615.GB15665@brightrain.aerifal.cx> <20190806161331.GX9017@brightrain.aerifal.cx> Reply-To: musl@lists.openwall.com Mime-Version: 1.0 Content-Type: multipart/alternative; boundary="0000000000004b492e058f7a0a58" Injection-Info: blaine.gmane.org; posting-host="blaine.gmane.org:195.159.176.226"; logging-data="191000"; mail-complaints-to="usenet@blaine.gmane.org" To: musl@lists.openwall.com Original-X-From: musl-return-14538-gllmg-musl=m.gmane.org@lists.openwall.com Wed Aug 07 00:07:01 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 1hv7bg-000nZa-NG for gllmg-musl@m.gmane.org; Wed, 07 Aug 2019 00:07:00 +0200 Original-Received: (qmail 13801 invoked by uid 550); 6 Aug 2019 22:06:58 -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 13751 invoked from network); 6 Aug 2019 22:06:57 -0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to; bh=9z2jjj4GT/LUoyexyvWcCrTm94GPmbttj/BVHh1YNS4=; b=h2P91/8Uawu/AkKKxVO3SIVQR2FtN9PubIuM23MDEUKr7R30AVtnmC3bWuRzKNDXZA x9P0igiV+CGXg7nNxIRF1ecOO+P4QNXh3zyOXcGvwQy+eCnlTUc9lAb7Wfje7dwHtbC6 eiYUSXmqB6E3pXm+Yg6+esYLKxzr69ViTH/uULgry58B47NyP94/medThXnP6iSASddK RIlqgZiGn6fMfdwFWSBysaUHaJBQ0jF17R4rDatxS14E7WAqt7PUc+q6hQxURQxjAO7N BgGXrAvhfXCqly0CFyQYakDWSOekmzm6AwZV/jK+jbUYyhZd6O2dwA2AvBiJu6ad3yL1 AByw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to; bh=9z2jjj4GT/LUoyexyvWcCrTm94GPmbttj/BVHh1YNS4=; b=pba20ts8IAxrbc29w2v8KohzpXq5KaKO34PptcYnK1rSWT5/IqF6cjNRhlG8ClQMzz JomdFTB8CEh1omxHrXvZPiPcXG9KHe7HIraZzsoXFGIb6yF3F283ghhl9cJQstM8Oq7g PLUBi/7jURwWP5woIWMGjk5g+GpI1fq2Dnj+4jmCvZfd5J8l1tOquH/nvaJyPN7qpXxN jdeHj5wk5JlogZZ1GHMua6Q7NQQh2c15Y6lkFaBMEJKkAVHDslXSMYOV675ldvYk0Rxv EQCzMLjNjZU859PdrwT4mpo3s6NW/ggbXTlxaJmyX9CcVtJYZvlsOIjEOYIKwo5RqlKB mK8Q== X-Gm-Message-State: APjAAAVdHIHLbEsD6iFQM3+nrZZHaRcbxkRrUkgchpk2Sp5+2AiJLy4E 1zjsbjGUpBh3Q+8sS94Qtakhj+5TYW5yLpaOdy5gISG7 X-Google-Smtp-Source: APXvYqy+zFO3C0N8RVHy77sAptKjEXwoni/tb7CezVW8fk72ryqEcWnFDoHi63d7x+nMqrbdLIZJAZC+wGh/fFEFGKs= X-Received: by 2002:a67:6282:: with SMTP id w124mr3930189vsb.4.1565129205004; Tue, 06 Aug 2019 15:06:45 -0700 (PDT) In-Reply-To: <20190806161331.GX9017@brightrain.aerifal.cx> Xref: news.gmane.org gmane.linux.lib.musl.general:14522 Archived-At: --0000000000004b492e058f7a0a58 Content-Type: text/plain; charset="UTF-8" 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 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!) > --0000000000004b492e058f7a0a58 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
On Tue, Aug 6, 2019, 12:13 PM Rich Felker <dalias@libc.org> 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 <error.h>
> +#include <stdarg.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include "libc.h"
> +
> +void (*error_print_progname) (void) =3D 0;
> +unsigned int error_message_count =3D 0;
> +int error_one_per_line =3D 0;
> +
> +static unsigned int saved_linenum =3D 0;
> +static const char *saved_file =3D 0;
> +
> +static void errorv(int status, int errnum, const char *file, unsigned= int linenum, const char *fmt, va_list ap)
> +{
> +=C2=A0 =C2=A0 =C2=A0++error_message_count;
> +
> +=C2=A0 =C2=A0 =C2=A0fflush(stdout);
> +=C2=A0 =C2=A0 =C2=A0flockfile(stderr);
> +
> +=C2=A0 =C2=A0 =C2=A0if (error_print_progname)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0error_print_progname(= );
> +=C2=A0 =C2=A0 =C2=A0else {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0fprintf(stderr, "= ;%s:", __progname_full);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (!file)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0fputc(' ', stderr);
> +=C2=A0 =C2=A0 =C2=A0}
> +
> +=C2=A0 =C2=A0 =C2=A0if (file)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0fprintf(stderr, "= ;%s:%u: ", file, linenum);
> +
> +=C2=A0 =C2=A0 =C2=A0vfprintf(stderr, fmt, ap);
> +=C2=A0 =C2=A0 =C2=A0if (errnum)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0fprintf(stderr, "= ;: %s", strerror(errnum));
> +=C2=A0 =C2=A0 =C2=A0fputc('\n', stderr);
> +
> +=C2=A0 =C2=A0 =C2=A0funlockfile(stderr);
> +
> +=C2=A0 =C2=A0 =C2=A0if (status)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0exit(status);
> +}
> +
> +void error(int status, int errnum, const char *fmt, ...)
> +{
> +=C2=A0 =C2=A0 =C2=A0va_list ap;
> +=C2=A0 =C2=A0 =C2=A0va_start(ap, fmt);
> +=C2=A0 =C2=A0 =C2=A0errorv(status, errnum, NULL, 0, fmt, ap);
> +=C2=A0 =C2=A0 =C2=A0va_end(ap);
> +}
> +
> +void error_at_line(int status, int errnum, const char *file, unsigned= int linenum, const char *fmt, ...)
> +{
> +=C2=A0 =C2=A0 =C2=A0if (error_one_per_line) {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if(saved_linenum =3D= =3D linenum && file !=3D NULL &&
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 saved_file != =3D NULL && !strcmp(file, saved_file))
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0return;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0saved_linenum =3D lin= enum;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0// Assuming that the = lifetime of the passed in file name extends
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0// until the next cal= l is rather questionable, but appears to be
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0// the expected seman= tics.
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0saved_file =3D file;<= br> > +=C2=A0 =C2=A0 =C2=A0}
> +
> +=C2=A0 =C2=A0 =C2=A0va_list ap;
> +=C2=A0 =C2=A0 =C2=A0va_start(ap, fmt);
> +=C2=A0 =C2=A0 =C2=A0errorv(status, errnum, file, linenum, fmt, ap); > +=C2=A0 =C2=A0 =C2=A0va_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 expe= ct most uses are single threaded but it's better not to be thread-unsaf= e.

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 wil= l still have to take care, which is unavoidable and fine.

I just noticed this is still using '<= span style=3D"font-family:sans-serif">__progname_full' instead of the p= ublic 'program_invocation_name', which I should also fix.

(But I'm out and won&#= 39;t be able to update anything for a couple weeks, sorry!)
--0000000000004b492e058f7a0a58--