<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/D6829" rel="noreferrer">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/D6829#inline-30001" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">slavebase.cpp:1461</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">PrivilegeOperationStatus</span> <span class="n">SlaveBase</span><span style="color: #aa2211">::</span><span class="n">privilegeOperationStatus</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;">Oh, indeed, can't be const, because it's not a getter, it's a method that asks for the status via communication with the app.<br />
That means the naming could be improved.</p>
<p style="padding: 0; margin: 8px;">Maybe something like queryPrivilegeOperationStatus() or requestPrivilegeOperation().</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/D6829#inline-30002" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">slavebase.cpp:1468</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">KIO</span><span style="color: #aa2211">::</span><span class="n">PrivilegeOperationStatus</span> <span class="n">currentOperationStatus</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">switch</span> <span class="p">(</span><span class="n">buffer</span><span class="p">.</span><span class="n">toInt</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">case</span> <span style="color: #601200">1</span><span style="color: #aa2211">:</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Not needed, just return KIO::PrivilegeOperationStatus(buffer.toInt()) (constructor-like syntax - or static_cast if that doesn't work).</p>
<p style="padding: 0; margin: 8px;">If you think casting is evil, hardcoding 1/2/3 is even more evil ;-)</p>
<p style="padding: 0; margin: 8px;">And in this case casting is not evil at all, we simply serialize/deserialize the enum using its integral value.</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/D6829#inline-29999" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">slavebase.h:940</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: #74777d"> * Checks with job if privilege operation is allowed.</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span style="color: #74777d"> * @return one of the four status, OperationNotAllowed, OperationAllowed or OperationCanceled.</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span style="color: #74777d"> * @see PrivilegeOperationStatus</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Four? I count only three. No need to duplicate the full enum docu anyway, it's bound to go out-of-date at some point, just refer to the enum.</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/D6829#inline-30003" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">file_unix.cpp:666</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">opStatus</span> <span style="color: #aa2211">==</span> <span class="n">KIO</span><span style="color: #aa2211">::</span><span class="n">OperationCanceled</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">mUserCanceled</span> <span style="color: #aa2211">=</span> <span style="color: #304a96">true</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;">No chance to return an enum instead of a bool here, to avoid modifying state?</p>
<p style="padding: 0; margin: 8px;">The problem with mUserCanceled is that it's easy to get it wrong. Like in this patch, which never resets it to false for the next operation after one canceled operation. It's just cleaner to return an enum instead of modifying a member variable temporarily, although the question is, in both cases, what the caller code will look like...</p>
<p style="padding: 0; margin: 8px;">My fear with a member var is that too many callers will forget to check it. <br />
Let's approach this from the viewpoint of the calling code.</p>
<p style="padding: 0; margin: 8px;">Before:</p>
<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 (!dir.rmdir(itemPath)) {
error(KIO::ERR_CANNOT_DELETE, itemPath);
return false;
}</pre></div>
<p style="padding: 0; margin: 8px;">After:</p>
<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 (!dir.rmdir(itemPath)) {
if (auto ret = execWithElevatedPrivilege(RMDIR, itemPath)) { // for this to work, success must be 0 (or the returned type can be a class with operator bool()...)
if (ret == KIO::OperationCanceled) { // or a different enum
error(KIO::ERR_USER_CANCELED, itemPath);
} else {
error(KIO::ERR_CANNOT_DELETE, itemPath);
}
return false;
}
}</pre></div>
<p style="padding: 0; margin: 8px;">Alternatively, since we know cancelling will always need to error(KIO::ERR_USER_CANCELED) (and the QString argument doesn't matter), we could call error from within execWithElevatedPrivilege. But we still need the enum ret val:</p>
<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 (!dir.rmdir(itemPath)) {
if (auto ret = execWithElevatedPrivilege(RMDIR, itemPath)) { // see above
if (ret != KIO::OperationCanceled) { // or a different enum
error(KIO::ERR_CANNOT_DELETE, itemPath);
}
return false;
}
}</pre></div>
<p style="padding: 0; margin: 8px;">I actually like the idea of returning a small class. Then it could even have a wasCanceled() method... we don't need an enum, we need a value that can convert to bool (for the if) and additionally let us know if cancelation happened. But all this in a temporary value, not in a member var of slavebase.</p>
<p style="padding: 0; margin: 8px;">To flesh it out in more details:</p>
<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);">class ExecWithElevatedPrivilegeReturnValue
{
public:
static ExecWithElevatedPrivilegeReturnValue success() { return ExecWithElevatedPrivilegeReturnValue{true, false}; }
static ExecWithElevatedPrivilegeReturnValue failure() { return ExecWithElevatedPrivilegeReturnValue{true, false}; }
static ExecWithElevatedPrivilegeReturnValue canceled() { return ExecWithElevatedPrivilegeReturnValue{true, true}; }
operator bool() const { return m_success; }
bool wasCanceled() const { return m_canceled; }
private:
ExecWithElevatedPrivilegeReturnValue(bool success, bool canceled) : m_success(success), m_canceled(canceled) {}
const bool m_success;
const bool m_canceled;
};</pre></div>
<p style="padding: 0; margin: 8px;">(no setters, all of the slave code shouldn't be able to change this object, it's created immediately with the right values)</p>
<p style="padding: 0; margin: 8px;">With this, the calling code can be simply</p>
<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 (!dir.rmdir(itemPath)) {
if (auto ret = execWithElevatedPrivilege(RMDIR, itemPath)) {
if (!ret.wasCanceled()) {
error(KIO::ERR_CANNOT_DELETE, itemPath);
}
return false;
}
}</pre></div>
<p style="padding: 0; margin: 8px;">and the code in this review (execWithElevatedPrivilege) can do things like <tt style="background: #ebebeb; font-size: 13px;">return ExecWithElevatedPrivilegeReturnValue::canceled();</tt> etc.</p>
<p style="padding: 0; margin: 8px;">How does that sound?</p></div></div></div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D6829" rel="noreferrer">https://phabricator.kde.org/D6829</a></div></div><br /><div><strong>To: </strong>chinmoyr, dfaure, Frameworks<br /><strong>Cc: </strong>Frameworks<br /></div>