[squid-dev] [PATCH] Fetch missing certificates

[Alex Rousskov]

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

Using "int" is the correct default because it does not force the reader
to pause and think "Why 8 bits?", 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.


Thank you,

Alex.