Re: [PATCH] Minor : converter: Add len converter

[Willy Tarreau]

On Fri, Dec 15, 2017 at 06:23:21AM +0100, Etienne Carrière wrote:
> I agree that my first intent "strlen" is not coherent with the other
> converter name. "length" is better in the logic of the name of other
> converters.

OK now renamed and pushed. Thanks!
Willy

Re: [PATCH] Minor : converter: Add len converter

[Etienne Carrière]

Hi,

2017-12-14 22:49 GMT+01:00 Willy Tarreau :
>
> On Thu, Dec 14, 2017 at 10:18:27PM +0100, Cyril Bonté wrote:
> > Le 14/12/2017 à 19:24, Willy Tarreau a écrit :
>
> Or maybe "length" then ? After all it could even work on binary samples if
> needed, thus avoiding "str" could avoid some confusion. Etienne, feel free
> to suggest as well, you're the first user :-)


I agree that my first intent "strlen" is not coherent with the other
converter name. "length"
is better in the logic of the name of other converters.

-- 
Etienne

Re: [PATCH] Minor : converter: Add len converter

[Willy Tarreau]

On Thu, Dec 14, 2017 at 10:18:27PM +0100, Cyril Bonté wrote:
> Le 14/12/2017 à 19:24, Willy Tarreau a écrit :
> > So I think we have two action plans for now :
> >1) find another (ideally short) name for "len" which doesn't match "len"
> >   nor "id" nor "table" nor "if" nor "unless". Etienne initially proposed
> >   "strlen" but I thought that it's a bit long for such a common use 
> > case.
> >   Ideas welcome.
> 
> Unfortunately, for now we have to rename it.

Renaming is a detail, it never was in any stable version.

> But I'm fine with "strlen",
> which follows the description in the documentation.

Or maybe "length" then ? After all it could even work on binary samples if
needed, thus avoiding "str" could avoid some confusion. Etienne, feel free
to suggest as well, you're the first user :-)

Willy

Re: [PATCH] Minor : converter: Add len converter

[Cyril Bonté]

Le 14/12/2017 à 19:10, Willy Tarreau a écrit :

If you want to make a patch for this one, in parallel I'm checking
sample_parse_expr().


Yes, I was ready to provide the patch but I failed to reproduce the 
issue today. After some hours, I realized that there was a recent (well, 
10 days ago) patch from Christopher that seems to prevent the condition 
to happen :

http://git.haproxy.org/?p=haproxy.git;a=commit;h=fd608dd2d212928e585da645d7d0c7089424a0de

My patch is less useful now but I think it should be applied too to be 
complete (this will also align the request code with the response one).


--
Cyril Bonté

Re: [PATCH] Minor : converter: Add len converter

[Cyril Bonté]

Le 14/12/2017 à 19:24, Willy Tarreau a écrit :

So I think we have two action plans for now :
   1) find another (ideally short) name for "len" which doesn't match "len"
  nor "id" nor "table" nor "if" nor "unless". Etienne initially proposed
  "strlen" but I thought that it's a bit long for such a common use case.
  Ideas welcome.


