<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/D6197" 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/D6197#inline-28721" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">chinmoyr</span> wrote in <span style="color: #4b4d51; font-weight: bold;">filehelper.cpp:86</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">In case of OPEN and OPENDIR, I can see why we need error checking, opendir() can return null and there are some issues (I read on internet, can't recall the source) if close() fails. But in case of single function calls like chmod() I can't see how return value matters. Say if unlink fails for whatever reasons, we can return the error code and the checking for exact error code can be done by ioslave. Is there something I am overlooking?</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">The documentation for chown and others says:</p>

<p style="padding: 0; margin: 8px;">Upon successful completion, these functions shall return 0.  Otherwise, these functions shall return −1 and set errno to indicate the error.</p>

<p style="padding: 0; margin: 8px;">It does NOT say, that errno isn't set when the function is successful. It seems to me that it would be perfectly valid for a libc implementation to do something like "try this, it fails (and sets errno), then try that, it worked, return 0".</p>

<p style="padding: 0; margin: 8px;">For this reason I would feel much safer (especially in code run by root!) if the error handling was more classic, i.e. by checking return values.</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/D6197#inline-28903" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">filehelper.cpp:41</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">enum</span> <span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">    <span class="n">CHMOD</span> <span style="color: #aa2211">=</span> <span style="color: #601200">1</span><span class="p">,</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">I assume this enum is duplicated elsewhere (in whoever is serializing the arguments for FileHelper), right?<br />
Can the duplication be avoided by sharing a private header? Otherwise this at least needs a comment like "keep in sync with..." in both places.</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/D6197#inline-28900" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">filehelper.cpp:102</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">closedir</span><span class="p">(</span><span class="n">dp</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 style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">        <span class="n">sendFileDescriptor</span><span class="p">(</span><span style="color: #aa2211">-</span><span style="color: #601200">1</span><span class="p">);</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">missing else or break here, no?<br />
You're sending the valid fd, followed by -1.</p></div></div></div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D6197" rel="noreferrer">https://phabricator.kde.org/D6197</a></div></div><br /><div><strong>To: </strong>chinmoyr, elvisangelaccio, Frameworks, dfaure<br /></div>