dfaure requested changes to this revision.
dfaure added inline comments.
This revision now requires changes to proceed.
INLINE COMMENTS
> file_unix.cpp:165
> + int dest_fd = fdRecv.fileDescriptor();
> + if (!dest_file.open(dest_fd, QIODevice::Truncate |
> QIODevice::WriteOnly, QFileDevice::AutoCloseHandle)) {
> + if (!ret.wasCanceled()) {
What will fileDescriptor() return if fdRecv isn't valid? -1 I suppose? So this
code is going to call open(-1), a bit weird.
Maybe it would be easier if all of the above was moved to a helper function or
lambda (with dest_file as input arg).
Basically we want to write
if (auto ret = tryOpen(dest_file, [...])) {
if (!ret.wasCanceled()) {
...
This way you can use early returns in tryOpen when fdRecv isn't valid, when
execWithElevatedPrivilege returns anything other than success, or when
QFile::open fails. Much better than nested ifs, or missing ifs like above ;)
> file_unix.cpp:175
> + src_file.close();
> + return;;
> }
One should be enough ;)
> file_unix.cpp:181
> + if (!dest_file.setPermissions(QFileDevice::ReadOwner |
> QFileDevice::WriteOwner)) {
> + // File was created by root user with the above permissions. We only
> need to
> + // change its owner.
Was it? How do you know the permissions? Don't they depent on the umask?
Unless I missed something, it would seem to me that a chmod is needed, to
ensure restricted permissions. Alternatively, chown + setPermissions again,
once the user owns the file.
> file_unix.cpp:343
> if (::utime(_dest.data(), &ut) != 0) {
> - qCWarning(KIO_FILE) << QStringLiteral("Couldn't preserve access and
> modification time for '%1'").arg(dest);
> + if (execWithElevatedPrivilege(UTIME, _dest, qint64(ut.actime),
> qint64(ut.modtime))) {
> + qCWarning(KIO_FILE) << QStringLiteral("Couldn't preserve access
> and modification time for '%1'").arg(dest);
These cases - without a variable inside the if - could be made more readable
with if (exec(...).failed()) { ... } (or whatever the method name was)
REVISION DETAIL
https://phabricator.kde.org/D6830
To: chinmoyr, dfaure, #frameworks
Cc: elvisangelaccio, #frameworks
