Re: error.h implementation

[Rich Felker <dalias@libc.org>]

On Tue, Aug 06, 2019 at 06:06:12PM -0400, James Y Knight wrote:
> 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) = 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