D6830: Make use of kauth helper in copy method of file ioslave

[David Faure]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20170824/dd37326d/attachment-0001.html>