<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/106636/">http://git.reviewboard.kde.org/r/106636/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On October 1st, 2012, 8:30 a.m., <b>David Faure</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="http://git.reviewboard.kde.org/r/106636/diff/2/?file=87785#file87785line2639" style="color: black; font-weight: bold; text-decoration: underline;">kioslave/ftp/ftp.cpp</a>
<span style="font-weight: normal;">
(Diff revision 2)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">void Ftp::fixupEntryName(FtpEntry* e)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">2639</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">if</span> <span class="p">(</span><span class="o">!</span><span class="n">ftpFileExists</span><span class="p">(</span><span class="n">e</span><span class="o">-></span><span class="n">name</span><span class="p">))</span> <span class="p">{</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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...</pre>
</blockquote>
<p>On October 2nd, 2012, 12:18 a.m., <b>Dawit Alemayehu</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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 ?
</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
<br />
<p>- David</p>
<br />
<p>On October 2nd, 2012, 4:08 a.m., Dawit Alemayehu wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/media/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for kdelibs and David Faure.</div>
<div>By Dawit Alemayehu.</div>
<p style="color: grey;"><i>Updated Oct. 2, 2012, 4:08 a.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</td>
</tr>
</table>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="http://bugs.kde.org/show_bug.cgi?id=300988">300988</a>
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>kioslave/ftp/ftp.h <span style="color: grey">(2465a4b)</span></li>
<li>kioslave/ftp/ftp.cpp <span style="color: grey">(26be283)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/106636/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>