[squid-dev] [PATCH] Fetch missing certificates

[Christos Tsantilas]

On 07/15/2016 12:59 AM, Alex Rousskov wrote:
> 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.

In the t4 patch I made Downloader::CbDialer a CallDialer kid.

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

Yep.
I hope it is ok now.

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

I planned to remove this if, but finally I forgot it.
I will make it locally, I will ppply to trunk with this.

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