[squid-dev] [PATCH] Non-HTTP bypass
chtsanti at users.sourceforge.net
Wed Oct 22 14:15:49 UTC 2014
On 10/22/2014 03:44 PM, Amos Jeffries wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> On 22/10/2014 9:12 p.m., Tsantilas Christos wrote:
>> On 10/21/2014 04:29 PM, Amos Jeffries wrote:
>>> 2) All changes in src/tunnel.cc seem to be needless.
>> Some changes are required!
>>> - tunnelStartShovelling() should *always* be the entrypoint to
>>> begin transmit on a tunnel in any direction. At that point there
>>> is maybe client data to send to server ...
>>> - The socket may not even permit one whole TCP_RCV_BUF worth of
>>> bytes to be written in a single action. comm_write can handle
>>> that just fine. If comm_write is unable to handle all the
>>> available conn->in.buf in one comm_write() call then that would
>>> be a bug in comm to fix separate from all this.
>> This is true. However we are using two buffers to read/write
>> to/from server/client. The TunnelState::client::buf and
>> TunnelState::server::buf which are of size SQUID_TCP_SO_RCVBUF
>> At early stages of this patch I tried to convert these buffers to
>> SBufs , but required many changes in the tunnel.cc code, so I
>> revert the changes and finally implemented the
>> TunnelStateData::copyClientBytes member.
> Arg, sorry you are right. I forgot that my previous conversion to SBuf
> did not make it through Alex audit requirements.
> Is this an urgent need? or can we work on fixing that problem first to
> reduce the changes this feature requires?
I do not know if it worths the required work.
My understanding is that this is requires changes in comm/* code too...
>>> There is simply no reason to add an extra if
>>> (preReadClientData.length()) conditional in *every* packet I/O
>> As you can see the old code copies the ConnStateData::in->buf data
>> to TunnelStateData->client.buf buffer. This is because in the case
>> you are read a CONNECT request you can be sure that the extra bytes
>> you read are not hugger than the SQUID_TCP_SO_RCVBUF.
>> But for this patch imagine the case where: - Squid configured
>> with: request_header_max_size 70kbs - Client sends something which
>> looks like an HTTP request eg: AMETHOD [spaces]* string [spaces]*
>> ... Other data Also assume that this is more than the size of 70k.
>> - Squid will read 70k before decide that this is not an HTTP
>> request. - The 70k are not fit to TunnelStateData->client.buf
>> An alternate solution is to add some code to build/fix
>> TunnelStateData->client.buf to have the required size. I need to
>> recheck, but looks a good solution. Is it ok?
> Okay. I find it acceptible as a workaround for the buffer limitation.
For now as a temporary solution I will check if ConnStateData::in->buf
is bigger than SQUID_TCP_SO_RCVBUF and if yes then do a realloc on
TunnelStateData->client.buf before copy.
It is a rare case so I believe it is not a huge problem...
> I would rather see the limit fixed though. Alex and I have now almost
> reached agreement on the Comm::Write API needed to SBuf the tunnel I/O
> fully. If this bypass feature can wait for a while I will divert my
> efforts to getting that completed now.
If you are near to finish it, then I suppose we can wait. But the
related tunnel.cc code is small, so I believe we are not conflicting.
More information about the squid-dev