[squid-dev] [PATCH] Support tunneling of bumped non-HTTP traffic. Other SslBump fixes.

[Amos Jeffries]

On 29/10/2016 4:07 a.m., Alex Rousskov wrote:
> On 10/28/2016 07:54 AM, Christos Tsantilas wrote:
>> On 10/28/2016 01:11 PM, Amos Jeffries wrote:
>>> On 21/10/2016 3:55 a.m., Christos Tsantilas wrote:
>>>> Support tunneling of bumped non-HTTP traffic. Other SslBump fixes.
> 
>>> Are any of these additional fixes able to be easily broken out into
>>> separate patches? It would greatly help the auditing process to get
>>> smaller patches.
> 
>> Personally I prefer to keep this patch as one patch, it is not easy to
>> split it.
> 
> I second Christos' opinion on this particular aspect: Complex state and
> reentrant doCallouts() code make some of the effects really hard to
> track for a human. This patch is a lot more than a few localized
> semi-independent fixes, even if it appears to look like that.
> 

I was afraid it might come down to that. Darn callouts.
Oh, well, going with Christos 'nop, cant simplify'.


>>>  - please take advantage of the surrounding code being re-written to
>>> cleanup:
>>>     if (peer_paths == NULL || peer_paths->size() < 1) {
> 
>>> as:
>>>     if (!peer_paths || peer_paths->size() < 1) {
> 
> I am against mixing cleanup of otherwise untouched code with in-scope
> changes, but if you do decide to polish this, then the right form is
> 
>        if (!peer_paths || peer_paths->empty()) ...
> 

Thanks for that extra catch.

So for doing it; Well, the FwdState.cc change is 'just cleanup the
code'. So this patch is already mixed. IMO if we are going to suffer
this change anyhow it's best to end up with something that is not
obviously needing further cleanup for bits like this.

Amos