Re: error.h implementation

[James Y Knight <jyknight@google.com>]

[-- Attachment #1: Type: text/plain, Size: 3778 bytes --]

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 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!)

>

[-- Attachment #2: Type: text/html, Size: 5267 bytes --]