[squid-dev] [PATCH] Fast SNI peek

[Amos Jeffries]

On 17/05/2016 6:52 a.m., Alex Rousskov wrote:
> 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?
> 

I had build errors when I tried to use it ~6-12 months ago. IIRC that
was GCC 4.8.2.
If its working now thats great news. Maybe we should just let this one
in and see what happens.

> 
>> * 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.
> 

Okay. It was just a polish request and sounds like not worth 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.
> 

To answer Christos' question;
 For iostreams to print the pointer value in hex like seems to be wanted
here one has to make sure the pointer is a void* and not a type that can
be dereferenced. Otherwise the streams will try to display its contents
which would either break, or "work" with unexpected values output.
 NP: 'this' pointer always has a known type, not void*.


> 
>> * 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.
> 

What sparked me to mention it was a ProtocolVersion called 'v' and
TlsDetails called 'details'.
Both prime examples of not being needed since the parameter name added
nothing to the understandig of what they are.

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

Nod. Fine with me.

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

So one browsing the file does not pay attention to the "ServerBio::" or
"Ssl::Bio::" scope paths in every single method name?
 Past experience shows that new code just gets added in where needed and
can make them become just another bogus comment. IIRC last time we
discussed this it was me trying to add this type of marker, and Alex
arguing me out of it.

NP: It also shows how that .cc is violating the coding guideline of one
class per file. If the classes are big enough to need some separation
between them it means they probably dont meet the spirit of the
exception on that guideline.

Anyhow, now the GCC 4.8 thing seems to be out of the way...

Christos:
 +1, please apply with whatever of the polishing suggestions you accept.

Amos