[squid-dev] [PATCH] Non-HTTP bypass

[Tsantilas Christos]

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 ...
>>
>> Yes.
>>
>>> - 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
>>> cycle.
>>
>>
>> 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.


>
> Amos
>