Hi David,

On Sat, Dec 18, 2010 at 02:23:33PM +0900, David Cournapeau wrote:
> (My apologies if this email appears twice - it seems it did not get
> through the first time)
> 
> Hi,
> 
> This patch against git master branch adds a url_param pattern
> extraction method to be used with stick tables, e.g.

This is a nice feature, thank you ! I remember someone already asked
about it.

> The patch is low quality: it keeps reallocating the output buffer,

The reallocation is indeed not acceptable. Not only it is extremly
expensive, it also introduces a risk of memory leak and of crash
due to the lack of return value. In fact you don't need to allocate
the output, you can simply make use of the returned chunk to put
a pointer to the request buffer there and store its length with it.
The string compare will be performed based on that length. Check
how the header-based patterns work for instance. Also, keep in
mind that you don't need to keep the data in memory for a long
time, you only need it until the store key is allocated.

> string search is brute force, and it should reuse haproxy existing
> code to get parameter value if possible (I did not find it).

That's not a big concern, because from time to time we find some
functions which can be factored out in the code and we do so. So if
you need some low level primitives you don't find, it's OK to add
them. However, please avoid starting your function names with
underscores ;-)

> It also
> only considers the first occurence of the parameter (e.g. with the
> config above, and a query string like
> "/foo?sess=1234&bar=fubar&sess=4321", it will only consider the value
> 1234).

That's fine, almost everything else has the same behaviour.

> I have tested it on a simple configuration with two haproxy instances,
> and under valgrind to check for memory leak/out of bounds errors.

OK. Do you think you can easily get rid of the malloc/strdups or do you
need some help on that ?

And when that's OK, I'll ask you for an update to the doc :-)

Cheers,
Willy


Reply via email to