From 62a6123fedb7ad5539a669de8dc88b983c0922e0 Mon Sep 17 00:00:00 2001 From: Willy Tarreau Date: Fri, 12 Apr 2013 18:13:46 +0200 Subject: [PATCH] BUG/MEDIUM: log: fix regression on log-format handling Commit a4312fa2 merged into dev18 improved log-format management by processing "log-format" and "unique-id-format" where they were declared, so that the faulty args could be reported with their correct line numbers. Unfortunately, the log-format parser considers the proxy mode (TCP/HTTP) and now if the directive is set before the "mode" statement, it can be rejected and report warnings. So we really need to parse these directives at the end of a section at least. Right now we do not have an "end of section" event, so we need to store the file name and line number for each of these directives, and take care of them at the end. One of the benefits is that now the line numbers can be inherited from the line passing "option httplog" even if it's in a defaults section. Future improvements should be performed to report line numbers in every log-format processed by the parser. --- include/types/proxy.h | 8 ++- src/cfgparse.c | 143 ++++++++++++++++++++++++------------------ src/haproxy.c | 12 ++-- src/proxy.c | 10 +-- 4 files changed, 101 insertions(+), 72 deletions(-) diff --git a/include/types/proxy.h b/include/types/proxy.h index 5729db80d..273fb8bc5 100644 --- a/include/types/proxy.h +++ b/include/types/proxy.h @@ -340,8 +340,6 @@ struct proxy { int no_options; /* PR_O_REDISP, PR_O_TRANSP, ... */ int no_options2; /* PR_O2_* */ - char *logformat_string; /* log format string */ - char *uniqueid_format_string; /* unique-id format string */ struct { char *file; /* file where the section appears */ int line; /* line where the section appears */ @@ -351,6 +349,12 @@ struct proxy { struct list bind; /* list of bind settings */ struct list listeners; /* list of listeners belonging to this frontend */ struct arg_list args; /* sample arg list that need to be resolved */ + char *logformat_string; /* log format string */ + char *lfs_file; /* file name where the logformat string appears (strdup) */ + int lfs_line; /* file name where the logformat string appears */ + char *uniqueid_format_string; /* unique-id format string */ + char *uif_file; /* file name where the unique-id-format string appears (strdup) */ + int uif_line; /* file name where the unique-id-format string appears */ } conf; /* config information */ void *parent; /* parent of the proxy when applicable */ struct comp *comp; /* http compression */ diff --git a/src/cfgparse.c b/src/cfgparse.c index 49a91c046..cc515a205 100644 --- a/src/cfgparse.c +++ b/src/cfgparse.c @@ -1837,12 +1837,17 @@ int cfg_parse_listen(const char *file, int linenum, char **args, int kwm) curproxy->defbe.name = strdup(defproxy.defbe.name); /* get either a pointer to the logformat string or a copy of it */ - curproxy->logformat_string = defproxy.logformat_string; - if (curproxy->logformat_string && - curproxy->logformat_string != default_http_log_format && - curproxy->logformat_string != default_tcp_log_format && - curproxy->logformat_string != clf_http_log_format) - curproxy->logformat_string = strdup(curproxy->logformat_string); + curproxy->conf.logformat_string = defproxy.conf.logformat_string; + if (curproxy->conf.logformat_string && + curproxy->conf.logformat_string != default_http_log_format && + curproxy->conf.logformat_string != default_tcp_log_format && + curproxy->conf.logformat_string != clf_http_log_format) + curproxy->conf.logformat_string = strdup(curproxy->conf.logformat_string); + + if (defproxy.conf.lfs_file) { + curproxy->conf.lfs_file = strdup(defproxy.conf.lfs_file); + curproxy->conf.lfs_line = defproxy.conf.lfs_line; + } } if (curproxy->cap & PR_CAP_BE) { @@ -1867,9 +1872,14 @@ int cfg_parse_listen(const char *file, int linenum, char **args, int kwm) LIST_ADDQ(&curproxy->logsrvs, &node->list); } - curproxy->uniqueid_format_string = defproxy.uniqueid_format_string; - if (curproxy->uniqueid_format_string) - curproxy->uniqueid_format_string = strdup(curproxy->uniqueid_format_string); + curproxy->conf.uniqueid_format_string = defproxy.conf.uniqueid_format_string; + if (curproxy->conf.uniqueid_format_string) + curproxy->conf.uniqueid_format_string = strdup(curproxy->conf.uniqueid_format_string); + + if (defproxy.conf.uif_file) { + curproxy->conf.uif_file = strdup(defproxy.conf.uif_file); + curproxy->conf.uif_line = defproxy.conf.uif_line; + } /* copy default header unique id */ if (defproxy.header_unique_id) @@ -1912,12 +1922,14 @@ int cfg_parse_listen(const char *file, int linenum, char **args, int kwm) free(defproxy.expect_str); if (defproxy.expect_regex) regfree(defproxy.expect_regex); - if (defproxy.logformat_string != default_http_log_format && - defproxy.logformat_string != default_tcp_log_format && - defproxy.logformat_string != clf_http_log_format) - free(defproxy.logformat_string); + if (defproxy.conf.logformat_string != default_http_log_format && + defproxy.conf.logformat_string != default_tcp_log_format && + defproxy.conf.logformat_string != clf_http_log_format) + free(defproxy.conf.logformat_string); - free(defproxy.uniqueid_format_string); + free(defproxy.conf.uniqueid_format_string); + free(defproxy.conf.lfs_file); + free(defproxy.conf.uif_file); for (rc = 0; rc < HTTP_ERR_SIZE; rc++) chunk_destroy(&defproxy.errmsg[rc]); @@ -3401,19 +3413,27 @@ int cfg_parse_listen(const char *file, int linenum, char **args, int kwm) goto out; } } - if (curproxy->logformat_string != default_http_log_format && - curproxy->logformat_string != default_tcp_log_format && - curproxy->logformat_string != clf_http_log_format) - free(curproxy->logformat_string); - curproxy->logformat_string = logformat; + if (curproxy->conf.logformat_string != default_http_log_format && + curproxy->conf.logformat_string != default_tcp_log_format && + curproxy->conf.logformat_string != clf_http_log_format) + free(curproxy->conf.logformat_string); + curproxy->conf.logformat_string = logformat; + + free(curproxy->conf.lfs_file); + curproxy->conf.lfs_file = strdup(curproxy->conf.args.file); + curproxy->conf.lfs_line = curproxy->conf.args.line; } else if (!strcmp(args[1], "tcplog")) { /* generate a detailed TCP log */ - if (curproxy->logformat_string != default_http_log_format && - curproxy->logformat_string != default_tcp_log_format && - curproxy->logformat_string != clf_http_log_format) - free(curproxy->logformat_string); - curproxy->logformat_string = default_tcp_log_format; + if (curproxy->conf.logformat_string != default_http_log_format && + curproxy->conf.logformat_string != default_tcp_log_format && + curproxy->conf.logformat_string != clf_http_log_format) + free(curproxy->conf.logformat_string); + curproxy->conf.logformat_string = default_tcp_log_format; + + free(curproxy->conf.lfs_file); + curproxy->conf.lfs_file = strdup(curproxy->conf.args.file); + curproxy->conf.lfs_line = curproxy->conf.args.line; } else if (!strcmp(args[1], "tcpka")) { /* enable TCP keep-alives on client and server sessions */ @@ -4829,20 +4849,12 @@ int cfg_parse_listen(const char *file, int linenum, char **args, int kwm) err_code |= ERR_ALERT | ERR_FATAL; goto out; } - free(curproxy->uniqueid_format_string); - curproxy->uniqueid_format_string = strdup(args[1]); + free(curproxy->conf.uniqueid_format_string); + curproxy->conf.uniqueid_format_string = strdup(args[1]); - /* get a chance to improve log-format error reporting by - * reporting the correct line-number when possible. - */ - if (curproxy != &defproxy) { - curproxy->conf.args.ctx = ARGC_UIF; - if (curproxy->uniqueid_format_string) - parse_logformat_string(curproxy->uniqueid_format_string, curproxy, &curproxy->format_unique_id, 0, - (proxy->cap & PR_CAP_FE) ? SMP_VAL_FE_HRQ_HDR : SMP_VAL_BE_HRQ_HDR); - free(curproxy->uniqueid_format_string); - curproxy->uniqueid_format_string = NULL; - } + free(curproxy->conf.uif_file); + curproxy->conf.uif_file = strdup(curproxy->conf.args.file); + curproxy->conf.uif_line = curproxy->conf.args.line; } else if (strcmp(args[0], "unique-id-header") == 0) { @@ -4867,11 +4879,15 @@ int cfg_parse_listen(const char *file, int linenum, char **args, int kwm) goto out; } - if (curproxy->logformat_string != default_http_log_format && - curproxy->logformat_string != default_tcp_log_format && - curproxy->logformat_string != clf_http_log_format) - free(curproxy->logformat_string); - curproxy->logformat_string = strdup(args[1]); + if (curproxy->conf.logformat_string != default_http_log_format && + curproxy->conf.logformat_string != default_tcp_log_format && + curproxy->conf.logformat_string != clf_http_log_format) + free(curproxy->conf.logformat_string); + curproxy->conf.logformat_string = strdup(args[1]); + + free(curproxy->conf.lfs_file); + curproxy->conf.lfs_file = strdup(curproxy->conf.args.file); + curproxy->conf.lfs_line = curproxy->conf.args.line; /* get a chance to improve log-format error reporting by * reporting the correct line-number when possible. @@ -4881,14 +4897,6 @@ int cfg_parse_listen(const char *file, int linenum, char **args, int kwm) file, linenum, curproxy->id); err_code |= ERR_WARN; } - else if (curproxy->cap & PR_CAP_FE) { - curproxy->conf.args.ctx = ARGC_LOG; - if (curproxy->logformat_string) - parse_logformat_string(curproxy->logformat_string, curproxy, &curproxy->logformat, LOG_OPT_MANDATORY, - SMP_VAL_FE_LOG_END); - free(curproxy->logformat_string); - curproxy->logformat_string = NULL; - } } else if (!strcmp(args[0], "log") && kwm == KWM_NO) { @@ -6471,22 +6479,35 @@ int check_config_validity() /* compile the log format */ if (!(curproxy->cap & PR_CAP_FE)) { - if (curproxy->logformat_string != default_http_log_format && - curproxy->logformat_string != default_tcp_log_format && - curproxy->logformat_string != clf_http_log_format) - free(curproxy->logformat_string); - curproxy->logformat_string = NULL; + if (curproxy->conf.logformat_string != default_http_log_format && + curproxy->conf.logformat_string != default_tcp_log_format && + curproxy->conf.logformat_string != clf_http_log_format) + free(curproxy->conf.logformat_string); + curproxy->conf.logformat_string = NULL; + free(curproxy->conf.lfs_file); + curproxy->conf.lfs_file = NULL; + curproxy->conf.lfs_line = 0; } - curproxy->conf.args.ctx = ARGC_LOG; - if (curproxy->logformat_string) - parse_logformat_string(curproxy->logformat_string, curproxy, &curproxy->logformat, LOG_OPT_MANDATORY, + if (curproxy->conf.logformat_string) { + curproxy->conf.args.ctx = ARGC_LOG; + curproxy->conf.args.file = curproxy->conf.lfs_file; + curproxy->conf.args.line = curproxy->conf.lfs_line; + parse_logformat_string(curproxy->conf.logformat_string, curproxy, &curproxy->logformat, LOG_OPT_MANDATORY, SMP_VAL_FE_LOG_END); + curproxy->conf.args.file = NULL; + curproxy->conf.args.line = 0; + } - curproxy->conf.args.ctx = ARGC_UIF; - if (curproxy->uniqueid_format_string) - parse_logformat_string(curproxy->uniqueid_format_string, curproxy, &curproxy->format_unique_id, 0, + if (curproxy->conf.uniqueid_format_string) { + curproxy->conf.args.ctx = ARGC_UIF; + curproxy->conf.args.file = curproxy->conf.uif_file; + curproxy->conf.args.line = curproxy->conf.uif_line; + parse_logformat_string(curproxy->conf.uniqueid_format_string, curproxy, &curproxy->format_unique_id, 0, (proxy->cap & PR_CAP_FE) ? SMP_VAL_FE_HRQ_HDR : SMP_VAL_BE_HRQ_HDR); + curproxy->conf.args.file = NULL; + curproxy->conf.args.line = 0; + } /* only now we can check if some args remain unresolved */ cfgerr += smp_resolve_args(curproxy); diff --git a/src/haproxy.c b/src/haproxy.c index 15c14e302..d4c118974 100644 --- a/src/haproxy.c +++ b/src/haproxy.c @@ -932,12 +932,14 @@ void deinit(void) free(p->capture_name); free(p->monitor_uri); free(p->rdp_cookie_name); - if (p->logformat_string != default_http_log_format && - p->logformat_string != default_tcp_log_format && - p->logformat_string != clf_http_log_format) - free(p->logformat_string); + if (p->conf.logformat_string != default_http_log_format && + p->conf.logformat_string != default_tcp_log_format && + p->conf.logformat_string != clf_http_log_format) + free(p->conf.logformat_string); - free(p->uniqueid_format_string); + free(p->conf.lfs_file); + free(p->conf.uniqueid_format_string); + free(p->conf.uif_file); for (i = 0; i < HTTP_ERR_SIZE; i++) chunk_destroy(&p->errmsg[i]); diff --git a/src/proxy.c b/src/proxy.c index 7bfe954a4..f909d201d 100644 --- a/src/proxy.c +++ b/src/proxy.c @@ -409,10 +409,12 @@ int proxy_cfg_ensure_no_http(struct proxy *curproxy) Warning("config : 'option httplog' not usable with %s '%s' (needs 'mode http'). Falling back to 'option tcplog'.\n", proxy_type_str(curproxy), curproxy->id); } - if (curproxy->logformat_string == default_http_log_format || - curproxy->logformat_string == clf_http_log_format) { - curproxy->logformat_string = default_tcp_log_format; - Warning("config : 'option httplog' not usable with %s '%s' (needs 'mode http'). Falling back to 'option tcplog'.\n", + if (curproxy->conf.logformat_string == default_http_log_format || + curproxy->conf.logformat_string == clf_http_log_format) { + /* Note: we don't change the directive's file:line number */ + curproxy->conf.logformat_string = default_tcp_log_format; + Warning("parsing [%s:%d] : 'option httplog' not usable with %s '%s' (needs 'mode http'). Falling back to 'option tcplog'.\n", + curproxy->conf.lfs_file, curproxy->conf.lfs_line, proxy_type_str(curproxy), curproxy->id); }