[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAA2zVHrTwFc26Zvgq5gk0=h39hUTYfVUk_JE4FB9FxJ76GjKJQ@mail.gmail.com>
Date: Tue, 30 Jul 2019 12:03:56 -0400
From: James Y Knight <jyknight@...gle.com>
To: musl@...ts.openwall.com
Subject: Re: error.h implementation
On Mon, Jul 22, 2019 at 9:16 PM Rich Felker <dalias@...c.org> wrote:
>
> > A question: is the weak_alias stuff required in this case? I'm not
> > 100% clear as to when that's required, and when it's not, but I think
> > perhaps it's not actually needed here, because nothing else in the
> > libc refers to these nonstandard symbols, and the file defines no
> > standard symbols?
>
> Not needed. It would only be needed if they were being used to
> implement something else in one of the standards-governed namespaces.
Thanks for the confirmation, fixed.
> > diff --git a/dynamic.list b/dynamic.list
> > index ee0d363b..d8f57b85 100644
> > --- a/dynamic.list
> > +++ b/dynamic.list
> > @@ -42,4 +42,11 @@ __progname;
> > __progname_full;
> >
> > __stack_chk_guard;
> > +
> > +error_print_progname;
> > +error_message_count;
> > +error_one_per_line;
> > +__error_print_progname;
> > +__error_message_count;
> > +__error_one_per_line;
> > };
>
> This is a rather bleh aspect of adding them. At least three of those
> would be gone with the aliases removed.
Yeah. It's really rather unfortunate that copy relocations even exist
and this list is even needed at all. Sigh.
> > +void error(int status, int errnum, const char *fmt, ...);
> > +void error_at_line(int status, int errnum,
> > + const char *file, unsigned int linenum,
> > + const char *fmt, ...);
>
> We don't use argument names in header files. Also the convention of
> aligning continuations of function arguments like this is not
> generally used (and not often needed without names taking up lots of
> column space), but not objectionable as long as it's aligned right.
Fixed.
> > +static void errorv(int status, int errnum,
> > + const char *file, unsigned int linenum,
> > + const char *fmt, va_list ap) {
>
> But this one isn't done right -- it's mixing tabs and spaces. Tabs
> should only be used for indention levels, spaces for alignment. So if
> you were aligning a continued line in an already-indented context, the
> continuation would start with the same number of tabs as the line
> being continued, followed by spaces to align. This is necessary to be
> agnostic to the width of tabs, which is an accessibility issue.
indeed, sorry about that. In this case, it looks like usually the
function signature is generally not wrapped in musl, even when it's
rather long (in this case 111 columns)? Assuming that's the desired
style, since it's commonly used elsewhere, I've simply unwrapped this
line (and the same for the other functions).
Digression -- while I agree in the abstract that the
indent-vs-alignment distinction is a great idea, it's uncommonly
supported by editors, and is invisible when you get it wrong. This
means it requires a good deal of human attention. I think it might be
worth considering switching to a spaces-only indentation style.
> Also, while { on same line is used for ifs/loops, it's not used for
>
> start of functions; { always goes on its own line here.
Fixed.
>
> > + ++__error_message_count;
> > +
> > + fflush(stdout);
> > + FLOCK(stderr);
>
> Please avoid using anything from stdio_impl.h or other implementation
> internals when implementing nonstandard functionality that can be done
> with 100% portable code in terms of the standard functions (e.g.
> flockfile). This principle avoids having "junk" interfaces like the
> error.h stuff become maintenance burdens by requiring someone to
> understand and change them if the internals change.
That's a great principle! Fixed. I wonder if it'd be a good idea to
shift around the source code layout for a more obvious layering --
putting the portable library code into one subdir and the OS-specific
bits into another.
> > + if (__error_print_progname)
> > + __error_print_progname();
> And since you're calling an application-provided callback, it's
> probably even necessary to use flockfile. I'm not sure it's safe to
> use the FLOCK() macro when application code may run in the context
> holding the lock, and there's not an intent that it be safe.
Indeed.
> > + fflush(stderr);
> > + FUNLOCK(stderr);
>
> Is there a reason for the fflush? stderr is unbuffered by default, and
> presumably if someone makes it buffered, they want it that way for a
> reason..?
Nope, I agree, this seems superfluous. Removed. :)
>
> Also, the first fflush, if it's to be done, should be done after
> locking, not before. Otherwise you just take and release the lock
> twice for no reason.
The previous fflush is flushing stdout, before writing to stderr, so I
think it doesn't belong within the locked region. The stdout flushing
is part of the documented behavior of this function.
> > +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;
> > + }
>
> I'm not sure if this is okay or not. It probably should have been
> designed as a macro that passes __FILE__ and __LINE__ opaquely to the
> application, so that the callee can assume file points to a literal
> with static storage, but of course everything about these functions is
> awful and maybe the implied contract is that you're supposed to use it
> that way.
Yes, this design is awful, and this undocumented requirement is
unfortunate. Yet, indeed, this is the implied contract. :(
Fortunately, the use of the 'error_one_per_line = 1' feature is rather
rare, e.g. see <https://codesearch.debian.net/search?q=error_one_per_line%5Cs*%3D>.
Attached updated patch.
View attachment "0001-Add-support-for-the-glibc-specific-error.h-header.patch" of type "text/x-patch" (3170 bytes)
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
