Support for xattrs on kio copy/move
Needs ReviewPublic

Authored by arrowd on Dec 27 2018, 9:38 AM.

Details

Summary

This patch adds support to KIO to preserve xattr of files and directories on copy or move.
Working for copy/move including overwrite and rename cases.
Not working for "write into" when coping folders have name conflict. I'm against adding to this user case as the name of the action don't imply overwrite.

Tested only on Linux.

FEATURE: 116617

Diff Detail

Branch
arcpatch-D17816
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 27179
Build 27197: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes
bruns added inline comments.Apr 1 2020, 4:07 PM
src/ioslaves/file/file_unix.cpp
560

Now try this when the source is e.g. FAT formatted ...

Make this dependent on errno.

cochise updated this revision to Diff 80336.Apr 16 2020, 11:39 PM
cochise marked an inline comment as done.

Are you talking about someting like this, @bruns? 512 bytes would be a good buffer size?
Not sure if prealocatting the buffers would be better than making syscals.

bruns requested changes to this revision.Apr 17 2020, 1:02 AM
bruns added inline comments.
src/ioslaves/file/file_unix.cpp
552

The idea is almost right, the implementation is wrong.

1. set listlen to 0

2. resize keylist to listlen
3. execute flistxattr with size = keylist.size();
4a. if (3.) returns listlen > 0 and keylist.size() == 0, go to (2.)
4b. if (3.) returns listlen > 0 and keylist.size() > 0 -> break
4c. if (3.) returns listlen == -1 and errno == ERANGE, set listlen to 0 and go to (2.)
4d. if (3.) returns listlen == -1 and errno == ENOTSUP -> return
4e. if (3.) returns listlen == 0 -> return

5. resize keylist to listlen, the list may shrink between flistxattr calls.
564

as listlen is -1 here, you always resize to 511 ...

574

man 2 flistxattr:

In addition, the errors documented in stat(2) can also occur.

So any error but ERANGE will exit the loop, and you will have bogus keylist contents.

Do return false on any error but ERANGE in the loop.

After the loop (i.e. when listlen is guaranteed to be > 0), resize keylist to listlen, the list may shrink.

584

Thats bad. Thats really bad.

squeeze may reallocate. This invalidates keyPtr.

597

QByteArray value should be created outside the keyPtr loop.

This reduces the calls to malloc for the QByteArray data storage.

Then for each attribute, do a value.resize(value.capacity).

607

-1 + 512 = 511 ...

Should be value.resize(value.size() + 512) or something similar.

609

Again, valuelen may be -1 here ...

This revision now requires changes to proceed.Apr 17 2020, 1:02 AM
arrowd commandeered this revision.May 15 2020, 4:04 PM
arrowd added a reviewer: cochise.
arrowd updated this revision to Diff 82960.May 15 2020, 4:06 PM

I decided to help with this a bit.

  • Fix detection of <sys/extattr.h> header. It requires <sys/types.h> to be included too.
  • Implement test helpers for BSD (extattr).

At the moment, some tests fail for me, because xattrs don't seem to be copied. I'll look into that later.

arrowd updated this revision to Diff 83016.May 17 2020, 10:27 AM
  • Fix tests on FreeBSD.
  • Fix bug: Move keylist.squeeze() call before acquiring an interator.

At this stage, all tests in jobtest pass on FreeBSD.

arrowd updated this revision to Diff 83068.May 19 2020, 7:28 PM
arrowd marked 18 inline comments as done.
  • Use full paths to command line utilities and pass xattr args correctly.
  • Mark some stale comments as done.

I decided to help with this a bit.

My hands are full at the moment, so I'm unable to finish this for some time. Thank you.

At this stage, all tests in jobtest pass on FreeBSD.

I'm very happy to see this working well on BSD as well.

arrowd updated this revision to Diff 83143.May 24 2020, 3:05 PM
arrowd marked 5 inline comments as done.
  • Refactor common code from compareXattr() into readXattr() as requested in comments.
bruns added inline comments.May 26 2020, 12:53 AM
autotests/jobtest.cpp
495

std::function<QStringList(const QString&, const QString&, const QString&)> m_setXattrFormatArgs;
...

m_setXattrFormatArgs = [](const QString& attrName, const QString& value, const QString& fileName) {
    return QStringList{QLatin1String("-n"), attrName, QLatin1String("-v"), value, fileName};
}

