From owner-svn-src-head@FreeBSD.ORG Sat Dec 5 18:02:49 2009 Return-Path: Delivered-To: svn-src-head@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 0718E106566B; Sat, 5 Dec 2009 18:02:49 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail06.syd.optusnet.com.au (mail06.syd.optusnet.com.au [211.29.132.187]) by mx1.freebsd.org (Postfix) with ESMTP id 8D5818FC15; Sat, 5 Dec 2009 18:02:48 +0000 (UTC) Received: from c220-239-235-116.carlnfd3.nsw.optusnet.com.au (c220-239-235-116.carlnfd3.nsw.optusnet.com.au [220.239.235.116]) by mail06.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id nB5I2UrZ024261 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Sun, 6 Dec 2009 05:02:45 +1100 Date: Sun, 6 Dec 2009 05:02:30 +1100 (EST) From: Bruce Evans X-X-Sender: bde@delplex.bde.org To: Stefan Farfeleder In-Reply-To: <200912051312.nB5DC4kQ088533@svn.freebsd.org> Message-ID: <20091206034620.C26414@delplex.bde.org> References: <200912051312.nB5DC4kQ088533@svn.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org Subject: Re: svn commit: r200120 - head/usr.bin/make X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 05 Dec 2009 18:02:49 -0000 On Sat, 5 Dec 2009, Stefan Farfeleder wrote: > Log: > Add a missing space to the error message when execvp() failed. > > Modified: > head/usr.bin/make/proc.c > > Modified: head/usr.bin/make/proc.c > ============================================================================== > --- head/usr.bin/make/proc.c Sat Dec 5 12:51:51 2009 (r200119) > +++ head/usr.bin/make/proc.c Sat Dec 5 13:12:04 2009 (r200120) > @@ -116,7 +116,7 @@ Proc_Exec(const ProcStuff *ps) > execvp(ps->argv[0], ps->argv); > Someone broke the formatting by adding this extra blank line. > write(STDERR_FILENO, ps->argv[0], strlen(ps->argv[0])); > - write(STDERR_FILENO, ":", 1); > + write(STDERR_FILENO, ": ", 2); > write(STDERR_FILENO, strerror(errno), strlen(strerror(errno))); > write(STDERR_FILENO, "\n", 1); > } else { > It would be nice if the err() family were not so broken in design and implementation so as to be unusable in signal handlers and other delicate contexts. Then it could just be used here (here we are in vfork context after a failed exec). The err() family uses stdio, so it might flush output stream(s), which would be bad as documented in vfork(2), and probably other things that might be done by stdio would be even worse in vfork() context. (The restructuring that added the above style bug also moved the execvp() far away from the vfork(), so it is unclear even that most things cannot be done in code near here.) Brokenness in the err() family includes not really documenting where the output goes. stdio is not mentioned. [stdio] streams are mentioned, but only in connection with the FreeBSD extension of err_set_file() (without this extension, the err() family could and should always write to the STDERR_FILENO non-stream using code like the above). Output is documented to go "on the standard error output", which should mean to STDERR_FILENO, but output actually goes to err_file (default stderr) using fprintf(). Corresponding brokenness in perror() is to be expected, since perror() is declared in , but perror() is documented to only write to "the standard output file descriptor", and it used to do this very carefully (using local variables), so it was usuable in signal handlers and other delicate contexts including vfork(). But it never documented this safeness, and FreeBSD turned its careful code into nonsense: % RCS file: /home/ncvs/src/lib/libc/stdio/perror.c,v % Working file: perror.c % head: 1.8 % ... % ---------------------------- % revision 1.8 % date: 2002/12/19 09:53:26; author: tjr; state: Exp; lines: +7 -1 % Write the message to stderr, not file descriptor 2, so that perror() % writes to the correct stream if stderr has been redirected with freopen(). The man page still documents writing to fd 2, and writing to stderr defeats not using stdio. The log message and man page are missing mention of the new flushing feature. % ============================================================================= % Index: perror.c % =================================================================== % RCS file: /home/ncvs/src/lib/libc/stdio/perror.c,v % retrieving revision 1.7 % retrieving revision 1.8 % diff -u -2 -r1.7 -r1.8 % --- perror.c 19 Dec 2002 09:50:10 -0000 1.7 % +++ perror.c 19 Dec 2002 09:53:26 -0000 1.8 % @@ -36,5 +36,5 @@ % #endif /* LIBC_SCCS and not lint */ % #include % -__FBSDID("$FreeBSD: src/lib/libc/stdio/perror.c,v 1.7 2002/12/19 09:50:10 tjr Exp $"); % +__FBSDID("$FreeBSD: src/lib/libc/stdio/perror.c,v 1.8 2002/12/19 09:53:26 tjr Exp $"); % % #include "namespace.h" % @@ -47,4 +47,6 @@ % #include % #include "un-namespace.h" % +#include "libc_private.h" % +#include "local.h" % % void % @@ -71,4 +73,8 @@ % v->iov_base = "\n"; % v->iov_len = 1; % - (void)_writev(STDERR_FILENO, iov, (v - iov) + 1); % + FLOCKFILE(stderr); Just locking stderr breaks signal-safeness. % + __sflush(stderr); Flushing also breaks use in [v]fork() after failed exec. Pre-flushing like this might be useful, but the err() family doesn't do any. Here the pre-flush is needed to synchronize with stderr, since the output here (now pointlessly) uses a direct writev. The err() family uses stderr, so it doesn't need to do anything to synchronize with stderr, but it could do more to sync with other streams and it should do more to ensure that its output actually goes out in case stderr is unbuffered (especially since its use of stdio is undocumented, its callers cannot know what synchronization is necessary or possible). % + (void)_writev(stderr->_file, iov, (v - iov) + 1); Maybe stderr->_file can be type-stable or something, so that it can be accessed not too unsafely without locking in signal handlers. The change should have been limited to something like that. % + stderr->_flags &= ~__SOFF; Another state change which must be avoided in delicate contexts. % + FUNLOCKFILE(stderr); The locking is hopefully idempotent, so it is safe in vfork context. % } Both Standard C and POSIX specify perror() to be perfectly broken. They specify it to write to the "standard error stream" (why not just stderr?). Standard C doesn't have STDERR_FILENO, so it cannot do the right thing here except by not really specifying where the output goes. Neither specifies any special synchronization. POSIX (and newer C99?) also specifies that perror() shall not change the orientation of the standard error stream. A non-broken err() would look like the non-broken perror(), except it would have to snprintf() to a local buffer before writev() and thus it could no longer support very long messages. Bruce