[squid-dev] Broken trunk after r14735, r14726

[Alex Rousskov]

On 07/18/2016 05:12 AM, Christos Tsantilas wrote:

> I must say that I am worrying a lot for all of these changes.
> It is very difficult for me to follow them, and already I have
> difficulties to read and debug squid openSSL relate code.
> 
> We are using our own naming scheme for openSSL structures, eg
> "Security::SessionPtr" instead of "SSL *" or
> "Security::SessionStatePointer" instead of "SSL_SESSION *" and this is
> make it very difficult to follow Squid/openSSL code.
> 
> It is difficult to read an example openSSL code and trying convert it to
> squid, or reading a reference implementation and trying check against
> squid code. Someone is obligated to search for definitions and
> equivalents types all the time.

> I am just expressing my worries here, I do not want to impose anything
> and if there was a related discussion in squid-dev, sorry that I do not
> participate and I did not express my concerns earlier.


Thank you for courageously documenting your worries and concerns. I
doubt you have missed any critical discussions -- many TLS changes are
committed without review (at best). Unfortunately, I do not see a
practical solution to this problem because

1. There are not enough hours in the day to review everything that
should be reviewed. We do not have enough developers, but the developers
we do have produce enough code to overwhelm even fewer available reviewers.

2. What should be reviewed is itself difficult to identify because many
critical changes (like naming things and changing class hierarchies) are
often hidden behind innocent "cleanup" or "no logic changes"
descriptions that may not even be posted for review.

3. A large portion of reviews are a waste of time because the reviewer
does not understand the problem being solved, the developer does not
understand the problem being solved, and/or the developer is refusing to
fix anything important unless it crashes Squid. And when an agreement is
miraculously reached, the ridiculous amount of time it took to reach
that agreement often negates the benefits of the fixed code.

These problems are not limited to TLS, of course.


Sometimes I wonder whether suspending the whole review process would
result in less pain and more working code overall, despite the fact that
some reviews do result in serious bugs fixed before deployment. However,
suspending all reviews may just open the floodgate of conflicting
designs and crappy code that does not even reach the review stage now
(and so we do not know about it).


If you have specific ideas on how to fix this, please share them, but I
suspect the pain will continue for the foreseeable future because the
problem does not have a solution given the available tools and parameters.

Alex.