[squid-dev] [PATCH] squid-3.5: fix for errors when receiving a non-existent file via ftp

[Vitaly Lavrov]

On 09.10.2015 04:45, Amos Jeffries wrote:
> On 9/10/2015 3:15 a.m., Vitaly Lavrov wrote:
>> On 07.10.2015 22:47, Alex Rousskov wrote:
>>
>> Hello Alex.
>>
>> Thanks for the quick response.
>>> Hello Vitaly,
>>>
>>>       I am glad you were able to fix the problem, and I thank you for
>>> sharing your fix.
>>>
>>> Unfortunately, the patch itself needs significant refactoring because
>>> you are [incorrectly] duplicating an already duplicated code. As you
>>> probably know, most of the code you added already exists in
>>> Ftp::Gateway::loginFailed() and, more importantly, in
>>> Ftp::Client::failed(). We do not need a third imperfect copy of that
>>> logic!
>> Yes, agree!
>>> 0. Undo your changes (the code may be reused at step #3).
>>>
>>> 1. Add an optional ErrorState *err parameter to ftpFail() with a default
>>> NULL value. When the parameter is not nil, ftpFail() should use it
>>> instead of creating its own ErrorState.
>>>
>>> 2. Change Ftp::Gateway::loginFailed() to always call ftpFail() after
>>> trying to create an ErrorState. Move the bottom of
>>> Ftp::Gateway::loginFailed() (i.e., the code that handles non-nil err) to
>>> ftpFail(). Test the refactored code for regressions. This change will
>>> not fix the bug you are after.
>>>
>>> 3. Compare the resulting ftpFail() with the code from your patch. Add
>>> any missing actions. Test that the resulting code works fine, including
>>> handling the bug you are after.
>> I added a field "ErrorState *ftperr" in class Ftp::Client. If it is not
>> NULL,
>> the Ftp::Client::failed() uses it.
>
> This is not what Alex meant by parameter.
>
> Function parameter is passed by the caller directly and only exists
> within the specific function call chain where it was passed. It cannot
> remain in memory interferring with other state or operations, does not
> need any extra constructor/destructor infrastructure code to manage its
> existence, and the value of the pointer being NULL/non-NULL can be used
> as the boolean of whether to use it or not.
>
> A member like ftperr may be setup by some earlier FTP transaction an
> unknown number of steps and time prior to the current error happening.
> Then prevents any following errors from using their own ErrorState
> object with details about the second error.
Here's a version without field "ftperr" and with an additional parameter
in the Ftp::CLient::failed().


-------------- next part --------------
A non-text attachment was scrubbed...
Name: squid-3.5-ftp_error_handle3.diff
Type: text/x-patch
Size: 4466 bytes
Desc: not available
URL: <http://lists.squid-cache.org/pipermail/squid-dev/attachments/20151009/6b910134/attachment.bin>