Re: [PATCH] Increase helper-to-Squid buffer size and warn on overflows

On 28.02.2012 09:02, Henrik Nordström wrote:
> mån 2012-02-27 klockan 11:41 -0700 skrev Alex Rousskov:
> Trivial patch attached using the same old raw buffer interface, only
> growing the buffer as needed. Please merge with your patch, but keep
> default size at 8KB (or less), adjust debug message and level,
> comments
> and commmit the result.
>
> + if (srv->rbuf_sz - srv->roffset - 1 <= 0)
> + srv->rbuf = (char *)memReallocBuf(srv->rbuf,
> srv->rbuf_sz +
> 4096, &srv->rbuf_sz);

+1 on this.

I would say make it a MemBuf, but there are write-related design bugs
that need fixing there first.

>
>> And an increase in minimum buffer capacity to 32KB should make such
>> events rare.
>
> Agreed, but not desired. Please keep default size at 8KB or even
> shrink
> it down to 4KB. These are permanent allocations for the duration of
> the
> lifetime of the helper.

+1 on that smaller minimum. This buffer is several hundred KB of unused
permanent memory on some installations. Basic auth for example does not
even need 1KB buffers most of the time, 32KB would be a massive waste of
RAM.

4KB (one page size) seems a reasonable middle ground.

>
>> Please note that this minimal patch does not attempt to fix many
>> other
>> problems with helper code, including rampant code duplication and
>> fixed
>> message buffer size. I am not planning to work on these problems
>> right
>> now, and patches to address them are welcomed.
>
> yes this part of the code should be reworked quite a bit next time we
> need to touch it, but it works for now.

FWIW: I hope to remove large sections of the duplication when merging
the old helper protocols into one flexible protocol based closely around
the external ACL arrangements. All the parsing will collapse into a
single helper method.

Amos
Received on Mon Feb 27 2012 - 22:09:41 MST