Re: Review Request 116524: Make kio_ftp work with ftp server that don't support absolute path with SIZE command
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116524/#review52715 --- kioslave/ftp/ftp.cpp https://git.reviewboard.kde.org/r/116524/#comment37182 Given that m_currentPath might not end with a '/' (as indicated by the if statement 2 lines below), this is buggy. m_currentPath=/home/adawit/dev path=/home/adawit/development Let's assume both dirs exist. ++pos and path.mid(pos) will lead to lopment. You should make a local slash-terminated copy of m_currentPath before calling startsWith(). - David Faure On March 7, 2014, 6:48 a.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116524/ --- (Updated March 7, 2014, 6:48 a.m.) Review request for kdelibs and David Faure. Bugs: 326292 http://bugs.kde.org/show_bug.cgi?id=326292 Repository: kdelibs Description --- This patch changes Ftp::ftpSize such that it has support for servers that do not allow absolute paths with the SIZE command. That means when sending the command SIZE /somefile fails, it will try sending SIZE somefile before giving up. See bug report for details. Diffs - kioslave/ftp/ftp.cpp ddc6eaf Diff: https://git.reviewboard.kde.org/r/116524/diff/ Testing --- Installed Ftp server from bug report on an Android device and run tests. Thanks, Dawit Alemayehu
Re: Review Request 116524: Make kio_ftp work with ftp server that don't support absolute path with SIZE command
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116524/ --- (Updated March 12, 2014, 11:36 a.m.) Review request for kdelibs and David Faure. Changes --- Updated patch based on feedback. Bugs: 326292 http://bugs.kde.org/show_bug.cgi?id=326292 Repository: kdelibs Description --- This patch changes Ftp::ftpSize such that it has support for servers that do not allow absolute paths with the SIZE command. That means when sending the command SIZE /somefile fails, it will try sending SIZE somefile before giving up. See bug report for details. Diffs (updated) - kioslave/ftp/ftp.cpp ddc6eaf Diff: https://git.reviewboard.kde.org/r/116524/diff/ Testing --- Installed Ftp server from bug report on an Android device and run tests. Thanks, Dawit Alemayehu
Re: Review Request 116524: Make kio_ftp work with ftp server that don't support absolute path with SIZE command
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116524/#review52744 --- Ship it! Ship It! - David Faure On March 12, 2014, 11:36 a.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116524/ --- (Updated March 12, 2014, 11:36 a.m.) Review request for kdelibs and David Faure. Bugs: 326292 http://bugs.kde.org/show_bug.cgi?id=326292 Repository: kdelibs Description --- This patch changes Ftp::ftpSize such that it has support for servers that do not allow absolute paths with the SIZE command. That means when sending the command SIZE /somefile fails, it will try sending SIZE somefile before giving up. See bug report for details. Diffs - kioslave/ftp/ftp.cpp ddc6eaf Diff: https://git.reviewboard.kde.org/r/116524/diff/ Testing --- Installed Ftp server from bug report on an Android device and run tests. Thanks, Dawit Alemayehu
Re: Review Request 116524: Make kio_ftp work with ftp server that don't support absolute path with SIZE command
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116524/#review52861 --- This review has been submitted with commit d6453435f0d475e16b2512782b9b3e40bb603da1 by Dawit Alemayehu to branch KDE/4.12. - Commit Hook On March 12, 2014, 11:36 a.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116524/ --- (Updated March 12, 2014, 11:36 a.m.) Review request for kdelibs and David Faure. Bugs: 326292 http://bugs.kde.org/show_bug.cgi?id=326292 Repository: kdelibs Description --- This patch changes Ftp::ftpSize such that it has support for servers that do not allow absolute paths with the SIZE command. That means when sending the command SIZE /somefile fails, it will try sending SIZE somefile before giving up. See bug report for details. Diffs - kioslave/ftp/ftp.cpp ddc6eaf Diff: https://git.reviewboard.kde.org/r/116524/diff/ Testing --- Installed Ftp server from bug report on an Android device and run tests. Thanks, Dawit Alemayehu
Re: Review Request 116524: Make kio_ftp work with ftp server that don't support absolute path with SIZE command
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116524/ --- (Updated March 13, 2014, 5:05 a.m.) Status -- This change has been marked as submitted. Review request for kdelibs and David Faure. Bugs: 326292 http://bugs.kde.org/show_bug.cgi?id=326292 Repository: kdelibs Description --- This patch changes Ftp::ftpSize such that it has support for servers that do not allow absolute paths with the SIZE command. That means when sending the command SIZE /somefile fails, it will try sending SIZE somefile before giving up. See bug report for details. Diffs - kioslave/ftp/ftp.cpp ddc6eaf Diff: https://git.reviewboard.kde.org/r/116524/diff/ Testing --- Installed Ftp server from bug report on an Android device and run tests. Thanks, Dawit Alemayehu
Re: Review Request 116524: Make kio_ftp work with ftp server that don't support absolute path with SIZE command
On March 5, 2014, 7:47 a.m., David Faure wrote: kioslave/ftp/ftp.cpp, line 2275 https://git.reviewboard.kde.org/r/116524/diff/2/?file=251597#file251597line2275 This is surely wrong. If you're in /home/dfaure (as the CWD would be, by default, on non-anonymous FTP) and you're downloading a file /home/dfaure/dir1/dir2/file.txt then surely you want to call SIZE dir1/dir2/file.txt and not SIZE home/dfaure/dir1/dir2/file.txt It has to be relative to the CWD, not just skip the first slash, which only works if the CWD is /. This gives two alternatives for the fix: make this code relative to the current CWD whatever it is, or call ftpFolder() with the directory name (e.g. /home/dfaure/dir1/dir2) followed by SIZE with just the filename. The latter sounds like it might work better on android (if it doesn't support absolute paths, maybe it doesn't support ../../foo/bar.txt either?). Dawit Alemayehu wrote: I have not been able to test whether it supported ../../foo/bar.txt yet. However, making the code relative to m_currentPath seems to work just fine ; so I can avoid having to call ftpFolder and hence sending a cwd request. I am done testing this patch and it seems to work with a bunch of different ftp servers I tried so far. I tested with whatever ftp.kde.org runs, the android ftp server that was the source of this fix and both pro-ftpd and vsftpd servers on my own machines. If there are no objections, I am going to commit this after the beta2 is out ; so it can go in for the next beta release. - Dawit --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116524/#review52013 --- On March 7, 2014, 6:48 a.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116524/ --- (Updated March 7, 2014, 6:48 a.m.) Review request for kdelibs and David Faure. Bugs: 326292 http://bugs.kde.org/show_bug.cgi?id=326292 Repository: kdelibs Description --- This patch changes Ftp::ftpSize such that it has support for servers that do not allow absolute paths with the SIZE command. That means when sending the command SIZE /somefile fails, it will try sending SIZE somefile before giving up. See bug report for details. Diffs - kioslave/ftp/ftp.cpp ddc6eaf Diff: https://git.reviewboard.kde.org/r/116524/diff/ Testing --- Installed Ftp server from bug report on an Android device and run tests. Thanks, Dawit Alemayehu
Re: Review Request 116524: Make kio_ftp work with ftp server that don't support absolute path with SIZE command
On March 5, 2014, 7:47 a.m., David Faure wrote: kioslave/ftp/ftp.cpp, line 2275 https://git.reviewboard.kde.org/r/116524/diff/2/?file=251597#file251597line2275 This is surely wrong. If you're in /home/dfaure (as the CWD would be, by default, on non-anonymous FTP) and you're downloading a file /home/dfaure/dir1/dir2/file.txt then surely you want to call SIZE dir1/dir2/file.txt and not SIZE home/dfaure/dir1/dir2/file.txt It has to be relative to the CWD, not just skip the first slash, which only works if the CWD is /. This gives two alternatives for the fix: make this code relative to the current CWD whatever it is, or call ftpFolder() with the directory name (e.g. /home/dfaure/dir1/dir2) followed by SIZE with just the filename. The latter sounds like it might work better on android (if it doesn't support absolute paths, maybe it doesn't support ../../foo/bar.txt either?). I have not been able to test whether it supported ../../foo/bar.txt yet. However, making the code relative to m_currentPath seems to work just fine ; so I can avoid having to call ftpFolder and hence sending a cwd request. - Dawit --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116524/#review52013 --- On March 3, 2014, 12:16 a.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116524/ --- (Updated March 3, 2014, 12:16 a.m.) Review request for kdelibs and David Faure. Bugs: 168011 and 326292 http://bugs.kde.org/show_bug.cgi?id=168011 http://bugs.kde.org/show_bug.cgi?id=326292 Repository: kdelibs Description --- This patch changes Ftp::ftpSize such that it has support for servers that do not allow absolute paths with the SIZE command. That means when sending the command SIZE /somefile fails, it will try sending SIZE somefile before giving up. See bug report for details. Diffs - kioslave/ftp/ftp.cpp 5bb2e8d Diff: https://git.reviewboard.kde.org/r/116524/diff/ Testing --- Installed Ftp server from bug report on an Android device and run tests. Thanks, Dawit Alemayehu
Re: Review Request 116524: Make kio_ftp work with ftp server that don't support absolute path with SIZE command
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116524/#review52013 --- kioslave/ftp/ftp.cpp https://git.reviewboard.kde.org/r/116524/#comment36956 This is surely wrong. If you're in /home/dfaure (as the CWD would be, by default, on non-anonymous FTP) and you're downloading a file /home/dfaure/dir1/dir2/file.txt then surely you want to call SIZE dir1/dir2/file.txt and not SIZE home/dfaure/dir1/dir2/file.txt It has to be relative to the CWD, not just skip the first slash, which only works if the CWD is /. This gives two alternatives for the fix: make this code relative to the current CWD whatever it is, or call ftpFolder() with the directory name (e.g. /home/dfaure/dir1/dir2) followed by SIZE with just the filename. The latter sounds like it might work better on android (if it doesn't support absolute paths, maybe it doesn't support ../../foo/bar.txt either?). - David Faure On March 3, 2014, 12:16 a.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116524/ --- (Updated March 3, 2014, 12:16 a.m.) Review request for kdelibs and David Faure. Bugs: 168011 and 326292 http://bugs.kde.org/show_bug.cgi?id=168011 http://bugs.kde.org/show_bug.cgi?id=326292 Repository: kdelibs Description --- This patch changes Ftp::ftpSize such that it has support for servers that do not allow absolute paths with the SIZE command. That means when sending the command SIZE /somefile fails, it will try sending SIZE somefile before giving up. See bug report for details. Diffs - kioslave/ftp/ftp.cpp 5bb2e8d Diff: https://git.reviewboard.kde.org/r/116524/diff/ Testing --- Installed Ftp server from bug report on an Android device and run tests. Thanks, Dawit Alemayehu
Re: Review Request 116524: Make kio_ftp work with ftp server that don't support absolute path with SIZE command
On March 2, 2014, 12:14 a.m., Dawit Alemayehu wrote: I am curious if stripping the leading / and sending a relative path with the SIZE command would work for every FTP server? It's worth a try. I took at look at the very old svn history, and it has just always been an absolute path, we never tried relative. (so it's not like this would be reverting some bugfix) I think it was just a way to avoid sending one more command (cwd). But it seems logical to do that (cwd, then size, then get). - David --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116524/#review51562 --- On March 2, 2014, 12:12 a.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116524/ --- (Updated March 2, 2014, 12:12 a.m.) Review request for kdelibs and David Faure. Bugs: 326292 http://bugs.kde.org/show_bug.cgi?id=326292 Repository: kdelibs Description --- This patch changes Ftp::ftpSize such that it has support for servers that do not allow absolute paths with the SIZE command. That means when sending the command SIZE /somefile fails, it will try sending SIZE somefile before giving up. See bug report for details. Diffs - kioslave/ftp/ftp.cpp 5bb2e8d Diff: https://git.reviewboard.kde.org/r/116524/diff/ Testing --- Installed Ftp server from bug report on an Android device and run tests. Thanks, Dawit Alemayehu
Re: Review Request 116524: Make kio_ftp work with ftp server that don't support absolute path with SIZE command
On March 2, 2014, 12:14 a.m., Dawit Alemayehu wrote: I am curious if stripping the leading / and sending a relative path with the SIZE command would work for every FTP server? David Faure wrote: It's worth a try. I took at look at the very old svn history, and it has just always been an absolute path, we never tried relative. (so it's not like this would be reverting some bugfix) I think it was just a way to avoid sending one more command (cwd). But it seems logical to do that (cwd, then size, then get). Yeah well I changed my mind once I saw the amount of change I would have to make. I have to change how both ftpGet and ftpPut work since everything in kio_ftp uses absolute paths. Instead of changing something that already works quite well, I rather stick to this simple patch that retries the SIZE command for those few servers that do not support using it with absolute paths. I have also updated the patch not to return an error by checking if m_iRespType != 2. Instead it should return UnknownSize. That way when a ftp server has no support or has a disabled SIZE command, kio_ftp will continue to work. This would solve a very old bug in kio_ftp #168011. - Dawit --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116524/#review51562 --- On March 2, 2014, 12:12 a.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116524/ --- (Updated March 2, 2014, 12:12 a.m.) Review request for kdelibs and David Faure. Bugs: 326292 http://bugs.kde.org/show_bug.cgi?id=326292 Repository: kdelibs Description --- This patch changes Ftp::ftpSize such that it has support for servers that do not allow absolute paths with the SIZE command. That means when sending the command SIZE /somefile fails, it will try sending SIZE somefile before giving up. See bug report for details. Diffs - kioslave/ftp/ftp.cpp 5bb2e8d Diff: https://git.reviewboard.kde.org/r/116524/diff/ Testing --- Installed Ftp server from bug report on an Android device and run tests. Thanks, Dawit Alemayehu
Re: Review Request 116524: Make kio_ftp work with ftp server that don't support absolute path with SIZE command
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116524/ --- (Updated March 3, 2014, 12:16 a.m.) Review request for kdelibs and David Faure. Changes --- Updated patch. Now fix also addresses bug# 168011. Bugs: 168011 and 326292 http://bugs.kde.org/show_bug.cgi?id=168011 http://bugs.kde.org/show_bug.cgi?id=326292 Repository: kdelibs Description --- This patch changes Ftp::ftpSize such that it has support for servers that do not allow absolute paths with the SIZE command. That means when sending the command SIZE /somefile fails, it will try sending SIZE somefile before giving up. See bug report for details. Diffs (updated) - kioslave/ftp/ftp.cpp 5bb2e8d Diff: https://git.reviewboard.kde.org/r/116524/diff/ Testing --- Installed Ftp server from bug report on an Android device and run tests. Thanks, Dawit Alemayehu
Re: Review Request 116524: Make kio_ftp work with ftp server that don't support absolute path with SIZE command
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116524/#review51562 --- I am curious if stripping the leading / and sending a relative path with the SIZE command would work for every FTP server? - Dawit Alemayehu On March 2, 2014, 12:12 a.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116524/ --- (Updated March 2, 2014, 12:12 a.m.) Review request for kdelibs and David Faure. Bugs: 326292 http://bugs.kde.org/show_bug.cgi?id=326292 Repository: kdelibs Description --- This patch changes Ftp::ftpSize such that it has support for servers that do not allow absolute paths with the SIZE command. That means when sending the command SIZE /somefile fails, it will try sending SIZE somefile before giving up. See bug report for details. Diffs - kioslave/ftp/ftp.cpp 5bb2e8d Diff: https://git.reviewboard.kde.org/r/116524/diff/ Testing --- Installed Ftp server from bug report on an Android device and run tests. Thanks, Dawit Alemayehu
