On 1/22/20 6:17 PM, Willy Tarreau wrote:
Strangely, while I was certain I had build-tested the original one, apparently
I failed as it didn't build for two errors, which I fixed in a subsequent patch.
I'm seeing that your patches still seem to rely on this bug (data_op < 0) which
normally cannot compile so I find this surprising.
I've using clang during the testing, and apparently -Wextra doesn't
enable it on clang (I'm using v9.0), only -pedantic helps:
clang -Wall -Wextra -Wpedantic test.c
test.c:6:12: warning: ordered comparison between pointer and zero ('char
*' and 'int') is an extension [-Wpedantic]
Sorry about that, after being away from C for a while, it's hard to
switch on some brain, instead of letting the compiler to catch stupid
bugs like this one.
I still merged the first one of these two because I think it's OK, however,
could you please add a bit of commit message to the second one ? As a rule
of thumb, the commit message should be used to "sell" me your patch and to
sell it to whoever would be hesitating in backporting it or not. Thus I think
that here the benefits and/or impacts are not obvious from this one-liner :
Here is me doing my best, selling you the good stuff, and the reason to
backport the whole feature (so we can detect the multi-filter from CLI,
and not rely on haproxy version or similar), see the attached patch.
Best regards,
Adis
--
Adis Nezirovic
Software Engineer
HAProxy Technologies - Powering your uptime!
375 Totten Pond Road, Suite 302 | Waltham, MA 02451, US
+1 (844) 222-4340 | https://www.haproxy.com
>From 8437fd4ac1a9cc5a9d88daad3a23e0d2a2eaeb4f Mon Sep 17 00:00:00 2001
From: Adis Nezirovic <[email protected]>
Date: Wed, 22 Jan 2020 16:50:27 +0100
Subject: [PATCH] MINOR: cli: Report location of errors or any extra data for
"show table"
When using multiple filters with "show table", it can be useful to
report which filter entry failed
> show table MY_TABLE data.gpc0 gt 0 data.gpc0a lt 1000
Filter entry #2: Unknown data type
> show table MY_TABLE data.gpc0 gt 0 data.gpc0 lt 1000a
Filter entry #2: Require a valid integer value to compare against
We now also catch garbage data after the filter
> show table MY_TABLE data.gpc0 gt 0 data.gpc0 lt 1000 data.gpc0 gt 1\
data.gpc0 gt 10 a
Detected extra data in filter, 16th word of input, after '10'
Even before multi-filter feature we've also silently accepted garbage
after the input, hiding potential bugs
> show table MY_TABLE data.gpc0 gt 0 data.gpc0
or
> show table MY_TABLE data.gpc0 gt 0 a
In both cases, only first filter entry would be used, silently ignoring
extra filter entry or garbage data.
Last, but not the least, it is now possible to detect multi-filter
feature from cli with something like the following:
> show table MY_TABLE data.blah
Filter entry #1: Unknown data type
---
src/stick_table.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/src/stick_table.c b/src/stick_table.c
index 1b397e59e..1e7d4f3a8 100644
--- a/src/stick_table.c
+++ b/src/stick_table.c
@@ -3601,6 +3601,7 @@ static int table_process_entry_per_key(struct appctx *appctx, char **args)
static int table_prepare_data_request(struct appctx *appctx, char **args)
{
int i;
+ char *err = NULL;
if (appctx->ctx.table.action != STK_CLI_ACT_SHOW && appctx->ctx.table.action != STK_CLI_ACT_CLR)
return cli_err(appctx, "content-based lookup is only supported with the \"show\" and \"clear\" actions\n");
@@ -3611,17 +3612,21 @@ static int table_prepare_data_request(struct appctx *appctx, char **args)
/* condition on stored data value */
appctx->ctx.table.data_type[i] = stktable_get_data_type(args[3+3*i] + 5);
if (appctx->ctx.table.data_type[i] < 0)
- return cli_err(appctx, "Unknown data type\n");
+ return cli_dynerr(appctx, memprintf(&err, "Filter entry #%i: Unknown data type\n", i + 1));
if (!((struct stktable *)appctx->ctx.table.target)->data_ofs[appctx->ctx.table.data_type[i]])
- return cli_err(appctx, "Data type not stored in this table\n");
+ return cli_dynerr(appctx, memprintf(&err, "Filter entry #%i: Data type not stored in this table\n", i + 1));
appctx->ctx.table.data_op[i] = get_std_op(args[4+3*i]);
if (appctx->ctx.table.data_op[i] < 0)
- return cli_err(appctx, "Require and operator among \"eq\", \"ne\", \"le\", \"ge\", \"lt\", \"gt\"\n");
+ return cli_dynerr(appctx, memprintf(&err, "Filter entry #%i: Require and operator among \"eq\", \"ne\", \"le\", \"ge\", \"lt\", \"gt\"\n", i + 1));
if (!*args[5+3*i] || strl2llrc(args[5+3*i], strlen(args[5+3*i]), &appctx->ctx.table.value[i]) != 0)
- return cli_err(appctx, "Require a valid integer value to compare against\n");
+ return cli_dynerr(appctx, memprintf(&err, "Filter entry #%i: Require a valid integer value to compare against\n", i + 1));
+ }
+
+ if (*args[3+3*i]) {
+ return cli_dynerr(appctx, memprintf(&err, "Detected extra data in filter, %ith word of input, after '%s'\n", 3+3*i + 1, args[2+3*i]));
}
/* OK we're done, all the fields are set */
--
2.25.0
