You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by "William A. Rowe, Jr." <wr...@rowe-clan.net> on 2009/05/13 22:38:19 UTC
Re: svn commit: r735516 - /httpd/httpd/trunk/server/log.c
-1. There's really no excuse to abstract an abstraction, when you
could have simply added 'int special_stderr;' to the piped_log struct.
There are plenty of unnecessary changes here to accomplish what your
patch set out to do. Let's stick with KISS, this isn't Java code :)
You are also patching many different things in this patch; error message
formatting, etc etc. Commit such as individual patches so that the
significant changes are not lost in the noise.
Please revert the introduction of a _wrapper struct and let's simply
fix the piped_log structure?
rjung@apache.org wrote:
> Author: rjung
> Date: Sun Jan 18 10:19:45 2009
> New Revision: 735516
>
> URL: http://svn.apache.org/viewvc?rev=735516&view=rev
> Log:
> Piped error loggers should use the reliable pipes,
> i.e. they should be automatically restarted when they die
> similar to what happens for access loggers.
>
> Patch makes error loggers use the same code path as
> access loggers.
>
> Side effect: patch adds ap_server_root_relative() to the error
> logger path before spawning.
>
> Reviewed by R. Pluem.
>
> Patch needs to be tested on Windows.
>
> 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=735516&r1=735515&r2=735516&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/server/log.c (original)
> +++ httpd/httpd/trunk/server/log.c Sun Jan 18 10:19:45 2009
> @@ -141,6 +141,14 @@
>
> static apr_file_t *stderr_log = NULL;
>
> +/* wrap a piped_log* with additional info
> + * about special stderr treatment used for main
> + * error logger */
> +typedef struct piped_log_wrapper {
> + piped_log *pl;
> + int special_stderr;
> +} piped_log_wrapper;
> +
> /* track pipe handles to close in child process */
> typedef struct read_handle_t {
> struct read_handle_t *next;
> @@ -257,6 +265,7 @@
> "%s", description);
> }
>
> +#ifndef AP_HAVE_RELIABLE_PIPED_LOGS
> /* Create a child process running PROGNAME with a pipe connected to
> * the childs stdin. The write-end of the pipe will be placed in
> * *FPIN on successful return. If dummy_stderr is non-zero, the
> @@ -310,6 +319,11 @@
>
> return rc;
> }
> +#endif
> +
> +/* forward declaration */
> +static piped_log* piped_log_open(apr_pool_t *p, const char *program,
> + int special_stderr);
>
> /* Open the error log for the given server_rec. If IS_MAIN is
> * non-zero, s is the main server. */
> @@ -319,20 +333,28 @@
> int rc;
>
> if (*s->error_fname == '|') {
> - apr_file_t *dummy = NULL;
> + piped_log *pl;
> + fname = ap_server_root_relative(p, s->error_fname + 1);
> +
> + if (!fname) {
> + ap_log_error(APLOG_MARK, APLOG_STARTUP, APR_EBADPATH, NULL,
> + "%s: Invalid error log path '%s'.",
> + ap_server_argv0, s->error_fname);
> + return NULL;
> + }
>
> /* Spawn a new child logger. If this is the main server_rec,
> * the new child must use a dummy stderr since the current
> * stderr might be a pipe to the old logger. Otherwise, the
> * child inherits the parents stderr. */
> - rc = log_child(p, s->error_fname + 1, &dummy, is_main);
> - if (rc != APR_SUCCESS) {
> - ap_log_error(APLOG_MARK, APLOG_STARTUP, rc, NULL,
> - "Couldn't start ErrorLog process");
> + pl = piped_log_open(p, fname, is_main);
> + if (pl == NULL) {
> + ap_log_error(APLOG_MARK, APLOG_STARTUP, 0, NULL,
> + "%s: Couldn't start ErrorLog process '%s'.",
> + ap_server_argv0, fname);
> return DONE;
> }
> -
> - s->error_log = dummy;
> + s->error_log = ap_piped_log_write_fd(pl);
> }
>
> #ifdef HAVE_SYSLOG
> @@ -361,7 +383,7 @@
> fname = ap_server_root_relative(p, s->error_fname);
> if (!fname) {
> ap_log_error(APLOG_MARK, APLOG_STARTUP, APR_EBADPATH, NULL,
> - "%s: Invalid error log path %s.",
> + "%s: Invalid error log path '%s'.",
> ap_server_argv0, s->error_fname);
> return DONE;
> }
> @@ -369,7 +391,7 @@
> APR_APPEND | APR_WRITE | APR_CREATE | APR_LARGEFILE,
> APR_OS_DEFAULT, p)) != APR_SUCCESS) {
> ap_log_error(APLOG_MARK, APLOG_STARTUP, rc, NULL,
> - "%s: could not open error log file %s.",
> + "%s: could not open error log file '%s'.",
> ap_server_argv0, fname);
> return DONE;
> }
> @@ -406,7 +428,7 @@
> */
> apr_pool_create(&stderr_p, apr_pool_parent_get(p));
> apr_pool_tag(stderr_p, "stderr_pool");
> -
> +
> if (open_error_log(s_main, 1, stderr_p) != OK) {
> return DONE;
> }
> @@ -414,7 +436,7 @@
> replace_stderr = 1;
> if (s_main->error_log) {
> apr_status_t rv;
> -
> +
> /* Replace existing stderr with new log. */
> apr_file_flush(s_main->error_log);
> rv = apr_file_dup2(stderr_log, s_main->error_log, stderr_p);
> @@ -868,16 +890,23 @@
>
> /* piped log support */
>
> +AP_DECLARE(piped_log *) ap_open_piped_log(apr_pool_t *p, const char *program)
> +{
> + return piped_log_open(p, program, 0);
> +}
> +
> #ifdef AP_HAVE_RELIABLE_PIPED_LOGS
> /* forward declaration */
> static void piped_log_maintenance(int reason, void *data, apr_wait_t status);
>
> /* Spawn the piped logger process pl->program. */
> -static apr_status_t piped_log_spawn(piped_log *pl)
> +static apr_status_t piped_log_spawn(piped_log_wrapper *plw)
> {
> apr_procattr_t *procattr;
> apr_proc_t *procnew = NULL;
> + apr_file_t *errfile;
> apr_status_t status;
> + piped_log *pl = plw->pl;
>
> if (((status = apr_procattr_create(&procattr, pl->p)) != APR_SUCCESS) ||
> ((status = apr_procattr_cmdtype_set(procattr,
> @@ -886,6 +915,14 @@
> ap_piped_log_read_fd(pl),
> ap_piped_log_write_fd(pl)))
> != APR_SUCCESS) ||
> + /* If special_stderr is non-zero, the stderr for the child will
> + * be the same as the stdout of the parent. Otherwise the child
> + * will inherit the stderr from the parent. */
> + (plw->special_stderr && (status = apr_file_open_stdout(&errfile, pl->p))
> + != APR_SUCCESS) ||
> + (plw->special_stderr && (status = apr_procattr_child_err_set(procattr,
> + errfile, NULL))
> + != APR_SUCCESS) ||
> ((status = apr_procattr_child_errfn_set(procattr, log_child_errfn))
> != APR_SUCCESS) ||
> ((status = apr_procattr_error_check_set(procattr, 1)) != APR_SUCCESS)) {
> @@ -895,6 +932,7 @@
> "piped_log_spawn: unable to setup child process '%s': %s",
> pl->program, apr_strerror(status, buf, sizeof(buf)));
> }
> +
> else {
> char **args;
> const char *pname;
> @@ -912,7 +950,7 @@
> * avoid a leak. */
> apr_file_close(procnew->in);
> procnew->in = NULL;
> - apr_proc_other_child_register(procnew, piped_log_maintenance, pl,
> + apr_proc_other_child_register(procnew, piped_log_maintenance, plw,
> ap_piped_log_write_fd(pl), pl->p);
> close_handle_in_child(pl->p, ap_piped_log_read_fd(pl));
> }
> @@ -931,7 +969,8 @@
>
> static void piped_log_maintenance(int reason, void *data, apr_wait_t status)
> {
> - piped_log *pl = data;
> + piped_log_wrapper *plw = data;
> + piped_log *pl = plw->pl;
> apr_status_t stats;
> int mpm_state;
>
> @@ -941,7 +980,7 @@
> pl->pid = NULL; /* in case we don't get it going again, this
> * tells other logic not to try to kill it
> */
> - apr_proc_other_child_unregister(pl);
> + apr_proc_other_child_unregister(plw);
> stats = ap_mpm_query(AP_MPMQ_MPM_STATE, &mpm_state);
> if (stats != APR_SUCCESS) {
> ap_log_error(APLOG_MARK, APLOG_STARTUP, 0, NULL,
> @@ -953,13 +992,26 @@
> ap_log_error(APLOG_MARK, APLOG_STARTUP, 0, NULL,
> "piped log program '%s' failed unexpectedly",
> pl->program);
> - if ((stats = piped_log_spawn(pl)) != APR_SUCCESS) {
> +
> + /* If special_stderr is non-zero, we need to get the write
> + * end of the pipe back from stderr before spawning. */
> + if ((plw->special_stderr && (stats = apr_file_dup(&ap_piped_log_write_fd(pl),
> + stderr_log, pl->p))
> + != APR_SUCCESS) ||
> + ((stats = piped_log_spawn(plw)) != APR_SUCCESS) ||
> + /* If special_stderr is non-zero, we need to close the write
> + * end of the pipe after spawning, because we write to it via
> + * stderr. */
> + (plw->special_stderr && (stats = apr_file_close(ap_piped_log_write_fd(pl)))
> + != APR_SUCCESS)) {
> +
> + char buf[120];
> +
> /* what can we do? This could be the error log we're having
> * problems opening up... */
> - char buf[120];
> ap_log_error(APLOG_MARK, APLOG_STARTUP, 0, NULL,
> "piped_log_maintenance: unable to respawn '%s': %s",
> - pl->program, apr_strerror(stats, buf, sizeof(buf)));
> + pl->program, apr_strerror(stats, buf, sizeof(buf)));
> }
> }
> break;
> @@ -1003,14 +1055,19 @@
> }
>
>
> -AP_DECLARE(piped_log *) ap_open_piped_log(apr_pool_t *p, const char *program)
> +static piped_log* piped_log_open(apr_pool_t *p, const char *program,
> + int special_stderr)
> {
> piped_log *pl;
> + piped_log_wrapper *plw;
>
> pl = apr_palloc(p, sizeof (*pl));
> pl->p = p;
> pl->program = apr_pstrdup(p, program);
> pl->pid = NULL;
> + plw = apr_palloc(p, sizeof (*plw));
> + plw->special_stderr = special_stderr;
> + plw->pl = pl;
> if (apr_file_pipe_create_ex(&ap_piped_log_read_fd(pl),
> &ap_piped_log_write_fd(pl),
> APR_FULL_BLOCK, p) != APR_SUCCESS) {
> @@ -1018,7 +1075,7 @@
> }
> apr_pool_cleanup_register(p, pl, piped_log_cleanup,
> piped_log_cleanup_for_exec);
> - if (piped_log_spawn(pl) != APR_SUCCESS) {
> + if (piped_log_spawn(plw) != APR_SUCCESS) {
> apr_pool_cleanup_kill(p, pl, piped_log_cleanup);
> apr_file_close(ap_piped_log_read_fd(pl));
> apr_file_close(ap_piped_log_write_fd(pl));
> @@ -1037,16 +1094,18 @@
> return APR_SUCCESS;
> }
>
> -AP_DECLARE(piped_log *) ap_open_piped_log(apr_pool_t *p, const char *program)
> +static piped_log* piped_log_open(apr_pool_t *p, const char *program,
> + int special_stderr)
> {
> piped_log *pl;
> apr_file_t *dummy = NULL;
> int rc;
>
> - rc = log_child(p, program, &dummy, 0);
> + rc = log_child(p, program, &dummy, special_stderr);
> if (rc != APR_SUCCESS) {
> ap_log_error(APLOG_MARK, APLOG_STARTUP, rc, NULL,
> - "Couldn't start piped log process");
> + "Couldn't start piped log process for '%s'",
> + (program == NULL) ? "NULL" : program);
> return NULL;
> }
>
> @@ -1072,4 +1131,3 @@
> const request_rec *r, apr_pool_t *pool,
> const char *errstr), (file, line, level,
> status, s, r, pool, errstr))
> -
>
>
>
>
Re: svn commit: r735516 - /httpd/httpd/trunk/server/log.c
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Rainer Jung wrote:
> On 13.05.2009 22:38, William A. Rowe, Jr. wrote:
>> -1. There's really no excuse to abstract an abstraction, when you
>> could have simply added 'int special_stderr;' to the piped_log struct.
>
> I reverted. Moving the new member to the existing piped_log structure
> makes it publicly visible. As for now we do not know any use case, where
> we actually want this to be set except for the main error log.
Please remember that any perl programmer is going to claim they can do just
about anything the core implementation can do in mod_perl, so I don't see
a reason to change that :) Even if we were to accomplish what you were
trying to do, we would rather do so with a fixed opaque extent. But...
>> Please revert the introduction of a _wrapper struct and let's simply
>> fix the piped_log structure?
>
> Do we really want to add it to the public API?
We don't "think like" apr in terms of transparency. The question we want
to ask is, what fields of piped_log belong in a public API, if any. And
if none, then let's move that out of the standard header.
Re: svn commit: r735516 - /httpd/httpd/trunk/server/log.c
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Rainer Jung wrote:
> On 14.05.2009 15:49, Joe Orton wrote:
>> On Thu, May 14, 2009 at 12:51:18PM +0200, Rainer Jung wrote:
>>> On 13.05.2009 22:38, William A. Rowe, Jr. wrote:
>>>> Please revert the introduction of a _wrapper struct and let's simply
>>>> fix the piped_log structure?
>>> Do we really want to add it to the public API?
>> There's no need for that structure to be exposed at all. I had a patch
>> to fix that lying around - updated for trunk below and can commit later
>> when I've had time to test.
>
> Right, we only actually use ap_piped_log_write_fd() directly after
> ap_close_piped_log() (apart from the things in log.c themselves) and
> none of the functions in the public log API need to expose the
> definition of the struct. Good catch!
+1; please commit, I have more log.c changes that rely on this branch not
breaking again soon, to resolve the extra sh interpreter introduced several
years ago.
> Is removing a structure definition from a public header file allowed
> between 2.2 and 2.4? Phrased differently: would it allowed for a module
> author to use the structure details available in the installed 2.2
> header file and can he expect to be able to simply recompile his module
> for 2.4?
httpd subversion bumps really are other code bases' minor bumps, the minor
bumps are truly API-changing major bumps. Which leaves major bumps to go
and completely rearrange the users expectations.
Yes; this is fine :)
Re: svn commit: r735516 - /httpd/httpd/trunk/server/log.c
Posted by Rainer Jung <ra...@kippdata.de>.
On 14.05.2009 15:49, Joe Orton wrote:
> On Thu, May 14, 2009 at 12:51:18PM +0200, Rainer Jung wrote:
>> On 13.05.2009 22:38, William A. Rowe, Jr. wrote:
>>> Please revert the introduction of a _wrapper struct and let's simply
>>> fix the piped_log structure?
>> Do we really want to add it to the public API?
>
> There's no need for that structure to be exposed at all. I had a patch
> to fix that lying around - updated for trunk below and can commit later
> when I've had time to test.
Right, we only actually use ap_piped_log_write_fd() directly after
ap_close_piped_log() (apart from the things in log.c themselves) and
none of the functions in the public log API need to expose the
definition of the struct. Good catch!
Is removing a structure definition from a public header file allowed
between 2.2 and 2.4? Phrased differently: would it allowed for a module
author to use the structure details available in the installed 2.2
header file and can he expect to be able to simply recompile his module
for 2.4?
Rainer
Re: svn commit: r735516 - /httpd/httpd/trunk/server/log.c
Posted by Rainer Jung <ra...@kippdata.de>.
On 14.05.2009 15:49, Joe Orton wrote:
> On Thu, May 14, 2009 at 12:51:18PM +0200, Rainer Jung wrote:
>> On 13.05.2009 22:38, William A. Rowe, Jr. wrote:
>>> Please revert the introduction of a _wrapper struct and let's simply
>>> fix the piped_log structure?
>> Do we really want to add it to the public API?
>
> There's no need for that structure to be exposed at all. I had a patch
> to fix that lying around - updated for trunk below and can commit later
> when I've had time to test.
I did some testing, looks good (only needed to add AP_DECLARE to the two
functions moved into log.c to silence MSVC). Should I wait for your
tests, or should I apply?
Regards,
Rainer
Re: svn commit: r735516 - /httpd/httpd/trunk/server/log.c
Posted by Joe Orton <jo...@redhat.com>.
On Thu, May 14, 2009 at 12:51:18PM +0200, Rainer Jung wrote:
> On 13.05.2009 22:38, William A. Rowe, Jr. wrote:
> > Please revert the introduction of a _wrapper struct and let's simply
> > fix the piped_log structure?
>
> Do we really want to add it to the public API?
There's no need for that structure to be exposed at all. I had a patch
to fix that lying around - updated for trunk below and can commit later
when I've had time to test.
Index: server/log.c
===================================================================
--- server/log.c (revision 774761)
+++ server/log.c (working copy)
@@ -149,6 +149,37 @@
static read_handle_t *read_handles;
+/**
+ * @brief The piped logging structure.
+ *
+ * Piped logs are used to move functionality out of the main server.
+ * For example, log rotation is done with piped logs.
+ */
+struct piped_log {
+ /** The pool to use for the piped log */
+ apr_pool_t *p;
+ /** The pipe between the server and the logging process */
+ apr_file_t *read_fd, *write_fd;
+ /* XXX - an #ifdef that needs to be eliminated from public view. Shouldn't
+ * be hard */
+#ifdef AP_HAVE_RELIABLE_PIPED_LOGS
+ /** The name of the program the logging process is running */
+ char *program;
+ /** The pid of the logging process */
+ apr_proc_t *pid;
+#endif
+};
+
+apr_file_t *ap_piped_log_read_fd(piped_log *pl)
+{
+ return pl->read_fd;
+}
+
+apr_file_t *ap_piped_log_write_fd(piped_log *pl)
+{
+ return pl->write_fd;
+}
+
/* clear_handle_list() is called when plog is cleared; at that
* point we need to forget about our old list of pipe read
* handles. We let the plog cleanups close the actual pipes.
@@ -876,8 +907,8 @@
((status = apr_procattr_cmdtype_set(procattr,
APR_SHELLCMD_ENV)) != APR_SUCCESS) ||
((status = apr_procattr_child_in_set(procattr,
- ap_piped_log_read_fd(pl),
- ap_piped_log_write_fd(pl)))
+ pl->read_fd,
+ pl->write_fd))
!= APR_SUCCESS) ||
((status = apr_procattr_child_errfn_set(procattr, log_child_errfn))
!= APR_SUCCESS) ||
@@ -900,14 +931,14 @@
if (status == APR_SUCCESS) {
pl->pid = procnew;
- /* procnew->in was dup2'd from ap_piped_log_write_fd(pl);
+ /* procnew->in was dup2'd from pl->write_fd;
* since the original fd is still valid, close the copy to
* avoid a leak. */
apr_file_close(procnew->in);
procnew->in = NULL;
apr_proc_other_child_register(procnew, piped_log_maintenance, pl,
- ap_piped_log_write_fd(pl), pl->p);
- close_handle_in_child(pl->p, ap_piped_log_read_fd(pl));
+ pl->write_fd, pl->p);
+ close_handle_in_child(pl->p, pl->read_fd);
}
else {
char buf[120];
@@ -979,8 +1010,8 @@
{
piped_log *pl = data;
- apr_file_close(ap_piped_log_read_fd(pl));
- apr_file_close(ap_piped_log_write_fd(pl));
+ apr_file_close(pl->read_fd);
+ apr_file_close(pl->write_fd);
return APR_SUCCESS;
}
@@ -1004,8 +1035,8 @@
pl->p = p;
pl->program = apr_pstrdup(p, program);
pl->pid = NULL;
- if (apr_file_pipe_create_ex(&ap_piped_log_read_fd(pl),
- &ap_piped_log_write_fd(pl),
+ if (apr_file_pipe_create_ex(&pl->read_fd,
+ &pl->write_fd,
APR_FULL_BLOCK, p) != APR_SUCCESS) {
return NULL;
}
@@ -1013,8 +1044,8 @@
piped_log_cleanup_for_exec);
if (piped_log_spawn(pl) != APR_SUCCESS) {
apr_pool_cleanup_kill(p, pl, piped_log_cleanup);
- apr_file_close(ap_piped_log_read_fd(pl));
- apr_file_close(ap_piped_log_write_fd(pl));
+ apr_file_close(pl->read_fd);
+ apr_file_close(pl->write_fd);
return NULL;
}
return pl;
@@ -1026,7 +1057,7 @@
{
piped_log *pl = data;
- apr_file_close(ap_piped_log_write_fd(pl));
+ apr_file_close(pl->write_fd);
return APR_SUCCESS;
}
@@ -1046,8 +1077,8 @@
pl = apr_palloc(p, sizeof (*pl));
pl->p = p;
- ap_piped_log_read_fd(pl) = NULL;
- ap_piped_log_write_fd(pl) = dummy;
+ pl->read_fd = NULL;
+ pl->write_fd = dummy;
apr_pool_cleanup_register(p, pl, piped_log_cleanup, piped_log_cleanup);
return pl;
Index: include/http_log.h
===================================================================
--- include/http_log.h (revision 774761)
+++ include/http_log.h (working copy)
@@ -268,27 +268,6 @@
typedef struct piped_log piped_log;
/**
- * @brief The piped logging structure.
- *
- * Piped logs are used to move functionality out of the main server.
- * For example, log rotation is done with piped logs.
- */
-struct piped_log {
- /** The pool to use for the piped log */
- apr_pool_t *p;
- /** The pipe between the server and the logging process */
- apr_file_t *fds[2];
- /* XXX - an #ifdef that needs to be eliminated from public view. Shouldn't
- * be hard */
-#ifdef AP_HAVE_RELIABLE_PIPED_LOGS
- /** The name of the program the logging process is running */
- char *program;
- /** The pid of the logging process */
- apr_proc_t *pid;
-#endif
-};
-
-/**
* Open the piped log process
* @param p The pool to allocate out of
* @param program The program to run in the logging process
@@ -303,18 +282,18 @@
AP_DECLARE(void) ap_close_piped_log(piped_log *pl);
/**
- * A macro to access the read side of the piped log pipe
+ * A function to return the read side of the piped log pipe
* @param pl The piped log structure
* @return The native file descriptor
*/
-#define ap_piped_log_read_fd(pl) ((pl)->fds[0])
+AP_DECLARE(apr_file_t *) ap_piped_log_read_fd(piped_log *pl);
/**
- * A macro to access the write side of the piped log pipe
+ * A function to return the write side of the piped log pipe
* @param pl The piped log structure
* @return The native file descriptor
*/
-#define ap_piped_log_write_fd(pl) ((pl)->fds[1])
+AP_DECLARE(apr_file_t *) ap_piped_log_write_fd(piped_log *pl);
/**
* hook method to log error messages
Re: svn commit: r735516 - /httpd/httpd/trunk/server/log.c
Posted by Rainer Jung <ra...@kippdata.de>.
On 13.05.2009 22:38, William A. Rowe, Jr. wrote:
> -1. There's really no excuse to abstract an abstraction, when you
> could have simply added 'int special_stderr;' to the piped_log struct.
I reverted. Moving the new member to the existing piped_log structure
makes it publicly visible. As for now we do not know any use case, where
we actually want this to be set except for the main error log.
> There are plenty of unnecessary changes here to accomplish what your
> patch set out to do. Let's stick with KISS, this isn't Java code :)
>
> You are also patching many different things in this patch; error message
> formatting, etc etc. Commit such as individual patches so that the
> significant changes are not lost in the noise.
OK
> Please revert the introduction of a _wrapper struct and let's simply
> fix the piped_log structure?
Do we really want to add it to the public API?