[squid-dev] [PATCH] Fetch missing certificates

[Amos Jeffries]

On 12/07/2016 7:13 a.m., Alex Rousskov wrote:
> On 07/11/2016 10:18 AM, Christos Tsantilas wrote:
> 
> 
>> +    debugs(33, 5, HERE);
> 
>> +    debugs(33 , 2, HERE);
> 
>> +    debugs(33, 6, HERE);
> 
> s/HERE/this/
> 

Agreed on removing HERE. Not sure if 'this' is better though.

There is a MYNAME macro for when debugs need to only output the
function/method name not the object details.



> 
>> +    ClientHttpRequest *const http = new ClientHttpRequest(NULL);
> 
> s/NULL/nullptr/ in new code
> 
> 
> The scope of the following few comments overlap. They should be
> considered together.
> 
>> +/// Used to hold and pass the required info and buffers to the
>> +/// clientStream callbacks
>> +class DownloaderContext: public RefCountable
> 
> Why do we need a dedicated class with complex lifetime controls [instead
> of using Downloader itself] for this? I am not saying this is wrong yet,
> just asking why it is needed...
> 
> 
>> +class DownloaderContext: public RefCountable
>> +{
>> +    CBDATA_CLASS(DownloaderContext);
> 
> The combination of cbdata and RefCountable can be deadly as both
> interfaces compete for object lifetime control without knowing about
> each other. IIRC, a few of these ticking time bombs remain in Squid code
> today, but here we are adding new code so, unless existing API require
> this, we should avoid this combination.
> 
> I assume that ClientStreams require cbdata; is RefCountable required by
> some other existing API?

There is no need for any RefCountable + CBDATA thing to exist.

If there does happen to be a case where any non-CBDATA type of Pointer
needs passing to a CBDATA callback. Use a temporary generic_cbdata
object instead.

Amos