[squid-dev] [PATCH] Fetch missing certificates

[Amos Jeffries]

On 15/07/2016 3:07 a.m., Alex Rousskov wrote:
> On 07/14/2016 05:16 AM, Amos Jeffries wrote:
>> * the debugs lines indicating constructor and destructor have been run
>> need to be symmetrical and name the object type.
>>
>>  - They are there for the find-alive.pl script, so need to match the
>> pattern it is searching for:
>>  eg. debugs(33, 6, "Downloader constructed, this=" << (void*)this);
>>  eg. debugs(33, 6, "Downloader destructed, this=" << (void*)this);
>>
>>  - same for other new objects contrustors and destructors. IF you have
>> debugs() namign them.
> 
> However, if a parent class already provides those, then I recommend not
> adding more constructed/destructed lines in kids as they will not help
> find-alive.pl. Not a big deal either way though.
> 
> 
>>   Since we are considering Downloader a type of high level protocol
>> actor its conceivable that we will want to use 11,2 for displaying its
>> HTTP-ish messages before the rest of Squid logic gets to mangle them.
> 
> I agree that showing Downloader requests at ALL,2 is a good idea,
> especially if it is easy to report [until the routing layer supplies
> that functionality for all routing clients]. Needless to say, that
> reporting should be done when the request is formed, not in the class
> destructor.
> 
> 
>> * Downloader::downloadFinished method
> 
>>  - that comment brings up the question of whether that Must() should be
>> confirming doneAll() instead of done() ?
> 
> Must(done()) is correct. doneAll() becomes true when a _positive_
> termination goal has been reached so when we want to assert that the job
> is done (for any reason) we must use the more comprehensive done().
> 
> Please remove the following "Squid will delete..." comment or replace it
> with a more accurate one:
> 
>     Must(done()); // we will be deleted upon exiting job's AsyncCall
> 
> We should probably add an AsyncJob method that encapsulates the above
> check _and_ verifies that the job is currently handling an async call,
> but that improvement is outside your project scope, of course.
> 
> 
>>  - the symbol name 'callback' has a clash between the private method,
>> and the private member Pointer. This is a case where the '_' suffix is
>> required to distinguish them.
> 
> AFAICT, that private method is spelled callBack (appropriately making
> the method name a verb rather than a "callback" noun used for the data
> member) so there is no clash. I am not against adding the '_' suffix though.
> 
> 
>> in src/ssl/PeerConnector.h:
>>
>> * since certsDownloads is apparently constrained to values up to
>> MaxCertsDownloads. Can we please use a small integer type like uint8_t
>> instead of a full 'int' ?
> 
> Please do not use uint8_t. I believe this has been discussed already:
> Size-specific types should only be used when there is a specific correct
> size (e.g., we must replicate a system structure) and/or we must
> minimize memory footprint (e.g., lots of structures containing this
> member are expected, especially inside a large container).

Or when we need some certainty about what the size of the data field
actually is.

Side track: For sizes of payload objects we should be centering on
uint64_t to handle the large objects instead of size_t or int which
can't handle them.

This situation is ambiguous for Downloader though since its
HTTP_REQBUF_SZ can only able to handle 4 KB or smaller objects.

I would like that increased to 64KB though. Since we have already had
sightings of quite large certificates coming out of Google with lots of
alternative-domain entries in the cert.


Anyhow back to MaxCertsDownloads...

> 
> Using "int" is the correct default because it does not force the reader
> to pause and think "Why 8 bits?",

Not having a fixed size forces the reader to stop and think "what can
int be?" then investgate all the code to find out what parts are using
int to mean something other than 32-bit signed values.

Note that Squid is used on some embeded systems where int != 32-bit.


> is usually faster, will be printed as
> a number on std::ostream (rather than a character), and has fewer
> chances to accidentally overflow when somebody decides to increase
> MaxCertsDownloads (e.g., by making it configurable) without updating
> certsDownloads.

The person doing that needs to know the value ranges they are changing
and what side effects the change will have. Using "int" just obscures
the issues.

Amos