[squid-dev] [PATCH] Fetch missing certificates

[Christos Tsantilas]

On 07/20/2016 04:42 PM, Amos Jeffries wrote:
> 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.

It is a fix unrelated to this patch.

Also the current CertsIndexedList type uses raw X509 pointer. To convert 
it to LockingPointer requires some work to test if all are ok after this 
change (eg for leaks or unexpected object de-structures).

This patch is enough complex alone. Better to finish with the one 
headache before involved with a new one.

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

Thank you. I will try to add short references to the related RFC.

>
>
> Thanks
>
> Amos