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

[Vitaly Lavrov]

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. Through "ftperr" also passed httpStatus correct and error-code.
Duplicate code in loginFailed() and ftpFail() removed.

See the new version of the patch.

I can not test all variants of handling errors when using the protocol ftp.
I checked for proper operation without anonymous access, and anonymous access
to the available file, missing file, missing directory in the file path and
the file inaccessible due to permissions.

>
> More work will be needed after the above steps to remove duplication
> with Ftp::Client::failed(), but these steps would move us a lot closer
> to that final goal.
>
> HTH,
>
> Alex.
>


-------------- next part --------------
A non-text attachment was scrubbed...
Name: squid-3.5-ftp_error_handle2.diff
Type: text/x-patch
Size: 5174 bytes
Desc: not available
URL: <http://lists.squid-cache.org/pipermail/squid-dev/attachments/20151008/cf0147fe/attachment.bin>