[squid-dev] [PATCH] Fast SNI peek

[Christos Tsantilas]

On 05/15/2016 04:49 PM, Amos Jeffries wrote:
>   (I will hold off on my PeerConnector related changes until this is in).

thank you.


>
> in src/client_side.h:
>
> * the cpmment block starting with:
>    "// The following block does not needed,"
>
>   can all be replaced with one line:
>    // Setting bio->rBufData() is not needed. inBuf and rbuf have the same
> content.
  Your suggestion is not correct here.
  The commented coded sets the ConnStateData::inBuf, not the bio->rBufData.

>
>    - I am a little doubtful about the accuracy of that. Both places are
> using SBuf to hold the buffer. If they are not kept in sync they might
> end up pointing at different MemBlob offsets or different MemBlob
> entirely. Did I might have missed the sync'ing ?

My old commend was wrong too.
Actually the ConnStateData reads client data (client TLS hello message) 
and store it to inBuf.
After this patch we do not actually care if the 
Ssl::ClientBio::rBufData(), has or not any data, and the nature of these 
data.

The tunnelStartShoveling, if there are any data in ConnStateData::inBuf 
takes care about.

I just removed the commented code and the comment.


>
>
> in src/client_side.cc:
>
> * files in src/security/ should not need wrapping in USE_OPENSSL
>   - referring to the #includes pulling in security/Handshake.h
>   - maybe others
>
> * tlsParser has no doxygen comment
>
>
> in src/parser/BinaryTokenizer.cc:
>
> * #include "parser/BinaryTokenizer.h" instead.
>
> * delegating constructors is a C++11 feature that will definitely fail
> on GCC 4.8.
>   - I have put the question to Alex in how recent patch about dropping
> that support. If we go ahead with that, the BinaryTokenizer construction
> is fine. Otherwise it will have to be expanded to non-delegating.

No opinion on this. I will wait Alex and you to decide about.
The only I can say is that it looks ok in my g++-4.8.4


>
> * please make BinaryTokenizer_tail a function not a macro.
>   - doing that will also fix the missing (void*) before use of 'this'
The (void *) is not missing here, it is not desired.
What am I missing?

The macro is used in two places and it is faster as macro I think.
But not problem I will make it a method.

>
> * please move the BinaryTokenizerContext inline method definitions
> inside the class {} block.
>   - then you wont need to even use "inline".

This is not possible. All of these requires BinaryTokenizer
definition.

>
> in src/ssl/PeekingPeerConnector.cc:
>
> * the debugs dropped from Ssl::PeekingPeerConnector::noteWantWrite() was
> about the srvBio->holdWrite() in the line above which still remains
> wasn't it?

Does not actually provides a useful info.
I removed this change from this patch, but we should at least increase 
the level of this debug message, it is not an important info.


>
>
> Amos
>