Unfortunately, for now we have to rename it. But I'm fine with "strlen", 
which follows the description in the documentation.



   2) kill this outdated way of unreliably parsing expressions among multiple
  words for 1.9. It even prevents us from improving the sample expressions
  language (ie: I'd love to have a stack-based language but we can do
  nothing if we can't even add reliable operators).


Yes, it was difficult to understand the goal of each part of this 
function and I fear that any minimal modification inside it can 
introduce side effects on one feature or another ;-/


--
Cyril Bonté

Re: [PATCH] Minor : converter: Add len converter

[Willy Tarreau]

Hi Cyril,

On Thu, Dec 14, 2017 at 06:48:28PM +0100, Cyril Bonté wrote:
> It's embarrassing, I was working on a patch for a nasty bug I've seen
> several times last week (randomly haproxy won't start), but now I've updated
> to the current master branch, it's even worse with this specific patch.
> 
> Example :
>   listen capture
> bind :9000
> mode http
> http-request capture hdr(X-Forwarded-Proto) len 8
> 
> With the new "len" converter, it fails with :
>   parsing [capture.cfg:4] : error detected in proxy 'capture' while parsing
> 'http-request capture' rule : expects 'len' or 'id', found '8'.
> 
> It seems that we have to fix sample_parse_expr(). Currently, be careful if
> you want to backport it.

Wow that's very strange. I suspect we indeed have a bug somewhere in the
sample parser, very likely it reads past the last delimiter and goes on
with the word it has found that happens to match a registered converter.

And indeed, if I go further I get the same error by giving this :

http-request capture hdr(X-Forwarded-Proto) upper lower base64 sha1 len 8

But placing "foo" in the middle immediately causes an error. That's scary!

> Otherwise, my original bug concerns the exact same configuration and looks
> to exist for a long time : there is an unwanted "check_ptr" assignment in
> parse_http_req_capture() when the "len" parameter is set instead of "id".
> 
> diff --git a/src/proto_http.c b/src/proto_http.c
> index 1330ac864..481688fdf 100644
> --- a/src/proto_http.c
> +++ b/src/proto_http.c
> @@ -12149,7 +12149,6 @@ enum act_parse_ret parse_http_req_capture(const char
> **args, int *orig_arg, stru
> 
>   rule->action   = ACT_CUSTOM;
>   rule->action_ptr   = http_action_req_capture;
> - rule->check_ptr= check_http_req_capture;
>   rule->arg.cap.expr = expr;
>   rule->arg.cap.hdr  = hdr;
>   }

Well, at first glance I didn't agree but in fact you're totally right,
the "len" and "id" are mutually exclusive and the one above if only for
the id.

If you want to make a patch for this one, in parallel I'm checking
sample_parse_expr().

Thanks!
Willy

Re: [PATCH] Minor : converter: Add len converter

[Cyril Bonté]

Hi Willy,

Le 14/12/2017 à 14:36, Willy Tarreau a écrit :

On Wed, Dec 13, 2017 at 02:56:34PM +0100, Etienne Carrière wrote:

Hi,

You will find attached a patch to add a converter that can returns the
len of a string.


Thank you Etienne, now applied.


It's embarrassing, I was working on a patch for a nasty bug I've seen 
several times last week (randomly haproxy won't start), but now I've 
updated to the current master branch, it's even worse with this specific 
patch.


Example :
  listen capture
bind :9000
mode http
http-request capture hdr(X-Forwarded-Proto) len 8

With the new "len" converter, it fails with :
  parsing [capture.cfg:4] : error detected in proxy 'capture' while 
parsing 'http-request capture' rule : expects 'len' or 'id', found '8'.


It seems that we have to fix sample_parse_expr(). Currently, be careful 
if you want to backport it.


Otherwise, my original bug concerns the exact same configuration and 
looks to exist for a long time : there is an unwanted "check_ptr" 
assignment in parse_http_req_capture() when the "len" parameter is set 
instead of "id".


diff --git a/src/proto_http.c b/src/proto_http.c
index 1330ac864..481688fdf 100644
--- a/src/proto_http.c
+++ b/src/proto_http.c
@@ -12149,7 +12149,6 @@ enum act_parse_ret parse_http_req_capture(const 
char **args, int *orig_arg, stru


rule->action   = ACT_CUSTOM;
rule->action_ptr   = http_action_req_capture;
-   rule->check_ptr= check_http_req_capture;
rule->arg.cap.expr = expr;
rule->arg.cap.hdr  = hdr;
}


--
Cyril Bonté

Re: [PATCH] Minor : converter: Add len converter

[Willy Tarreau]

On Wed, Dec 13, 2017 at 02:56:34PM +0100, Etienne Carrière wrote:
> Hi,
> 
> You will find attached a patch to add a converter that can returns the
> len of a string.

Thank you Etienne, now applied.

Willy