[squid-dev] [PATCH] Fetch missing certificates

[Alex Rousskov]

On 07/13/2016 10:48 AM, Christos Tsantilas wrote:
> On 07/11/2016 10:13 PM, Alex Rousskov wrote:
>> On 07/11/2016 10:18 AM, Christos Tsantilas wrote:
>>> +        SBuf object;
>>> +        Http::StatusCode status;
>>> +    };
>>
>> If you can make Downloader::CbDialer print the status parameter and
>> perhaps, for successful downloads, the beginning of object parameter
>> (via Raw), please do so. See CallDialer::print().
> 
> I did not change anything here.
> The PeerConnectorCertDownloaderDialer which is Downloader::CbDialer and
> CallDialer kid implements the print method.

Ah, I see. CbDialer is called "dialer" but it actually is not!


> If you prefer we can make the Downloader::CbDialer a CallDialer kid and
> implement the print method.

Yes, please.

The PeerConnectorCertDownloaderDialer::print() method also prints
peerConnector_ which Downloader::CbDialer cannot print, but
peerConnector_ is not a call parameter (it is a part of the call
destination), and the AsyncCall itself will print enough information to
figure the destination out.

Someday, we will add proper callback API by separating call parameters
from call dialers and using relevant C++11 features, but that difficult
work is outside this project scope.


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

>  2) The clientStream code requires a RefCountable class.
> Because Downloader is an AsyncJob cbdata class and used as cbdata class
> I did not feel safe to use it with clientStream code.
> 
> The DownloaderContext class is build for clientStream and destroyed by
> clientStream code after finished. I think it is better to keep this class.

Understood. It looks like ClientStream APIs leaves us no choice here.
Your other changes to remove cbdata protection from DownloaderContext
address my concern here (to the extent it can be addressed given the
ClientStream API we have to use).



>>> +    Downloader* downloader;
>>
>>> +    if (!cbdataReferenceValid(context->downloader))
>>> +        return;
>>
>> The check only makes sense if context->downloader is a CbcPointer or
>> equivalent, not a raw pointer. Otherwise, to reliably test downloader
>> validity you need the downloader pointer to be valid (i.e., either it
>> always works and the check is not needed or it may crash and the check
>> is broken).
> 
> I can not see the mistake here.
> The cotnext->downloader is set as cbdataReference of cbdata.

... but its type was (and it was dereferenced as) a Downloader pointer.
This works only for simple cases where both "downloader->toCbdata()" and
"downloader" point to the same memory. With multiple inheritance, that
may not be the case. I bet your code was working fine, but innocent
changes in the Downloader-related class hierarchy like adding a
BackgroundDownloader child could trigger hard-to-find cbdata bugs in
your code. This is exactly why we added CbcPointer.

The "either it always works and check is not needed" part of my own
comment was incorrect. Sorry. I should have said, "either you are lucky
that context->downloader points to cbdata" instead. The check is always
needed, of course.

Your CbcPointer changes resolved this issue.


>>> +    bool exceedSize = (existingContent > -1 &&
>>> (size_t)existingContent > MaxObjectSize) ||
>>> +        ((object.length() + receivedData.length) > MaxObjectSize);
>>
>> ...
>>
>>> +    if (receivedData.length > 0) {
>>> +        object.append(receivedData.data, receivedData.length);
>>> +        http->out.size += receivedData.length;
>>> +        http->out.offset += receivedData.length;
>>> +    }
>>
>> The combination does not make sense as far as non-positive
>> receivedData.length is concerned:
>>
>> * If receivedData.length cannot be negative, then the if-statement is
>> not needed.
>> * If receivedData.length can be negative, then it cannot be used in
>> exceedSize calculations without protection.


> It can be zero.
> I replaced "if (recevedData.length >0)"  with a "if (receivedData.length)"

which I recommend removing as a [minor] waste of CPU cycles. The code
you are protecting should handle _rare_ zero receivedData.length values
just fine AFAICT. This is not a big deal, of course.


Personally, I do not plan to participate in another review round. I am
sure you can handle the above changes and address Amos' comments without it.


Thank you,

Alex.