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. For example; if you have some minor error early in the protocol that the code causes to be ignored (because its minor), then something major like login could get an irrelevant error page displayed and break the login process. > 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. >> This new patch is making things even more complicated than they started off just by using that new member. Alexs' suggestion was a specific set of steps that when followed reduces complexity. Amos _______________________________________________ squid-dev mailing list squid-dev@lists.squid-cache.org http://lists.squid-cache.org/listinfo/squid-dev