You are viewing a plain text version of this content. The canonical link for it is here.
Posted to cvs@httpd.apache.org by co...@apache.org on 2018/06/19 12:07:19 UTC
svn commit: r1833827 - /httpd/httpd/trunk/server/log.c
Author: covener
Date: Tue Jun 19 12:07:19 2018
New Revision: 1833827
URL: http://svn.apache.org/viewvc?rev=1833827&view=rev
Log:
add server_rec to log.c fatal startup errors
Not strictly necessary for trunk, but in 2.4.x if the main ErrorLog is
using syslog, these messages are lost. In trunk, the low-level logging
routines reach up and find the syslog provider when no server_rec is provided
but that backport is stalled.
Modified:
httpd/httpd/trunk/server/log.c
Modified: httpd/httpd/trunk/server/log.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/server/log.c?rev=1833827&r1=1833826&r2=1833827&view=diff
==============================================================================
--- httpd/httpd/trunk/server/log.c (original)
+++ httpd/httpd/trunk/server/log.c Tue Jun 19 12:07:19 2018
@@ -185,14 +185,14 @@ AP_DECLARE(apr_status_t) ap_replace_stde
char *filename = ap_server_root_relative(p, fname);
if (!filename) {
ap_log_error(APLOG_MARK, APLOG_STARTUP|APLOG_CRIT,
- APR_EBADPATH, NULL, APLOGNO(00085) "Invalid -E error log file %s",
+ APR_EBADPATH, ap_server_conf, APLOGNO(00085) "Invalid -E error log file %s",
fname);
return APR_EBADPATH;
}
if ((rc = apr_file_open(&stderr_file, filename,
APR_APPEND | APR_WRITE | APR_CREATE | APR_LARGEFILE,
APR_OS_DEFAULT, p)) != APR_SUCCESS) {
- ap_log_error(APLOG_MARK, APLOG_STARTUP, rc, NULL, APLOGNO(00086)
+ ap_log_error(APLOG_MARK, APLOG_STARTUP, rc, ap_server_conf, APLOGNO(00086)
"%s: could not open error log file %s.",
ap_server_argv0, fname);
return rc;
@@ -324,7 +324,7 @@ static int open_error_log(server_rec *s,
* child inherits the parents stderr. */
rc = log_child(p, fname, &dummy, cmdtype, is_main);
if (rc != APR_SUCCESS) {
- ap_log_error(APLOG_MARK, APLOG_STARTUP, rc, NULL, APLOGNO(00089)
+ ap_log_error(APLOG_MARK, APLOG_STARTUP, rc, ap_server_conf, APLOGNO(00089)
"Couldn't start ErrorLog process '%s'.",
s->error_fname + 1);
return DONE;
@@ -343,7 +343,7 @@ static int open_error_log(server_rec *s,
else {
fname = ap_server_root_relative(p, s->error_fname);
if (!fname) {
- ap_log_error(APLOG_MARK, APLOG_STARTUP, APR_EBADPATH, NULL, APLOGNO(00090)
+ ap_log_error(APLOG_MARK, APLOG_STARTUP, APR_EBADPATH, ap_server_conf, APLOGNO(00090)
"%s: Invalid error log path %s.",
ap_server_argv0, s->error_fname);
return DONE;
@@ -351,7 +351,7 @@ static int open_error_log(server_rec *s,
if ((rc = apr_file_open(&s->error_log, fname,
APR_APPEND | APR_WRITE | APR_CREATE | APR_LARGEFILE,
APR_OS_DEFAULT, p)) != APR_SUCCESS) {
- ap_log_error(APLOG_MARK, APLOG_STARTUP, rc, NULL, APLOGNO(00091)
+ ap_log_error(APLOG_MARK, APLOG_STARTUP, rc, ap_server_conf, APLOGNO(00091)
"%s: could not open error log file %s.",
ap_server_argv0, fname);
return DONE;
@@ -1532,7 +1532,7 @@ AP_DECLARE(void) ap_log_pid(apr_pool_t *
fname = ap_runtime_dir_relative(p, filename);
if (!fname) {
ap_log_error(APLOG_MARK, APLOG_STARTUP|APLOG_CRIT, APR_EBADPATH,
- NULL, APLOGNO(00097) "Invalid PID file path %s, ignoring.", filename);
+ ap_server_conf, APLOGNO(00097) "Invalid PID file path %s, ignoring.", filename);
return;
}
@@ -1585,7 +1585,7 @@ AP_DECLARE(apr_status_t) ap_read_pid(apr
fname = ap_runtime_dir_relative(p, filename);
if (!fname) {
ap_log_error(APLOG_MARK, APLOG_STARTUP|APLOG_CRIT, APR_EBADPATH,
- NULL, APLOGNO(00101) "Invalid PID file path %s, ignoring.", filename);
+ ap_server_conf, APLOGNO(00101) "Invalid PID file path %s, ignoring.", filename);
return APR_EGENERAL;
}
@@ -1657,7 +1657,7 @@ static apr_status_t piped_log_spawn(pipe
!= APR_SUCCESS) ||
((status = apr_procattr_error_check_set(procattr, 1)) != APR_SUCCESS)) {
/* Something bad happened, give up and go away. */
- ap_log_error(APLOG_MARK, APLOG_STARTUP, status, NULL, APLOGNO(00103)
+ ap_log_error(APLOG_MARK, APLOG_STARTUP, status, ap_server_conf, APLOGNO(00103)
"piped_log_spawn: unable to setup child process '%s'",
pl->program);
}
@@ -1682,7 +1682,7 @@ static apr_status_t piped_log_spawn(pipe
}
else {
/* Something bad happened, give up and go away. */
- ap_log_error(APLOG_MARK, APLOG_STARTUP, status, NULL, APLOGNO(00104)
+ ap_log_error(APLOG_MARK, APLOG_STARTUP, status, ap_server_conf, APLOGNO(00104)
"unable to start piped log program '%s'",
pl->program);
}
@@ -1812,7 +1812,7 @@ AP_DECLARE(piped_log *) ap_open_piped_lo
rc = log_child(p, program, &dummy, cmdtype, 0);
if (rc != APR_SUCCESS) {
- ap_log_error(APLOG_MARK, APLOG_STARTUP, rc, NULL, APLOGNO(00108)
+ ap_log_error(APLOG_MARK, APLOG_STARTUP, rc, ap_server_conf, APLOGNO(00108)
"Couldn't start piped log process '%s'.",
(program == NULL) ? "NULL" : program);
return NULL;
Re: svn commit: r1833827 - /httpd/httpd/trunk/server/log.c
Posted by Eric Covener <co...@gmail.com>.
> I looked at that a bit and the loop seemed pretty tight. At some
> point I was looking to explicit reset ap_server_conf but then thought
> it was not necessary -- maybe no hooks called between cleanups and
> re-assignment?
Thanks for the original followup on this one Bill -- It is actually a
big window due to EXEC_ON_READ http//svn.apache.org/r1835094
Re: svn commit: r1833827 - /httpd/httpd/trunk/server/log.c
Posted by Eric Covener <co...@gmail.com>.
On Wed, Jun 20, 2018 at 10:16 AM William A Rowe Jr <wr...@rowe-clan.net> wrote:
>
> On Wed, Jun 20, 2018 at 9:08 AM, Eric Covener <co...@gmail.com> wrote:
>>
>> On Wed, Jun 20, 2018 at 10:04 AM William A Rowe Jr <wr...@rowe-clan.net> wrote:
>> >
>> > Question, what is a server_conf, before httpd's argv[] array
>> > has even been processed?
>>
>> It's NULL, but these (log.c only) uses should be well after.
>
>
> In fact, some of these are argv[] handlers, but as long as we have
> server_conf of NULL, then your patch should be a no-op in such cases.
> I'd expect any APLOG_STARTUP phase messages to predate the
> successful creation of server_conf.
A good # of these are related to e.g. opening logs which comes much
later since it is based on the config --
I think APLOG_STARTUP is just misused/misunderstood (the issue with
syslog is actually independent of APLOG_STARTUP)
>
> The exception would be if some of this processing happens after
> pconf is torn down during a graceful restart, without resetting
> server_conf to NULL, first. That might show up as graceful restart
> segv's.
I looked at that a bit and the loop seemed pretty tight. At some
point I was looking to explicit reset ap_server_conf but then thought
it was not necessary -- maybe no hooks called between cleanups and
re-assignment?
--
Eric Covener
covener@gmail.com
Re: svn commit: r1833827 - /httpd/httpd/trunk/server/log.c
Posted by William A Rowe Jr <wr...@rowe-clan.net>.
On Wed, Jun 20, 2018 at 9:08 AM, Eric Covener <co...@gmail.com> wrote:
> On Wed, Jun 20, 2018 at 10:04 AM William A Rowe Jr <wr...@rowe-clan.net>
> wrote:
> >
> > Question, what is a server_conf, before httpd's argv[] array
> > has even been processed?
>
> It's NULL, but these (log.c only) uses should be well after.
>
In fact, some of these are argv[] handlers, but as long as we have
server_conf of NULL, then your patch should be a no-op in such cases.
I'd expect any APLOG_STARTUP phase messages to predate the
successful creation of server_conf.
The exception would be if some of this processing happens after
pconf is torn down during a graceful restart, without resetting
server_conf to NULL, first. That might show up as graceful restart
segv's.
Re: svn commit: r1833827 - /httpd/httpd/trunk/server/log.c
Posted by Eric Covener <co...@gmail.com>.
On Wed, Jun 20, 2018 at 10:04 AM William A Rowe Jr <wr...@rowe-clan.net> wrote:
>
> Question, what is a server_conf, before httpd's argv[] array
> has even been processed?
It's NULL, but these (log.c only) uses should be well after.
Re: svn commit: r1833827 - /httpd/httpd/trunk/server/log.c
Posted by William A Rowe Jr <wr...@rowe-clan.net>.
Question, what is a server_conf, before httpd's argv[] array
has even been processed?
On Tue, Jun 19, 2018 at 7:07 AM, <co...@apache.org> wrote:
> Author: covener
> Date: Tue Jun 19 12:07:19 2018
> New Revision: 1833827
>
> URL: http://svn.apache.org/viewvc?rev=1833827&view=rev
> Log:
> add server_rec to log.c fatal startup errors
>
> Not strictly necessary for trunk, but in 2.4.x if the main ErrorLog is
> using syslog, these messages are lost. In trunk, the low-level logging
> routines reach up and find the syslog provider when no server_rec is
> provided
> but that backport is stalled.
>
>
>
> Modified:
> httpd/httpd/trunk/server/log.c
>
> Modified: httpd/httpd/trunk/server/log.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/server/log.
> c?rev=1833827&r1=1833826&r2=1833827&view=diff
> ============================================================
> ==================
> --- httpd/httpd/trunk/server/log.c (original)
> +++ httpd/httpd/trunk/server/log.c Tue Jun 19 12:07:19 2018
> @@ -185,14 +185,14 @@ AP_DECLARE(apr_status_t) ap_replace_stde
> char *filename = ap_server_root_relative(p, fname);
> if (!filename) {
> ap_log_error(APLOG_MARK, APLOG_STARTUP|APLOG_CRIT,
> - APR_EBADPATH, NULL, APLOGNO(00085) "Invalid -E error
> log file %s",
> + APR_EBADPATH, ap_server_conf, APLOGNO(00085)
> "Invalid -E error log file %s",
> fname);
> return APR_EBADPATH;
> }
> if ((rc = apr_file_open(&stderr_file, filename,
> APR_APPEND | APR_WRITE | APR_CREATE |
> APR_LARGEFILE,
> APR_OS_DEFAULT, p)) != APR_SUCCESS) {
> - ap_log_error(APLOG_MARK, APLOG_STARTUP, rc, NULL, APLOGNO(00086)
> + ap_log_error(APLOG_MARK, APLOG_STARTUP, rc, ap_server_conf,
> APLOGNO(00086)
> "%s: could not open error log file %s.",
> ap_server_argv0, fname);
> return rc;
> @@ -324,7 +324,7 @@ static int open_error_log(server_rec *s,
> * child inherits the parents stderr. */
> rc = log_child(p, fname, &dummy, cmdtype, is_main);
> if (rc != APR_SUCCESS) {
> - ap_log_error(APLOG_MARK, APLOG_STARTUP, rc, NULL,
> APLOGNO(00089)
> + ap_log_error(APLOG_MARK, APLOG_STARTUP, rc, ap_server_conf,
> APLOGNO(00089)
> "Couldn't start ErrorLog process '%s'.",
> s->error_fname + 1);
> return DONE;
> @@ -343,7 +343,7 @@ static int open_error_log(server_rec *s,
> else {
> fname = ap_server_root_relative(p, s->error_fname);
> if (!fname) {
> - ap_log_error(APLOG_MARK, APLOG_STARTUP, APR_EBADPATH, NULL,
> APLOGNO(00090)
> + ap_log_error(APLOG_MARK, APLOG_STARTUP, APR_EBADPATH,
> ap_server_conf, APLOGNO(00090)
> "%s: Invalid error log path %s.",
> ap_server_argv0, s->error_fname);
> return DONE;
> @@ -351,7 +351,7 @@ static int open_error_log(server_rec *s,
> if ((rc = apr_file_open(&s->error_log, fname,
> APR_APPEND | APR_WRITE | APR_CREATE |
> APR_LARGEFILE,
> APR_OS_DEFAULT, p)) != APR_SUCCESS) {
> - ap_log_error(APLOG_MARK, APLOG_STARTUP, rc, NULL,
> APLOGNO(00091)
> + ap_log_error(APLOG_MARK, APLOG_STARTUP, rc, ap_server_conf,
> APLOGNO(00091)
> "%s: could not open error log file %s.",
> ap_server_argv0, fname);
> return DONE;
> @@ -1532,7 +1532,7 @@ AP_DECLARE(void) ap_log_pid(apr_pool_t *
> fname = ap_runtime_dir_relative(p, filename);
> if (!fname) {
> ap_log_error(APLOG_MARK, APLOG_STARTUP|APLOG_CRIT, APR_EBADPATH,
> - NULL, APLOGNO(00097) "Invalid PID file path %s,
> ignoring.", filename);
> + ap_server_conf, APLOGNO(00097) "Invalid PID file
> path %s, ignoring.", filename);
> return;
> }
>
> @@ -1585,7 +1585,7 @@ AP_DECLARE(apr_status_t) ap_read_pid(apr
> fname = ap_runtime_dir_relative(p, filename);
> if (!fname) {
> ap_log_error(APLOG_MARK, APLOG_STARTUP|APLOG_CRIT, APR_EBADPATH,
> - NULL, APLOGNO(00101) "Invalid PID file path %s,
> ignoring.", filename);
> + ap_server_conf, APLOGNO(00101) "Invalid PID file
> path %s, ignoring.", filename);
> return APR_EGENERAL;
> }
>
> @@ -1657,7 +1657,7 @@ static apr_status_t piped_log_spawn(pipe
> != APR_SUCCESS) ||
> ((status = apr_procattr_error_check_set(procattr, 1)) !=
> APR_SUCCESS)) {
> /* Something bad happened, give up and go away. */
> - ap_log_error(APLOG_MARK, APLOG_STARTUP, status, NULL,
> APLOGNO(00103)
> + ap_log_error(APLOG_MARK, APLOG_STARTUP, status, ap_server_conf,
> APLOGNO(00103)
> "piped_log_spawn: unable to setup child process
> '%s'",
> pl->program);
> }
> @@ -1682,7 +1682,7 @@ static apr_status_t piped_log_spawn(pipe
> }
> else {
> /* Something bad happened, give up and go away. */
> - ap_log_error(APLOG_MARK, APLOG_STARTUP, status, NULL,
> APLOGNO(00104)
> + ap_log_error(APLOG_MARK, APLOG_STARTUP, status,
> ap_server_conf, APLOGNO(00104)
> "unable to start piped log program '%s'",
> pl->program);
> }
> @@ -1812,7 +1812,7 @@ AP_DECLARE(piped_log *) ap_open_piped_lo
>
> rc = log_child(p, program, &dummy, cmdtype, 0);
> if (rc != APR_SUCCESS) {
> - ap_log_error(APLOG_MARK, APLOG_STARTUP, rc, NULL, APLOGNO(00108)
> + ap_log_error(APLOG_MARK, APLOG_STARTUP, rc, ap_server_conf,
> APLOGNO(00108)
> "Couldn't start piped log process '%s'.",
> (program == NULL) ? "NULL" : program);
> return NULL;
>
>
>