<table><tr><td style="">dfaure requested changes to this revision.<br />dfaure added inline comments.<br />This revision now requires changes to proceed.
</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><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-148163">View Inline</a><span style="color: #4b4d51; font-weight: bold;">jobtest.cpp:529</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span class="n">QHash</span><span style="color: #aa2211"><</span><span class="n">QString</span><span class="p">,</span> <span class="n">QString</span><span style="color: #aa2211">></span> <span class="n">getSampleAttrs</span><span class="p">()</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span class="p">{</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">prepend <tt style="background: #ebebeb; font-size: 13px;">static</tt></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-148164">View Inline</a><span style="color: #4b4d51; font-weight: bold;">jobtest.cpp:626</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span class="n">QString</span> <span class="n">getXattrCommand</span><span class="p">()</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span class="p">{</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">prepend <tt style="background: #ebebeb; font-size: 13px;">static</tt></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-148165">View Inline</a><span style="color: #4b4d51; font-weight: bold;">jobtest.cpp:638</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">if</span> <span class="p">(</span><span class="n">command</span><span class="p">.</span><span class="n">isEmpty</span><span class="p">())</span> <span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">command</span> <span style="color: #aa2211">=</span> <span class="n">QStandardPaths</span><span style="color: #aa2211">::</span><span class="n">findExecutable</span><span class="p">(</span><span style="color: #766510">"xattr"</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="p">}</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><div class="remarkup-code-block" style="margin: 12px 0;" data-code-lang="text" data-sigil="remarkup-code-block"><pre class="remarkup-code" style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; padding: 12px; margin: 0; background: rgba(71, 87, 120, 0.08);">if (command.isEmpty())
qWarning() << "ERROR: setfattr/setextattr/xattr not found";</pre></div>
<p style="padding: 0; margin: 8px;">to make it easier to debug than just this method returning empty.</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-148166">View Inline</a><span style="color: #4b4d51; font-weight: bold;">jobtest.cpp:650</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">if</span> <span class="p">(</span><span class="n">command</span><span class="p">.</span><span class="n">isEmpty</span><span class="p">()</span> <span style="color: #aa2211">||</span> <span class="n">command</span> <span style="color: #aa2211">!=</span> <span style="color: #766510">"setfattr"</span><span class="p">)</span> <span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">return</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="p">}</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">QSKIP("This test requires setfattr")</p>
<p style="padding: 0; margin: 8px;">Otherwise it will look like a "PASS" in the test output.</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-148167">View Inline</a><span style="color: #4b4d51; font-weight: bold;">jobtest.cpp:753</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">if</span> <span class="p">(</span><span class="n">command</span><span class="p">.</span><span class="n">isEmpty</span><span class="p">()</span> <span style="color: #aa2211">||</span> <span class="n">command</span> <span style="color: #aa2211">!=</span> <span style="color: #766510">"setfattr"</span><span class="p">)</span> <span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">return</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="p">}</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">QSKIP</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-147603">View Inline</a><span style="color: #4b4d51; font-weight: bold;">tmarshall</span> wrote in <span style="color: #4b4d51; font-weight: bold;">copyjob.cpp:1119</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">Thanks to <span class="phabricator-remarkup-mention-unknown">@ahmad78</span> for chatting with me about this in irc.</p>
<p style="padding: 0; margin: 8px;">Everything works for me if I replace the <tt style="background: #ebebeb; font-size: 13px;">exec</tt> call with a <tt style="background: #ebebeb; font-size: 13px;">start</tt> call. Currently the result of the exec call is thrown away as the job itself does the error handling and there isn't much to do if the xattrs can't be copied for whatever reason.</p>
<p style="padding: 0; margin: 8px;">As for why this is a different job, that was a design decision by the original author. It seems like a decent enough way forward to me as it leans on the side of modularity. The applications of xattrs are so broad that it's hard to say what they will be used for in the future or what the desired functionality might be. Having a separate job will allow for more flexibility regarding xattrs going forward.</p>
<p style="padding: 0; margin: 8px;">In short, I don't really think it hurts to have a separate job and there is certainly the potential that it comes in handy.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">I don't mind that errors don't propagate up here, that's fine, we certainly don't want the copy to fail if attributes can't be copied.</p>
<p style="padding: 0; margin: 8px;">I do mind that this uses exec() (Qt eventloop re-entrancy is a nasty source of bugs), and I do mind a fire-and-forget subjob (if that's what start() here does -- unless the subjob gets registered?).</p>
<p style="padding: 0; margin: 8px;">I have provided the solution already, connect the subjob's result to a lambda that contains the rest of the code in this method [but see below].</p>
<p style="padding: 0; margin: 8px;">I just realized another problem: this is at the wrong layer. It should be in FileCopyJob, not in the high-level CopyJob (which lists directories, creates directories, etc. and delegates the task of copying one file to FileCopyJob). FileCopyJob is also used directly in many places (when a single file has to be copied) so if you want that to preserve xattr (rethorical question, I'm assuming you do) this should go down to FileCopyJob.</p>
<p style="padding: 0; margin: 8px;">About this being a different job: actually, one doesn't prevent the other. Look at permissions. We have ChmodJob for setting permissions, but when FileCopyJob finds that the kioslave supports copy() (see DirectCopyJob on the app side) then copy() takes care of copying permissions too. FileCopyJob only calls ChmodJob for the special case of renaming and because the FileCopyJob API actually allows to change permissions, this doesn't apply to xattr.</p>
<p style="padding: 0; margin: 8px;">When the kioslave doesn't support copy() and it falls back to "data pump" mode (get() + put()), FileCopyJob passes the permissions to the put() job. *That* is the case where it should follow that with a KIO::copy_xattr subjob, unless xattr can be passed by metadata to the put job?</p>
<p style="padding: 0; margin: 8px;">I believe the same idea should be done for xattr (kio_file's copy() should copy xattr all by itself), to minimize roundtrips between the app and the slave. People complain that copying files with KIO is too slow, let's not make it worse.</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-99766">View Inline</a><span style="color: #4b4d51; font-weight: bold;">cfeck</span> wrote in <span style="color: #4b4d51; font-weight: bold;">filecopyjob.cpp:519</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">This waits (i.e. spawns a separate event loop) until the job is finished. Should use a subjob.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">That's what should happen in copy() IMHO. But that's an optimization, I can accept that being done later.</p>
<p style="padding: 0; margin: 8px;">And if you want to support other cases than file->file, like desktop, trash, smb, etc. then this should also be done when the put job finishes.</p>
<p style="padding: 0; margin: 8px;">What's for sure is that doing it both in CopyJob and in FileCopyJob is overkill since the former uses the latter :-)</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>tmarshall, dfaure, chinmoyr, bruns, Frameworks, cochise<br /><strong>Cc: </strong>anthonyfieroni, tmarshall, arrowd, cfeck, bruns, phidrho, dhaumann, funkybomber, abika, pino, davidedmundson, ngraham, atha.kane, spoorun, nicolasfella, kde-frameworks-devel, LeGast00n, GB_2, michaelh<br /></div>