dfaure requested changes to this revision.
dfaure added inline comments.
This revision now requires changes to proceed.
INLINE COMMENTS
> chinmoyr wrote in filehelper.cpp:86
> 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?
The documentation for chown and others says:
Upon successful completion, these functions shall return 0. Otherwise, these
functions shall return −1 and set errno to indicate the error.
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".
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.
> filehelper.cpp:41
> +
> +enum {
> + CHMOD = 1,
I assume this enum is duplicated elsewhere (in whoever is serializing the
arguments for FileHelper), right?
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.
> filehelper.cpp:102
> + closedir(dp);
> + }
> + sendFileDescriptor(-1);
missing else or break here, no?
You're sending the valid fd, followed by -1.
REVISION DETAIL
https://phabricator.kde.org/D6197
To: chinmoyr, elvisangelaccio, #frameworks, dfaure
