Hello Willy,
On 04/15/2017 04:43 PM, Willy Tarreau wrote:
On Sat, Apr 15, 2017 at 02:24:41PM +0200, Michal wrote:
Hi,
Maybe my email wasn't nice enough, but breaking compilation
You were the first one to experience the build breakage, it worked for
most of us, but you didn't even give the smallest hints about the error(s)
you met, making it much harder to fix the problem. Fortunately others were
much more constructive and reported it the normal way.
and simplest
config with server using "source" got me very angry. I didn't send any
reproducer, because even simple
"server name 1.1.1.1:80 source 1.2.3.4 track other"
wasn't parsing.
Indeed, in fact this is any keyword provided *after* 'source' keyword
which could not be parsed anymore. The first keyword ('track' here) was
skipped leading to this error message: 'other' unknown keyword.
I agree that I have not tested such cases.
srv_parse_source() 'source' keyword parser updates 'cur_arg' variable
(the index in the line of the current argument to be parsed). It is its
job because 'source' keyword number of variable arguments. So, in this
case we should not update 'cur_arg' variable anymore outside of
srv_parse_souce() parser.
The patch attached to this mail fixes this major bug.
Note that all such added lines 'if (kw->skip == -1)' may be removed,
but I am not sure it is a good thing.
Regards,
Fred.
>From 6febf2ca609d62dd3b7408f7488d82682853a845 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20L=C3=A9caille?= <flecai...@haproxy.com>
Date: Sun, 16 Apr 2017 17:14:14 +0200
Subject: [PATCH] BUG/MAJOR: Broken parsing for valid keywords provided after
'source' setting.
Any valid keyword could not be parsed anymore if provided after 'source' keyword.
This was due to the fact that 'source' number of arguments is variable.
So, as its parser srv_parse_source() is the only one who may know how many arguments
was provided after 'source' keyword, it updates 'cur_arg' variable (the index
in the line of the current arg to be parsed), this is a good thing.
This variable is also incremented by one (to skip the 'source' keyword).
This patch disable this behavior.
Should have come with dba9707 commit.
---
src/server.c | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/src/server.c b/src/server.c
index b5d8890..0f12dbd 100644
--- a/src/server.c
+++ b/src/server.c
@@ -1357,6 +1357,7 @@ void srv_compute_all_admin_states(struct proxy *px)
* Optional keywords are also declared with a NULL ->parse() function so that
* the config parser can report an appropriate error when a known keyword was
* not enabled.
+ * Note: -1 as ->skip value means that the number of arguments are variable.
*/
static struct srv_kw_list srv_kws = { "ALL", { }, {
{ "addr", srv_parse_addr, 1, 1 }, /* IP address to send health to or to probe from agent-check */
@@ -1380,12 +1381,7 @@ static struct srv_kw_list srv_kws = { "ALL", { }, {
{ "redir", srv_parse_redir, 1, 1 }, /* Enable redirection mode */
{ "send-proxy", srv_parse_send_proxy, 0, 1 }, /* Enforce use of PROXY V1 protocol */
{ "send-proxy-v2", srv_parse_send_proxy_v2, 0, 1 }, /* Enforce use of PROXY V2 protocol */
- /*
- * Note: the following 'skip' field value is 0.
- * Here this does not mean that "source" setting does not need any argument.
- * This means that the number of argument is variable.
- */
- { "source", srv_parse_source, 0, 1 }, /* Set the source address to be used to connect to the server */
+ { "source", srv_parse_source, -1, 1 }, /* Set the source address to be used to connect to the server */
{ "stick", srv_parse_stick, 0, 1 }, /* Enable stick-table persistence */
{ "track", srv_parse_track, 1, 1 }, /* Set the current state of the server, tracking another one */
{ NULL, NULL, 0 },
@@ -2226,7 +2222,8 @@ int parse_server(const char *file, int linenum, char **args, struct proxy *curpr
if (!kw->parse) {
Alert("parsing [%s:%d] : '%s %s' : '%s' option is not implemented in this version (check build options).\n",
file, linenum, args[0], args[1], args[cur_arg]);
- cur_arg += 1 + kw->skip ;
+ if (kw->skip != -1)
+ cur_arg += 1 + kw->skip ;
err_code |= ERR_ALERT | ERR_FATAL;
goto out;
}
@@ -2234,7 +2231,8 @@ int parse_server(const char *file, int linenum, char **args, struct proxy *curpr
if (defsrv && !kw->default_ok) {
Alert("parsing [%s:%d] : '%s %s' : '%s' option is not accepted in default-server sections.\n",
file, linenum, args[0], args[1], args[cur_arg]);
- cur_arg += 1 + kw->skip ;
+ if (kw->skip != -1)
+ cur_arg += 1 + kw->skip ;
err_code |= ERR_ALERT;
continue;
}
@@ -2246,12 +2244,14 @@ int parse_server(const char *file, int linenum, char **args, struct proxy *curpr
display_parser_err(file, linenum, args, cur_arg, &err);
if (code & ERR_FATAL) {
free(err);
- cur_arg += 1 + kw->skip;
+ if (kw->skip != -1)
+ cur_arg += 1 + kw->skip;
goto out;
}
}
free(err);
- cur_arg += 1 + kw->skip;
+ if (kw->skip != -1)
+ cur_arg += 1 + kw->skip;
continue;
}
--
2.1.4