and do it when doing the path lookup, once.

arrowd updated this revision to Diff 83166.May 27 2020, 8:57 PM
arrowd marked an inline comment as done.
  • Use std::function to store a generator function for m_setXattrCmd's arguments.
arrowd marked an inline comment as done.May 27 2020, 8:59 PM

Mark stale command as done.

bruns requested changes to this revision.May 27 2020, 9:43 PM
bruns added inline comments.
autotests/jobtest.cpp
471

this obviously needs test cases with a key ley/value len > 512, to check the the array-to-short/resize path.

src/ioslaves/file/file_unix.cpp
566

This is still definitely wrong ...

611

also wrong ...

This revision now requires changes to proceed.May 27 2020, 9:43 PM
arrowd updated this revision to Diff 83192.Jun 1 2020, 6:03 PM
arrowd marked 2 inline comments as done.
  • Change the algorithm in FileProtocol::copyXattrs()
arrowd marked an inline comment as done.Jun 1 2020, 6:05 PM
bruns requested changes to this revision.Jun 1 2020, 6:24 PM
bruns added inline comments.
src/ioslaves/file/file_unix.cpp
617

WRONG WRONG WRONG !!!

This revision now requires changes to proceed.Jun 1 2020, 6:24 PM
arrowd updated this revision to Diff 83203.Jun 2 2020, 7:42 PM
arrowd marked 17 inline comments as done.
  • Address comments.
arrowd added a comment.Jun 2 2020, 7:43 PM

Almost every comment have been addressed. Please, give this another look.

bruns added inline comments.Jun 2 2020, 11:13 PM
src/ioslaves/file/file_unix.cpp
553

just QByteArray keylist;

554

while (true) instead of do {} while(true)

575

"does not"

586

why .squeeze() ?

588

"return a list of null terminated strings"

589

"BSDs return a list of items, ..." or "BSD returns a list of items, ..."
"..., each item consisting of the size byte prepended to the key."

596

"key size" or "size of the key"

637

"does not"

arrowd updated this revision to Diff 83207.Jun 3 2020, 5:55 PM
arrowd marked 8 inline comments as done.
  • Address comments.
arrowd added a comment.Jun 3 2020, 5:56 PM

I'd appreciate if someone test this on Linux and MacOS. I got FreeBSD covered.

arrowd updated this revision to Diff 83302.Jun 20 2020, 4:30 PM
  • Rebase on master.
bruns added inline comments.Jun 22 2020, 2:51 PM
src/ioslaves/file/file_unix.cpp
580

src_fd is quite meaningless as debug output

625

Infinite loop on valuelen == 0

arrowd updated this revision to Diff 83309.Jun 23 2020, 4:44 PM
arrowd marked an inline comment as done.
  • Improve comment.
arrowd added inline comments.Jun 23 2020, 5:23 PM
src/ioslaves/file/file_unix.cpp
625

Why? ERANGE means we need to come up with new value for valuelen, so we set it to zero and start over. On the new iteration it gets passed into fgetxattr/extattr_get_fd to find out sufficient value.

bruns added inline comments.Jun 24 2020, 12:29 PM
src/ioslaves/file/file_unix.cpp
625

"0" is a regular return value. Try your code with the following file:

touch t
setfattr -n user.foo -v "" t

and preferably add a test case ...

arrowd updated this revision to Diff 83318.Jun 29 2020, 6:09 PM
arrowd marked 3 inline comments as done.
  • Handle attrs with empty values.
  • Add test for it.
  • Fix syscalls for FreeBSD case.
bruns added inline comments.Jun 29 2020, 8:15 PM
autotests/jobtest.cpp
471

not done ...

src/ioslaves/file/file_unix.cpp
620

No extra spaces for qDebug, inserts spaces itself.

arrowd updated this revision to Diff 83319.Jun 30 2020, 8:07 AM
arrowd marked an inline comment as done.
  • Remove extraneous debugging output.
arrowd added inline comments.Jun 30 2020, 8:08 AM
autotests/jobtest.cpp
471

There are no arrays of 512 size anymore. Or am I missing something?

arrowd updated this revision to Diff 83334.Thu, Jul 30, 9:00 AM

Rebase on master.