[squid-dev] [PATCH] Fetch missing certificates

[Amos Jeffries]

On 16/07/2016 2:08 a.m., Christos Tsantilas wrote:
> A new patch.
> 
> It also includes the following fixes:
>   - Sets the log_uri for ClientHttpRequest build by Downloader
>   - Removes two XXX comments from PeerConnector class, which are not
> valid any more
>   - Make the Downloader::CbDialer a CallDialer kid.
> 
> 
> Please also see my comments bellow.
> 

> On 07/14/2016 02:16 PM, Amos Jeffries wrote:
>>
>>   - our coding style places documentation of class 'private' methods in
>> the .cc file not the .h.
> 
> ok, I fixed them, but we should change this style.
> It is easier to have open the .h file and read the documentation for a
> method you are going to use, than searching in the cc file to find the
> documentation.
> 

The reasoning behind the guideline was that these are *private* methods
of the class.

The only time(s) one should be looking at (or bothered by) their
documentation is when one has the .cc open already. eg, when altering
the class, or when the public API methods are so badly documented, one
has resorted to reading their code.

So we can reduce already large .h files a bit by having the
documentation internal to the .cc for these methods. Away from being a
distraction to anyone just needing to *use* the class.

>>
>>   - why is DER format being used here? I think I know, but it needs to be
>> stated for clarity, Squid normally uses PEM format.
> 
> The DER format is used by RFCs for this and similar cases.
> 

Sorry if I wasn't clear. I meant that needed documenting in the code.

"Assume that it is in DER format." is not enough to inform anyone in
future what RFCs etc. are saying that makes this assumption reasonable.


>>
>> in src/ssl/support.h:
>>
>> * can we use Security::CertPointer for the second parameter of
>> CertsIndexedList ?
> 
> I am not seeing any reason.
> In any case we can do it with an other patch if required.
> 

Reason being to avoid another patch just for that. Your choice though.

FYI: Alex and I have agreed to steer away from raw-pointer usage now
when we can. And you also mentioned having trouble with the distinction
between Pointer and Ptr in the SSL/TLS code the other day. So I'm
looking to start proactively dropping the Ptr whenever possible now, and
just stick with Pointer.


+1 from me. Apart from the DER comment improvement I think this is ready
to go in unless Alex spots anything.


Thanks

Amos