[squid-dev] [PATCH] Fast SNI peek

[Alex Rousskov]

On 05/15/2016 07:49 AM, Amos Jeffries wrote:

> in src/parser/BinaryTokenizer.cc:
> 
> 
> * delegating constructors is a C++11 feature that will definitely fail
> on GCC 4.8.


When I wrote and tested this part of the code, I was using GCC 4.8. GCC
documentation matches my experience:
https://gcc.gnu.org/gcc-4.8/cxx0x_status.html

Why do you think that GCC 4.8 has trouble with delegating constructors?


> * please make BinaryTokenizer_tail a function not a macro.

It cannot be a function (with reasonable performance/style) because it
does not have access to the debugging stream and would have to
accumulate output in a temporary SBuf.

To avoid this macro, we would have add a new class with a 4-argument
constructor (or a _friend_ class with a 3-argument constructor). I
really doubt the increased complexity would be worth avoiding macro in
this particular case, but please let me know if you insist on this
change, and we will make it.


>  - doing that will also fix the missing (void*) before use of 'this'

I agree that we should cast, but that improvement is not related to
BinaryTokenizer_tail() being a macro.


> * please add to the BinaryTokenizer intN() methods that they assume
> network-endian input.

I recommend documenting that in the BinaryTokenizer class description
instead of every intN() method because there are other existing methods
that assume big-endian/network byte order and no existing method assumes
another byte order. When/if we add methods that support alternative byte
ordering, we will decide whether to specify the byte order via the
method name, method parameter, or tokenizer object configuration.


> in src/parser/BinaryTokenizer.h:
> 
> * please move the BinaryTokenizerContext inline method definitions
> inside the class {} block.
>  - then you wont need to even use "inline".

Besides being impossible in this case, as explained by Christos, I do
not think moving multiline methods into the class declaration is better
in general. IMHO, the class API is easier to read when multiline methods
are kept away from the class declaration.


> in src/security/Handshake.cc:
> 
> * Security::HandshakeParser::skipMessage
>  - the comment seems to be mentioning some serious breakage of the
> debugging output. That should probably get an XXX marker.

Or we can resolve that problem by appending " [fragment]" to
skipMessage() calls:

  skipMessage("unknown ContentType msg [fragment]");
  skipMessage("ChangeCipherCpec msg [fragment]");
  skipMessage("app data [fragment]");


>  NP: just a note on class method definitions. Try to avoid naming the
> parameters if you can.

... but only when you are absolutely sure that the parameter type
clearly describes the parameter, which is not the case in many (most?)
cases. If we have to pick between Doxygen and STUBS convenience and code
clarity, we should pick code clarity.

Given the uncertainty regarding what is "clear" to whom, avoiding
parameter names would be a bad default approach/recommendation IMHO.


> 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?


IIRC, that debug produced one debugging line for each SSL transaction in
some tests. @Christos, if you restore this debugging, please change the
debugging level to at least 2.


> in src/ssl/PeerConnector.cc:
> 
> * "Log connection details, if any"
>  - in the function above Ssl::PeerConnector::noteWantRead()
> 
> * change in Ssl::PeerConnector::noteWantRead() is not necessary.
>  - we leave local variable definitions until the point they are needed.
>  - which for that 'fd' is still where it was before.
> 
> 
> in src/ssl/bio.cc:
> 
> * you dont need to put /* Ssl:Bio */ or /* ServerBio */ markers between
> definitions at the file global level.

Somebody added draconian "no double empty lines" rules to source
formatting scripts and those markers are the only thing that clearly
separates different class methods in .cc file now. FWIW, they _are_
useful to some of us browsing the code.


Thank you,

Alex.