httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Rainer Jung <rainer.j...@kippdata.de>
Subject Re: svn commit: r1722154 - in /httpd/httpd/trunk: docs/log-message-tags/next-number modules/core/mod_watchdog.c
Date Mon, 19 Dec 2016 20:24:47 GMT
Late review due to compiler warning in 2.4.25 release testing, see below:

Am 29.12.2015 um 16:38 schrieb jim@apache.org:
> Author: jim
> Date: Tue Dec 29 15:38:29 2015
> New Revision: 1722154
>
> URL: http://svn.apache.org/viewvc?rev=1722154&view=rev
> Log:
> Update w/ better logging
>
> Modified:
>     httpd/httpd/trunk/docs/log-message-tags/next-number
>     httpd/httpd/trunk/modules/core/mod_watchdog.c
>
> Modified: httpd/httpd/trunk/docs/log-message-tags/next-number
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/docs/log-message-tags/next-number?rev=1722154&r1=1722153&r2=1722154&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/docs/log-message-tags/next-number (original)
> +++ httpd/httpd/trunk/docs/log-message-tags/next-number Tue Dec 29 15:38:29 2015
> @@ -1 +1 @@
> -2972
> +2982
>
> Modified: httpd/httpd/trunk/modules/core/mod_watchdog.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/core/mod_watchdog.c?rev=1722154&r1=1722153&r2=1722154&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/modules/core/mod_watchdog.c (original)
> +++ httpd/httpd/trunk/modules/core/mod_watchdog.c Tue Dec 29 15:38:29 2015
> @@ -155,8 +155,8 @@ static void* APR_THREAD_FUNC wd_worker(a
>      if (w->is_running) {
>          watchdog_list_t *wl = w->callbacks;
>          ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, wd_server_conf->s,
> -                     "%sWatchdog (%s) running",
> -                     w->singleton ? "Singleton" : "", w->name);
> +                     APLOGNO(02972) "%sWatchdog (%s) running",
> +                     w->singleton ? "Singleton " : "", w->name);
>          apr_time_clock_hires(w->pool);
>          if (wl) {
>              apr_pool_t *ctx = NULL;
> @@ -251,8 +251,8 @@ static void* APR_THREAD_FUNC wd_worker(a
>          }
>      }
>      ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, wd_server_conf->s,
> -                 "%sWatchdog (%s) stopping",
> -                 w->singleton ? "Singleton" : "", w->name);
> +                 APLOGNO(02973) "%sWatchdog (%s) stopping",
> +                 w->singleton ? "Singleton " : "", w->name);
>
>      if (locked)
>          apr_proc_mutex_unlock(w->mutex);
> @@ -456,11 +456,16 @@ static int wd_post_config_hook(apr_pool_
>          const ap_list_provider_names_t *wn;
>          int i;
>
> +        ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s, APLOGNO(02974)
> +                "Watchdog: found parent providers.");
> +
>          wn = (ap_list_provider_names_t *)wl->elts;
>          for (i = 0; i < wl->nelts; i++) {
>              ap_watchdog_t *w = ap_lookup_provider(AP_WATCHDOG_PGROUP,
>                                                    wn[i].provider_name,
>                                                    AP_WATCHDOG_PVERSION);
> +            ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s, APLOGNO(02975)
> +                    "Watchdog: Looking for parent (%s).", wn[i].provider_name);
>              if (w) {
>                  if (!w->active) {
>                      int status = ap_run_watchdog_need(s, w->name, 1,
> @@ -481,6 +486,8 @@ static int wd_post_config_hook(apr_pool_
>                                  "Watchdog: Failed to create parent worker thread.");
>                          return rv;
>                      }
> +                    ap_log_error(APLOG_MARK, APLOG_DEBUG, rv, s, APLOGNO(02976)
> +                            "Watchdog: Created parent worker thread (%s).", w->name);
>                      wd_server_conf->parent_workers++;
>                  }
>              }
> @@ -495,13 +502,17 @@ static int wd_post_config_hook(apr_pool_
>                                              AP_WATCHDOG_CVERSION))) {
>          const ap_list_provider_names_t *wn;
>          int i;
> +        ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s, APLOGNO(02977)
> +                "Watchdog: found child providers.");
>
>          wn = (ap_list_provider_names_t *)wl->elts;
>          for (i = 0; i < wl->nelts; i++) {
>              ap_watchdog_t *w = ap_lookup_provider(AP_WATCHDOG_PGROUP,
>                                                    wn[i].provider_name,
>                                                    AP_WATCHDOG_CVERSION);
> -            if (w) {
> +            ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s, APLOGNO(02978)
> +                    "Watchdog: Looking for child (%s).", wn[i].provider_name);
> +           if (w) {
>                  if (!w->active) {
>                      int status = ap_run_watchdog_need(s, w->name, 0,
>                                                        w->singleton);
> @@ -524,7 +535,9 @@ static int wd_post_config_hook(apr_pool_
>                              return rv;
>                          }
>                      }
> -                    wd_server_conf->child_workers++;
> +                    ap_log_error(APLOG_MARK, APLOG_DEBUG, rv, s, APLOGNO(02979)
> +                            "Watchdog: Created child worker thread (%s).", w->name);
 > +                   wd_server_conf->child_workers++;

Here "rv" is used potentially uninitialized as a log argument. More 
complete snippet:

526                 if (w->active) {
527                     /* We have some callbacks registered.
528                      * Create mutexes for singleton watchdogs
529                      */
530                     if (w->singleton) {
531                         rv = ap_proc_mutex_create(&w->mutex, NULL, 
wd_proc_mutex_type,
532                                                   w->name, s,
533 
wd_server_conf->pool, 0);
534                         if (rv != APR_SUCCESS) {
535                             return rv;
536                         }
537                     }
538                     ap_log_error(APLOG_MARK, APLOG_DEBUG, rv, s, 
APLOGNO(02979)
539                             "Watchdog: Created child worker thread 
(%s).", w->name);
540                     wd_server_conf->child_workers++;
541                 }

"rv" is only set in the w->singleton case. The log message itself also 
does not really reflect what is happening here, so I'm unsure what the 
correct fix is, replacing "rv" by "0" in the logging, or move the log 
message into the w->singleton block and log mutex creation instead of 
thread creation?

Regards,

Rainer

Mime
View raw message