<table><tr><td style="">cochise added a comment.
</td><a style="text-decoration: none; padding: 4px 8px; margin: 0 8px 8px; float: right; color: #464C5C; font-weight: bold; border-radius: 3px; background-color: #F7F7F9; background-image: linear-gradient(to bottom,#fff,#f1f0f1); display: inline-block; border: 1px solid rgba(71,87,120,.2);" href="https://phabricator.kde.org/D17816">View Revision</a></tr></table><br /><div><div><blockquote style="border-left: 3px solid #8C98B8;
color: #6B748C;
font-style: italic;
margin: 4px 0 12px 0;
padding: 8px 12px;
background-color: #F8F9FC;">
<div style="font-style: normal;
padding-bottom: 4px;">In <a href="https://phabricator.kde.org/D17816#386810" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: none;">D17816#386810</a>, <a href="https://phabricator.kde.org/p/funkybomber/" style="
border-color: #f1f7ff;
color: #19558d;
background-color: #f1f7ff;
border: 1px solid transparent;
border-radius: 3px;
font-weight: bold;
padding: 0 4px;">@funkybomber</a> wrote:</div>
<div style="margin: 0;
padding: 0;
border: 0;
color: rgb(107, 116, 140);"><p>On a related note, is it possible that the "QSaveFile" is also responsible for a similar behaviour in Ark?</p></div>
</blockquote>
<p>Probably yes: <a href="https://github.com/KDE/ark/search?q=qsavefile&unscoped_q=qsavefile" class="remarkup-link" target="_blank" rel="noreferrer">https://github.com/KDE/ark/search?q=qsavefile&unscoped_q=qsavefile</a></p>
<p><a href="https://github.com/search?p=1&q=org%3AKDE+qsavefile&type=Code" class="remarkup-link" target="_blank" rel="noreferrer">Many</a> KDE applications use this class (the most prominent ones being Krita and Ark), and it provides a useful feature:</p>
<blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><p>QSaveFile is an I/O device for writing text and binary files, without losing existing data if the writing operation fails.<br />
While writing, the contents will be written to a temporary file, and if no error happened, commit() will move it to the final file.</p></blockquote>
<p>But as it causes loss of ACLs and xattrs as side effect, I think it's use deserves a proper discussion in a dedicated topic.</p>
<p><strong>On the support for KIO::file_copy:</strong></p>
<p>Trying to add support on the low level <tt style="background: #ebebeb; font-size: 13px;">KIO::file_copy</tt> I found that it would be hard without code duplication or exposing the function to copy xattrs that are currently on <tt style="background: #ebebeb; font-size: 13px;">KIO::CopyJobPrivate</tt>, but this would change the API, adding a non virtual method, what I <strong>think</strong> wont break the ABI.</p>
<p>I think the best way to do this is:</p>
<ol class="remarkup-list">
<li class="remarkup-list-item">Put copyXattr as a public method of <tt style="background: #ebebeb; font-size: 13px;">FileCopyJob</tt>.</li>
<li class="remarkup-list-item">Call copyXattr for files in <tt style="background: #ebebeb; font-size: 13px;">FileCopyJob::file_copy</tt>, because <tt style="background: #ebebeb; font-size: 13px;">CopyJob::copy</tt> uses it internally.</li>
<li class="remarkup-list-item">Call copyXattr for directories on CopyJob, because it's the lowest abstraction level that knows that the mkdir have a source.</li>
</ol>
<p>I can't find uses of file_copy related to user files in a <strong>very</strong> superficial search, only backups, config files and network stuff. So, in theory, the current patch is good enough and we don't need to change the API, if this is a problem.</p></div></div><br /><div><strong>INLINE COMMENTS</strong><div><div style="margin: 6px 0 12px 0;"><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D17816#inline-98745">View Inline</a><span style="color: #4b4d51; font-weight: bold;">pino</span> wrote in <span style="color: #4b4d51; font-weight: bold;">jobtest.cpp:534</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">you can use <tt style="background: #ebebeb; font-size: 13px;">QStandardPaths::findExecutable()</tt> to check whether <tt style="background: #ebebeb; font-size: 13px;">setfattr</tt> is available, and QSKIP the test if not</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">On linux, the command line bin are list/get/set<strong>f</strong>attr, on BSD list/get/set<strong>x</strong>attr, on mac are xattr -l/-p/-w, so this part will need a lot of ifdefs too, to work on many platforms.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D17816#inline-98263">View Inline</a><span style="color: #4b4d51; font-weight: bold;">pino</span> wrote in <span style="color: #4b4d51; font-weight: bold;">config-kiocore.h.cmake:8</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;"><tt style="background: #ebebeb; font-size: 13px;">HAVE_SYS_XATTR_H</tt> is available here as side-effect of using the FindACL.cmake module.<br />
Better explicitly look for <tt style="background: #ebebeb; font-size: 13px;">sys/xattr.h</tt>, like <tt style="background: #ebebeb; font-size: 13px;">src/ioslaves/file/CMakeLists.txt</tt> does.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Didn't get it defined until added the snippet. Will research <tt style="background: #ebebeb; font-size: 13px;">src/ioslaves/file/CMakeLists.txt</tt> to see if I can simplify the include for *BSD too.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D17816#inline-98265">View Inline</a><span style="color: #4b4d51; font-weight: bold;">pino</span> wrote in <span style="color: #4b4d51; font-weight: bold;">copyjob.cpp:2191-2193</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">if <tt style="background: #ebebeb; font-size: 13px;">HAVE_SYS_XATTR_H</tt> is not defined, the first instruction in <tt style="background: #ebebeb; font-size: 13px;">copyXattrs</tt> will be a return, and some compilers may warn/error out because the rest of the function is unreachable; better stub out the whole function instead</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">I think looks better this way, but if it throw a warning I will revert to encapsulating the whole function.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D17816#inline-98262">View Inline</a><span style="color: #4b4d51; font-weight: bold;">pino</span> wrote in <span style="color: #4b4d51; font-weight: bold;">copyjob.cpp:2195</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><ul class="remarkup-list">
<li class="remarkup-list-item">you don't need to call <tt style="background: #ebebeb; font-size: 13px;">data()</tt> here, the return value of <tt style="background: #ebebeb; font-size: 13px;">toLocal8Bit()</tt> is already a QByteArray</li>
<li class="remarkup-list-item"><tt style="background: #ebebeb; font-size: 13px;">toLocal8Bit()</tt> is the wrong function here: please use <tt style="background: #ebebeb; font-size: 13px;">QFile::encodeName()</tt>, which does the right job for QString -> local filesystem paths</li>
</ul></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">I need <tt style="background: #ebebeb; font-size: 13px;">data()</tt>, because the compiler complains about non private cast. but using <tt style="background: #ebebeb; font-size: 13px;">encodeName()</tt> should be better either way.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D17816#inline-98270">View Inline</a><span style="color: #4b4d51; font-weight: bold;">pino</span> wrote in <span style="color: #4b4d51; font-weight: bold;">copyjob.cpp:2206</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">there is not just glibc; also, better check for <tt style="background: #ebebeb; font-size: 13px;">errno</tt> as <tt style="background: #ebebeb; font-size: 13px;">ENOTSUP</tt>, because that means the source filesystem does not support xattrs, and thus you can directly skip the rest of the function (as it will not work anyway)</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">If a error is found at this point, all the rest of the function will not run, because it is on the a statement, but doing a early check for destination filesystem support of xattr is a good idea.</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R241 KIO</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D17816">https://phabricator.kde.org/D17816</a></div></div><br /><div><strong>To: </strong>cochise, dfaure<br /><strong>Cc: </strong>phidrho, dhaumann, funkybomber, abika, pino, davidedmundson, ngraham, atha.kane, spoorun, nicolasfella, kde-frameworks-devel, michaelh, bruns<br /></div>