On Mon, Dec 20, 2010 at 2:40 PM, Willy Tarreau <[email protected]> wrote:
> 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.
I don't think I understand the chunk (as passed in data->str) lifecycle:
- I noticed that the chunk passed in data->str (inside
pattern_fetch_url_param function) is always one of two pointers, and I
don't know how there were allocated (is it ok to change data->str.str
value or do I create a memory leak) ?
- At first, I tried to simply point data->str.str to the parameter
value + updating data->str.len:
/* url_param_value points to the beginning of the url
parameter value, url_param_value_l is the length of the value */
data->str.str = url_param_value;
data->str.len = url_param_value_l;
But that did not work. The case where it broke is as follows:
curl http://localhost:9999/foo?sess=12345 # all subsequent calls with
sess=12345 are put to the same backend
curl http://localhost:9999/foo?sess=123456 # all subsequent calls with
sess=123456 are put to the same backend
curl http://localhost:9999/foo?sess=12345 # not put to the same
backend anymore...
I am not sure I understand why allocating a new string works, though.
>> 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 ;-)
understood.
> OK. Do you think you can easily get rid of the malloc/strdups or do you
> need some help on that ?
I think getting answers to my questions above would be enough to fix the patch
> And when that's OK, I'll ask you for an update to the doc :-)
sure - should it be added with the other methods for pattern
extractions (+ wiki ?), or somewhere else as well ?
cheers,
David
