> On Oct. 1, 2012, 8:30 a.m., David Faure wrote: > > kioslave/ftp/ftp.cpp, line 2639 > > <http://git.reviewboard.kde.org/r/106636/diff/2/?file=87785#file87785line2639> > > > > Does this really work? This method is being called in the middle of the > > parsing of "list" command output, so the output of the "SIZE" command > > issued by ftpFileExists will only come after the end of the directory > > listing, won't it? > > > > Did you test this with a non-trivial folder? (10 elements should be > > enough, unless I'm missing some buffering somewhere). > > > > It sounds to me like the filename fixing up should come *after* the > > full directory listing. > > > > Also, this looks very slow. On a "normal" FTP directory without > > whitespace to be fixed up, this is going to issue one SIZE command per file > > and one CWD command per subdirectory (and a lot more in case of whitespace > > issues). Maybe we can do the fixing up only when retrieving a single file > > or entering a subdir later on, to avoid slowing down directory listing so > > much. On a large folder and a slow FTP server, this must be unbearably > > slower... > > Dawit Alemayehu wrote: > Well, I did test it on my own machine using vsftpd. I created 300 folders > and text files (150 each) whose name starts with a space and accessed that > directory. The listing worked just fine and I did not see any lag. However, > that is on a local machine. I do not have a remote ftp server with which to > test this at the moment. However, I see your point. I too am now curious how > this works at all and it does work for me as I have stated. Perhaps the Qt > socket classes do some buffering ? > > As far as "normal" FTP directory without whitespace being fixed up, the > very first if statement in the fixupEntryName function prevents it so that > should be a not be an issue. > > My original inclination was to fix it as you suggested here. That is if > listing the directory or retrieving a file fails, do the fixup and try again > before returning an error to the client. However, that would mean fixing it > in a lot of places, e.g. when deleting, renaming, moving, etc. That is in > addition to the listing and retrieving. But that is not even the worst of it. > The potential to trick the user is the biggest issue. For example, a user > sees the whitespace in a file or folder name and wants to remove it. > Unfortunately any attempt to do so through rename will simply fail because > there is no whitespace in the name to begin with. :( > > I wonder how other clients deal with such issues ? Perhaps they do not > and simply trim all whitespaces and ignore servers that allow such things ? > >
The "very first if statement in fixupEntryName" is still an FTP operation (SIZE or CWD), that's my point (about hurting performance). I see your point, we have to show a correct filename to the user in the first place, so forget my idea of on-demand fixup. As to other clients, well, you have the perfect setup for testing this... Personnally I would just trim whitespace and not allow people to store files that start with a space on FTP. It just doesn't make sense, given that FTP is a text-based whitespace-separated protocol. Thanks. - David ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/106636/#review19675 ----------------------------------------------------------- On Oct. 2, 2012, 4:08 a.m., Dawit Alemayehu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/106636/ > ----------------------------------------------------------- > > (Updated Oct. 2, 2012, 4:08 a.m.) > > > Review request for kdelibs and David Faure. > > > Description > ------- > > The attached patch fixes a regression caused by a commit to fix bug# 88575. > Namely, it fixed a problem where filenames with whitespaces in them were not > handled correctly by kio_ftp. That is because the filenames were > automatically trimmed when read from the directory. However, the fix then > re-introduced the original bug and the reason why names were automatically > trimmed in the first place. Some ftp servers add bogus whitespace between the > date and filename in their listings. Hence, we need need to fix both of these > opposing issues without breaking the other. This patch tries to do just that > by actually validating each name entry that starts with a whitespace. That > way the correct name is sent to the client. > > > This addresses bug 300988. > http://bugs.kde.org/show_bug.cgi?id=300988 > > > Diffs > ----- > > kioslave/ftp/ftp.h 2465a4b > kioslave/ftp/ftp.cpp 26be283 > > Diff: http://git.reviewboard.kde.org/r/106636/diff/ > > > Testing > ------- > > > Thanks, > > Dawit Alemayehu > >
