<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/125515/">https://git.reviewboard.kde.org/r/125515/</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 4th, 2015, 5:45 p.m. UTC, <b>Frank Reininghaus</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Looks good! The only thing that I'm not sure about is whether 1000 bytes are guaranteed to be enough. Some quick Googling tells me that path lengths of 4096 are possible. Maybe we could allocate a temporary large array on the heap if the readlink call fails with the 1000 byte buffer on the stack?</p></pre>
</blockquote>
<p>On October 4th, 2015, 6:10 p.m. UTC, <b>Xuetian Weng</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Just FYI, even PATH_MAX may not exists on some system (AFAIK, hurd, allowed by posix).
And though st.st_size is mentioned in readlink man page, it may not return useful value.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">More information http://stackoverflow.com/questions/9385386/howto-use-readlink-with-dynamic-memory-allocation</p></pre>
</blockquote>
<p>On October 4th, 2015, 6:18 p.m. UTC, <b>David Faure</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Just for info, I basically restored this code from kdelibs4 :)
But of course I could have a missed a bug report about someone with a huge symlink target.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Checking what Qt does... ah, PATH_MAX if defined, otherwise a loop which reallocs twice more every time (starting from 256 bytes).
Personally I'm tempted to just make it 4096, "it ought to be enough for everybody" as the saying goes :-)</p></pre>
</blockquote>
<p>On October 4th, 2015, 6:29 p.m. UTC, <b>Xuetian Weng</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">So.. is it possible to directly use Qt API there? Why bother handle it ourselves :) ?</p></pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I explain that in the comments in the patch: because Qt resolves relative symlink targets to absolute ones, which is exactly what triggered this bug in the first place.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I think it makes sense for Qt to do so; the 99% use case for symlinks is to read the target file (or dir). In our case however, to copy a symlink, we do care whether it's relative or absolute.</p></pre>
<br />
<p>- David</p>
<br />
<p>On October 4th, 2015, 9:24 a.m. UTC, David Faure wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: 1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
<div>Review request for KDE Frameworks.</div>
<div>By David Faure.</div>
<p style="color: grey;"><i>Updated Oct. 4, 2015, 9:24 a.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="https://bugs.kde.org/show_bug.cgi?id=352927">352927</a>
</div>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kio
</div>
<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;">BUG: 352927
Change-Id: I7d3c988da32cae9d14750c8adb9ca5af6d140572</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">2 unit tests added</p></pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>autotests/jobtest.h <span style="color: grey">(ef8c3e111ec647cc59c5a9715ab3220ce1651c9e)</span></li>
<li>src/ioslaves/file/file.cpp <span style="color: grey">(a0a533c957628b00eff175a949685d4497c5f095)</span></li>
<li>autotests/jobtest.cpp <span style="color: grey">(c24bcead70f78f2bec3b938819fb2fa842e937d5)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/125515/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>