From 1c0d617742e01c68d5846032e7cf4941bf1d9945 Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Thu, 26 Feb 2015 00:02:29 +0900 Subject: [PATCH] nghttpx: Rename WorkerConfig as LogConfig This is a sign that we only use thread-local storage for logging only. --- src/Makefile.am | 2 +- src/shrpx.cc | 14 ++--- src/shrpx_client_handler.cc | 1 - src/shrpx_connection_handler.cc | 1 - src/shrpx_http.cc | 1 - src/shrpx_http2_downstream_connection.cc | 1 - src/shrpx_http2_session.cc | 1 - src/shrpx_http2_upstream.cc | 1 - src/shrpx_http_downstream_connection.cc | 4 +- src/shrpx_https_upstream.cc | 4 +- src/shrpx_log.cc | 51 +++++++++---------- src/shrpx_log.h | 6 ++- ...x_worker_config.cc => shrpx_log_config.cc} | 8 +-- ...rpx_worker_config.h => shrpx_log_config.h} | 21 +++----- src/shrpx_spdy_upstream.cc | 1 - src/shrpx_worker.cc | 8 ++- 16 files changed, 56 insertions(+), 69 deletions(-) rename src/{shrpx_worker_config.cc => shrpx_log_config.cc} (89%) rename src/{shrpx_worker_config.h => shrpx_log_config.h} (85%) diff --git a/src/Makefile.am b/src/Makefile.am index 4b274327..1b13db54 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -114,7 +114,7 @@ NGHTTPX_SRCS = \ shrpx_io_control.cc shrpx_io_control.h \ shrpx_ssl.cc shrpx_ssl.h \ shrpx_worker.cc shrpx_worker.h \ - shrpx_worker_config.cc shrpx_worker_config.h \ + shrpx_log_config.cc shrpx_log_config.h \ shrpx_connect_blocker.cc shrpx_connect_blocker.h \ shrpx_downstream_connection_pool.cc shrpx_downstream_connection_pool.h \ shrpx_rate_limit.cc shrpx_rate_limit.h \ diff --git a/src/shrpx.cc b/src/shrpx.cc index 30162219..7121077b 100644 --- a/src/shrpx.cc +++ b/src/shrpx.cc @@ -62,7 +62,7 @@ #include "shrpx_config.h" #include "shrpx_connection_handler.h" #include "shrpx_ssl.h" -#include "shrpx_worker_config.h" +#include "shrpx_log_config.h" #include "shrpx_worker.h" #include "shrpx_accept_handler.h" #include "shrpx_http2_upstream.h" @@ -418,7 +418,7 @@ void reopen_log_signal_cb(struct ev_loop *loop, ev_signal *w, int revents) { auto conn_handler = static_cast(w->data); if (LOG_ENABLED(INFO)) { - LOG(INFO) << "Reopening log files: worker_info(" << worker_config << ")"; + LOG(INFO) << "Reopening log files: main"; } (void)reopen_log_files(); @@ -1884,16 +1884,16 @@ int main(int argc, char **argv) { } if (get_config()->uid != 0) { - if (worker_config->accesslog_fd != -1 && - fchown(worker_config->accesslog_fd, get_config()->uid, + if (log_config->accesslog_fd != -1 && + fchown(log_config->accesslog_fd, get_config()->uid, get_config()->gid) == -1) { auto error = errno; LOG(WARN) << "Changing owner of access log file failed: " << strerror(error); } - if (worker_config->errorlog_fd != -1 && - fchown(worker_config->errorlog_fd, get_config()->uid, - get_config()->gid) == -1) { + if (log_config->errorlog_fd != -1 && + fchown(log_config->errorlog_fd, get_config()->uid, get_config()->gid) == + -1) { auto error = errno; LOG(WARN) << "Changing owner of error log file failed: " << strerror(error); diff --git a/src/shrpx_client_handler.cc b/src/shrpx_client_handler.cc index a0e1b131..305f17db 100644 --- a/src/shrpx_client_handler.cc +++ b/src/shrpx_client_handler.cc @@ -35,7 +35,6 @@ #include "shrpx_http2_downstream_connection.h" #include "shrpx_ssl.h" #include "shrpx_worker.h" -#include "shrpx_worker_config.h" #include "shrpx_downstream_connection_pool.h" #include "shrpx_downstream.h" #ifdef HAVE_SPDYLAY diff --git a/src/shrpx_connection_handler.cc b/src/shrpx_connection_handler.cc index 29442891..91873ff4 100644 --- a/src/shrpx_connection_handler.cc +++ b/src/shrpx_connection_handler.cc @@ -32,7 +32,6 @@ #include "shrpx_client_handler.h" #include "shrpx_ssl.h" #include "shrpx_worker.h" -#include "shrpx_worker_config.h" #include "shrpx_config.h" #include "shrpx_http2_session.h" #include "shrpx_connect_blocker.h" diff --git a/src/shrpx_http.cc b/src/shrpx_http.cc index 004b3b64..5867c02c 100644 --- a/src/shrpx_http.cc +++ b/src/shrpx_http.cc @@ -26,7 +26,6 @@ #include "shrpx_config.h" #include "shrpx_log.h" -#include "shrpx_worker_config.h" #include "http2.h" #include "util.h" diff --git a/src/shrpx_http2_downstream_connection.cc b/src/shrpx_http2_downstream_connection.cc index 775d2215..a3aae8c7 100644 --- a/src/shrpx_http2_downstream_connection.cc +++ b/src/shrpx_http2_downstream_connection.cc @@ -35,7 +35,6 @@ #include "shrpx_error.h" #include "shrpx_http.h" #include "shrpx_http2_session.h" -#include "shrpx_worker_config.h" #include "http2.h" #include "util.h" diff --git a/src/shrpx_http2_session.cc b/src/shrpx_http2_session.cc index 3a0d5207..f6522089 100644 --- a/src/shrpx_http2_session.cc +++ b/src/shrpx_http2_session.cc @@ -39,7 +39,6 @@ #include "shrpx_client_handler.h" #include "shrpx_ssl.h" #include "shrpx_http.h" -#include "shrpx_worker_config.h" #include "http2.h" #include "util.h" #include "base64.h" diff --git a/src/shrpx_http2_upstream.cc b/src/shrpx_http2_upstream.cc index eae890cd..1fcdde8d 100644 --- a/src/shrpx_http2_upstream.cc +++ b/src/shrpx_http2_upstream.cc @@ -35,7 +35,6 @@ #include "shrpx_downstream_connection.h" #include "shrpx_config.h" #include "shrpx_http.h" -#include "shrpx_worker_config.h" #include "shrpx_worker.h" #include "http2.h" #include "util.h" diff --git a/src/shrpx_http_downstream_connection.cc b/src/shrpx_http_downstream_connection.cc index 6f123dea..b251f64c 100644 --- a/src/shrpx_http_downstream_connection.cc +++ b/src/shrpx_http_downstream_connection.cc @@ -30,7 +30,7 @@ #include "shrpx_config.h" #include "shrpx_error.h" #include "shrpx_http.h" -#include "shrpx_worker_config.h" +#include "shrpx_log_config.h" #include "shrpx_connect_blocker.h" #include "shrpx_downstream_connection_pool.h" #include "shrpx_worker.h" @@ -358,7 +358,7 @@ int HttpDownstreamConnection::push_request_headers() { if (LOG_ENABLED(INFO)) { const char *hdrp; std::string nhdrs; - if (worker_config->errorlog_tty) { + if (log_config->errorlog_tty) { nhdrs = http::colorizeHeaders(hdrs.c_str()); hdrp = nhdrs.c_str(); } else { diff --git a/src/shrpx_https_upstream.cc b/src/shrpx_https_upstream.cc index 7bb954d9..7a7d1e46 100644 --- a/src/shrpx_https_upstream.cc +++ b/src/shrpx_https_upstream.cc @@ -34,7 +34,7 @@ #include "shrpx_http.h" #include "shrpx_config.h" #include "shrpx_error.h" -#include "shrpx_worker_config.h" +#include "shrpx_log_config.h" #include "shrpx_worker.h" #include "http2.h" #include "util.h" @@ -810,7 +810,7 @@ int HttpsUpstream::on_downstream_abort_request(Downstream *downstream, void HttpsUpstream::log_response_headers(const std::string &hdrs) const { const char *hdrp; std::string nhdrs; - if (worker_config->errorlog_tty) { + if (log_config->errorlog_tty) { nhdrs = http::colorizeHeaders(hdrs.c_str()); hdrp = nhdrs.c_str(); } else { diff --git a/src/shrpx_log.cc b/src/shrpx_log.cc index ca669eac..407ae5e3 100644 --- a/src/shrpx_log.cc +++ b/src/shrpx_log.cc @@ -37,7 +37,6 @@ #include "shrpx_config.h" #include "shrpx_downstream.h" -#include "shrpx_worker_config.h" #include "util.h" #include "template.h" @@ -100,10 +99,10 @@ Log::~Log() { return; } - auto wconf = worker_config; + auto lgconf = log_config; if (!log_enabled(severity_) || - (wconf->errorlog_fd == -1 && !get_config()->errorlog_syslog)) { + (lgconf->errorlog_fd == -1 && !get_config()->errorlog_syslog)) { return; } @@ -121,10 +120,10 @@ Log::~Log() { } char buf[4096]; - auto tty = wconf->errorlog_tty; + auto tty = lgconf->errorlog_tty; - wconf->update_tstamp(std::chrono::system_clock::now()); - auto &time_local = wconf->time_local_str; + lgconf->update_tstamp(std::chrono::system_clock::now()); + auto &time_local = lgconf->time_local_str; if (severity_ == NOTICE) { rv = snprintf(buf, sizeof(buf), "%s PID%d [%s%s%s] %s\n", @@ -145,7 +144,7 @@ Log::~Log() { auto nwrite = std::min(static_cast(rv), sizeof(buf) - 1); - while (write(wconf->errorlog_fd, buf, nwrite) == -1 && errno == EINTR) + while (write(lgconf->errorlog_fd, buf, nwrite) == -1 && errno == EINTR) ; } @@ -160,9 +159,9 @@ std::pair copy(const char *src, size_t avail, } // namespace void upstream_accesslog(const std::vector &lfv, LogSpec *lgsp) { - auto wconf = worker_config; + auto lgconf = log_config; - if (wconf->accesslog_fd == -1 && !get_config()->accesslog_syslog) { + if (lgconf->accesslog_fd == -1 && !get_config()->accesslog_syslog) { return; } @@ -173,9 +172,9 @@ void upstream_accesslog(const std::vector &lfv, LogSpec *lgsp) { auto p = buf; auto avail = sizeof(buf) - 2; - wconf->update_tstamp(lgsp->time_now); - auto &time_local = wconf->time_local_str; - auto &time_iso8601 = wconf->time_iso8601_str; + lgconf->update_tstamp(lgsp->time_now); + auto &time_local = lgconf->time_local_str; + auto &time_iso8601 = lgconf->time_iso8601_str; for (auto &lf : lfv) { switch (lf.type) { @@ -266,26 +265,26 @@ void upstream_accesslog(const std::vector &lfv, LogSpec *lgsp) { *p++ = '\n'; auto nwrite = p - buf; - while (write(wconf->accesslog_fd, buf, nwrite) == -1 && errno == EINTR) + while (write(lgconf->accesslog_fd, buf, nwrite) == -1 && errno == EINTR) ; } int reopen_log_files() { int res = 0; - auto wconf = worker_config; + auto lgconf = log_config; - if (wconf->accesslog_fd != -1) { - close(wconf->accesslog_fd); - wconf->accesslog_fd = -1; + if (lgconf->accesslog_fd != -1) { + close(lgconf->accesslog_fd); + lgconf->accesslog_fd = -1; } if (!get_config()->accesslog_syslog && get_config()->accesslog_file) { - wconf->accesslog_fd = + lgconf->accesslog_fd = util::reopen_log_file(get_config()->accesslog_file.get()); - if (wconf->accesslog_fd == -1) { + if (lgconf->accesslog_fd == -1) { LOG(ERROR) << "Failed to open accesslog file " << get_config()->accesslog_file.get(); res = -1; @@ -299,7 +298,7 @@ int reopen_log_files() { new_errorlog_fd = util::reopen_log_file(get_config()->errorlog_file.get()); if (new_errorlog_fd == -1) { - if (wconf->errorlog_fd != -1) { + if (lgconf->errorlog_fd != -1) { LOG(ERROR) << "Failed to open errorlog file " << get_config()->errorlog_file.get(); } else { @@ -311,15 +310,15 @@ int reopen_log_files() { } } - if (wconf->errorlog_fd != -1) { - close(wconf->errorlog_fd); - wconf->errorlog_fd = -1; - wconf->errorlog_tty = false; + if (lgconf->errorlog_fd != -1) { + close(lgconf->errorlog_fd); + lgconf->errorlog_fd = -1; + lgconf->errorlog_tty = false; } if (new_errorlog_fd != -1) { - wconf->errorlog_fd = new_errorlog_fd; - wconf->errorlog_tty = isatty(wconf->errorlog_fd); + lgconf->errorlog_fd = new_errorlog_fd; + lgconf->errorlog_tty = isatty(lgconf->errorlog_fd); } return res; diff --git a/src/shrpx_log.h b/src/shrpx_log.h index e4da4305..01358297 100644 --- a/src/shrpx_log.h +++ b/src/shrpx_log.h @@ -34,6 +34,8 @@ #include #include +#include "shrpx_log_config.h" + namespace shrpx { class Downstream; @@ -95,8 +97,8 @@ private: static int severity_thres_; }; -#define TTY_HTTP_HD (worker_config->errorlog_tty ? "\033[1;34m" : "") -#define TTY_RST (worker_config->errorlog_tty ? "\033[0m" : "") +#define TTY_HTTP_HD (log_config->errorlog_tty ? "\033[1;34m" : "") +#define TTY_RST (log_config->errorlog_tty ? "\033[0m" : "") enum LogFragmentType { SHRPX_LOGF_NONE, diff --git a/src/shrpx_worker_config.cc b/src/shrpx_log_config.cc similarity index 89% rename from src/shrpx_worker_config.cc rename to src/shrpx_log_config.cc index 5507c8d0..24fa15b5 100644 --- a/src/shrpx_worker_config.cc +++ b/src/shrpx_log_config.cc @@ -22,23 +22,23 @@ * OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION * WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. */ -#include "shrpx_worker_config.h" +#include "shrpx_log_config.h" #include "util.h" using namespace nghttp2; namespace shrpx { -WorkerConfig::WorkerConfig() +LogConfig::LogConfig() : accesslog_fd(-1), errorlog_fd(-1), errorlog_tty(false) {} #ifndef NOTHREADS thread_local #endif // NOTHREADS - WorkerConfig *worker_config = new WorkerConfig(); + LogConfig *log_config = new LogConfig(); void -WorkerConfig::update_tstamp(const std::chrono::system_clock::time_point &now) { +LogConfig::update_tstamp(const std::chrono::system_clock::time_point &now) { auto t0 = std::chrono::system_clock::to_time_t(time_str_updated_); auto t1 = std::chrono::system_clock::to_time_t(now); if (t0 == t1) { diff --git a/src/shrpx_worker_config.h b/src/shrpx_log_config.h similarity index 85% rename from src/shrpx_worker_config.h rename to src/shrpx_log_config.h index 273ab904..3fcc9b9e 100644 --- a/src/shrpx_worker_config.h +++ b/src/shrpx_log_config.h @@ -22,8 +22,8 @@ * OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION * WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. */ -#ifndef SHRPX_WORKER_CONFIG_H -#define SHRPX_WORKER_CONFIG_H +#ifndef SHRPX_LOG_CONFIG_H +#define SHRPX_LOG_CONFIG_H #include "shrpx.h" @@ -31,13 +31,7 @@ namespace shrpx { -namespace ssl { -class CertLookupTree; -} // namespace ssl - -struct TicketKeys; - -struct WorkerConfig { +struct LogConfig { std::chrono::system_clock::time_point time_str_updated_; std::string time_local_str; std::string time_iso8601_str; @@ -46,17 +40,18 @@ struct WorkerConfig { // true if errorlog_fd is referring to a terminal. bool errorlog_tty; - WorkerConfig(); + LogConfig(); void update_tstamp(const std::chrono::system_clock::time_point &now); }; -// We need WorkerConfig per thread +// We need LogConfig per thread to avoid data race around opening file +// descriptor for log files. extern #ifndef NOTHREADS thread_local #endif // NOTHREADS - WorkerConfig *worker_config; + LogConfig *log_config; } // namespace shrpx -#endif // SHRPX_WORKER_CONFIG_H +#endif // SHRPX_LOG_CONFIG_H diff --git a/src/shrpx_spdy_upstream.cc b/src/shrpx_spdy_upstream.cc index 6a2b95ad..095e3b24 100644 --- a/src/shrpx_spdy_upstream.cc +++ b/src/shrpx_spdy_upstream.cc @@ -36,7 +36,6 @@ #include "shrpx_downstream_connection.h" #include "shrpx_config.h" #include "shrpx_http.h" -#include "shrpx_worker_config.h" #include "http2.h" #include "util.h" #include "template.h" diff --git a/src/shrpx_worker.cc b/src/shrpx_worker.cc index 7cfa58ba..3ef524c5 100644 --- a/src/shrpx_worker.cc +++ b/src/shrpx_worker.cc @@ -32,7 +32,7 @@ #include "shrpx_log.h" #include "shrpx_client_handler.h" #include "shrpx_http2_session.h" -#include "shrpx_worker_config.h" +#include "shrpx_log_config.h" #include "shrpx_connect_blocker.h" #include "util.h" #include "template.h" @@ -137,8 +137,7 @@ void Worker::process_events() { } case RENEW_TICKET_KEYS: if (LOG_ENABLED(INFO)) { - WLOG(INFO, this) << "Renew ticket keys: worker_info(" << worker_config - << ")"; + WLOG(INFO, this) << "Renew ticket keys: worker(" << this << ")"; } ticket_keys_ = wev.ticket_keys; @@ -146,8 +145,7 @@ void Worker::process_events() { break; case REOPEN_LOG: if (LOG_ENABLED(INFO)) { - WLOG(INFO, this) << "Reopening log files: worker_info(" << worker_config - << ")"; + WLOG(INFO, this) << "Reopening log files: worker(" << this << ")"; } reopen_log_files();