D17816: Support for xattrs on kio copy/move

cochise added a comment. In D17816#671870 , @arrowd wrote: > 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. In D17816#672405

D17816: Support for xattrs on kio copy/move

D17816: Support for xattrs on kio copy/move

cochise updated this revision to Diff 79054. cochise added a comment. qCDebug -> qCWarning REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D17816?vs=78777=79054 BRANCH arcpatch-D17816_2 REVISION DETAIL https://phabricator.kde.org/D17816 AFFECTED FILES

D17816: Support for xattrs on kio copy/move

cochise marked an inline comment as done. cochise added a comment. Hi, @usta and @bruns We still have some change pending? Thanks. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D17816 To: cochise, dfaure, chinmoyr, bruns, #frameworks, tmarshall, usta Cc: usta,

D17816: Support for xattrs on kio copy/move

cochise marked an inline comment as done. cochise added inline comments. INLINE COMMENTS > usta wrote in jobtest.cpp:531 > typo :) xattRreader => xattrReader I really need to sleep, rs. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D17816 To: cochise, dfaure, chinmoyr,

D17816: Support for xattrs on kio copy/move

cochise updated this revision to Diff 78777. cochise added a comment. typo REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D17816?vs=78776=78777 BRANCH arcpatch-D17816_1 REVISION DETAIL https://phabricator.kde.org/D17816 AFFECTED FILES

D17816: Support for xattrs on kio copy/move

cochise marked 3 inline comments as done. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D17816 To: cochise, dfaure, chinmoyr, bruns, #frameworks, tmarshall, usta Cc: usta, scheirle, anthonyfieroni, tmarshall, arrowd, cfeck, bruns, phidrho, dhaumann, funkybomber, abika,

D17816: Support for xattrs on kio copy/move

cochise updated this revision to Diff 78776. cochise added a comment. Camel case REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D17816?vs=78773=78776 BRANCH arcpatch-D17816_1 REVISION DETAIL https://phabricator.kde.org/D17816 AFFECTED FILES

D17816: Support for xattrs on kio copy/move

cochise updated this revision to Diff 78773. cochise marked an inline comment as done. cochise added a comment. missed cast REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D17816?vs=78769=78773 BRANCH arcpatch-D17816_1 REVISION DETAIL

D17816: Support for xattrs on kio copy/move

cochise marked an inline comment as done. cochise added a comment. done REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D17816 To: cochise, dfaure, chinmoyr, bruns, #frameworks, tmarshall, usta Cc: usta, scheirle, anthonyfieroni, tmarshall, arrowd, cfeck, bruns, phidrho,

D17816: Support for xattrs on kio copy/move

cochise marked an inline comment as done. cochise added inline comments. INLINE COMMENTS > dfaure wrote in file_unix.cpp:179 > Ah, I see, you still need to call `strlen()` yourself, for the code at the > end of the iteration > > Well, then you might as well pass the value to the

D17816: Support for xattrs on kio copy/move

cochise updated this revision to Diff 78769. cochise added a comment. Simplify. REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D17816?vs=78767=78769 BRANCH arcpatch-D17816_1 REVISION DETAIL https://phabricator.kde.org/D17816 AFFECTED FILES

D17816: Support for xattrs on kio copy/move

cochise added inline comments. INLINE COMMENTS > dfaure wrote in file_unix.cpp:184 > You're doing two copies here. From `offset` to `key.data()`, and then from > `key.data()` -- the return value of qstrcpy -- into the QByteArray key (which > calls the QByteArray(const char*) constructor). > >

D17816: Support for xattrs on kio copy/move

cochise updated this revision to Diff 78767. cochise marked 6 inline comments as done. cochise added a comment. Move away from strcpy =] REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D17816?vs=78763=78767 BRANCH arcpatch-D17816_1 REVISION DETAIL

D17816: Support for xattrs on kio copy/move

cochise updated this revision to Diff 78763. cochise marked an inline comment as done. cochise added a comment. trailing space REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D17816?vs=78760=78763 BRANCH arcpatch-D17816_1 REVISION DETAIL

D17816: Support for xattrs on kio copy/move

cochise marked 3 inline comments as done. cochise added inline comments. INLINE COMMENTS > dfaure wrote in file_unix.cpp:183 > What's the purpose of these two lines modifying `key`, given that the next > line assigns to `key` anyway? Removed clear, as is redundant. REPOSITORY R241 KIO

D17816: Support for xattrs on kio copy/move

cochise updated this revision to Diff 78760. cochise added a comment. Using a STL-style iterator for keyList and qstrcpy and qtrncpy to get each key value. Small fixes REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D17816?vs=77962=78760 BRANCH

D17816: Support for xattrs on kio copy/move

cochise marked 9 inline comments as done. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D17816 To: cochise, dfaure, chinmoyr, bruns, #frameworks, tmarshall, usta Cc: usta, scheirle, anthonyfieroni, tmarshall, arrowd, cfeck, bruns, phidrho, dhaumann, funkybomber, abika,

D17816: Support for xattrs on kio copy/move

cochise updated this revision to Diff 77962. cochise added a comment. Tried to make a function to feed m_keyList on BSD, but the result was somewhat ugly. I don't like to use buffers if I can have typed data, but libc uses buffers, so maybe I was putting the sqare piece on the circle hole.

D17816: Support for xattrs on kio copy/move

cochise added inline comments. INLINE COMMENTS > dfaure wrote in jobtest.cpp:118 > typo in Xatr, and in "foud". I suggest: > > qWarning() << "Neither getfattr, getextattr nor xattr was found"; > > Although I have to wonder why this looks for all three, to then only use > getfattr and skip

D17816: Support for xattrs on kio copy/move

cochise updated this revision to Diff 77955. cochise marked 9 inline comments as done. cochise added a comment. Small fixes. REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D17816?vs=77804=77955 BRANCH arcpatch-D17816_1 REVISION DETAIL

D17816: Support for xattrs on kio copy/move

cochise added inline comments. INLINE COMMENTS > dfaure wrote in jobtest.cpp:638 > Was my comment ignored? Fixed on new commit. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D17816 To: cochise, dfaure, chinmoyr, bruns, #frameworks, tmarshall, usta Cc: usta, scheirle,

D17816: Support for xattrs on kio copy/move

cochise added inline comments. INLINE COMMENTS > usta wrote in jobtest.cpp:573 > but you have written it as with single T , i think it must be double ( TT ) You are right. Sorry. Fixed. =] REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D17816 To: cochise, dfaure,

D17816: Support for xattrs on kio copy/move

cochise updated this revision to Diff 77804. cochise added a comment. Refactored the tests: Plattform command configured on JobTest::initTestCase Removal duplicated code of functions [...]WithXattr Added check is filesystens supports users xattr: > Try to write xattr on

D17816: Support for xattrs on kio copy/move

cochise added a comment. In D17816#627925 , @pino wrote: > xattrs on the Linux tmpfs have been supported for many years, almost a decade. But only enough to support ACL. > The tmpfs filesystem supports extended attributes (see

D17816: Support for xattrs on kio copy/move

cochise added inline comments. INLINE COMMENTS > cochise wrote in jobtest.cpp:573 > {set,get}fattr = linux command > {set.get}extattr = BSD command > {set.get}xtattr = MacOS command > > This command could have a little more consistency... xtattr = MacOS command It uses a single command to read

D17816: Support for xattrs on kio copy/move

cochise added a comment. In D17816#627918 , @bruns wrote: > Not true in general ... please add back, and **check** if the destination FS supports XAttrs. If not, print a warning and SKIP Not true in general. But test the copies to /tmp,

D17816: Support for xattrs on kio copy/move

cochise updated this revision to Diff 77559. cochise added a comment. As @dfaure sugested, moved the attribute copy logic to a function on file_unix.cpp. As a side effect we lost hability to copy extened attributes of directories, (they are not copied, but created in CopyJob but now all

D17816: Support for xattrs on kio copy/move

cochise commandeered this revision. cochise edited reviewers, added: tmarshall; removed: cochise. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D17816 To: cochise, dfaure, chinmoyr, bruns, #frameworks, tmarshall Cc: scheirle, anthonyfieroni, tmarshall, arrowd, cfeck,

D17816: Support for xattrs on kio copy/move

cochise added a comment. Many thanks to getting this live again, @tmarshall . A year ago my vacations ended and I can't get this ready to ship. > Everything works for me if I replace the exec call with a start call. Currently the result of the exec call is thrown away as the job itself

D17816: Support for xattrs on kio copy/move

cochise added a comment. My vacancies ended and I'm making the annual planning for my classes, so I will be busy for at least two weeks, but I plan to work on the subjob bug as soon my schedule allows it. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D17816 To:

D17816: Support for xattrs on kio copy/move

cochise added a comment. Can we ship it, or using a subjob on `KIO::copy` is mandatory here? REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D17816 To: cochise, dfaure Cc: cfeck, bruns, phidrho, dhaumann, funkybomber, abika, pino, davidedmundson, ngraham, atha.kane,

D17816: Support for xattrs on kio copy/move

cochise added a comment. I'm not sure why the tests fail, and the tests failing are unrelated to xattrs. I think there is some problem in queuing the subjobs and ensuring they all have finished. After the copy job is finished, the copyLocalDirectory test can't find the file inside the

D17816: Support for xattrs on kio copy/move

cochise marked an inline comment as done. cochise added a comment. I'm not sure why the tests fail, and the tests failing are unrelated to xattrs. I think there is some problem in queuing the subjobs and ensuring they all have finished. After the copy job is finished, the

D17816: Support for xattrs on kio copy/move

cochise updated this revision to Diff 49487. cochise marked 3 inline comments as done. cochise added a comment. Use subjobs on file_copy and other small fixes Not using subjobs in KIO::copy, as it breakes the tests. REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE

D17816: Support for xattrs on kio copy/move

cochise updated this revision to Diff 48960. cochise added a comment. For some reason the added kio was not included REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D17816?vs=48959=48960 BRANCH xattr-copy-support (branched from master) REVISION DETAIL

D17816: Support for xattrs on kio copy/move

cochise updated this revision to Diff 48959. cochise marked an inline comment as done. cochise added a comment. All use cases working, added a new KIO to copy xattr Working on copy and move with or without overwrite for calls of KIO::copy, KIO::copyAs and KIO::file_copy of files and

D17816: Support for xattrs on kio copy/move

cochise added a comment. In D17816#386810 , @funkybomber wrote: > On a related note, is it possible that the "QSaveFile" is also responsible for a similar behaviour in Ark? Probably yes:

D17816: Support for xattrs on kio copy/move

cochise updated this revision to Diff 48733. cochise added a comment. small cleanup REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D17816?vs=48731=48733 BRANCH xattr-copy-support (branched from master) REVISION DETAIL https://phabricator.kde.org/D17816

D17816: Support for xattrs on kio copy/move

cochise edited the summary of this revision. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D17816 To: cochise, dfaure Cc: dhaumann, funkybomber, abika, pino, davidedmundson, ngraham, atha.kane, spoorun, nicolasfella, kde-frameworks-devel, michaelh, bruns

D17816: Support for xattrs on kio copy/move

cochise updated this revision to Diff 48731. cochise added a comment. Tests refactored, but no multiplatform. Other small fixes. The tests aren't a big mess anymore, and some groundwork for multiplatform tests are in place, but as I don't have a non linux system, I can't finish this

D17816: Support for xattrs on kio copy/move

cochise edited the summary of this revision. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D17816 To: cochise, dfaure Cc: abika, pino, davidedmundson, ngraham, atha.kane, spoorun, nicolasfella, kde-frameworks-devel, michaelh, bruns

D17816: Initial support for xattrs on kio copy/move

cochise updated this revision to Diff 48586. cochise added a comment. Tests added, includes and ifdefs reworked Initial tests. Not crossplatform of extensive yet. The includes and ifdefs were reworked, and I think they are more concise and simple, but some feedback is needed, as they

D17816: Support for xattrs on kio copy/move

cochise retitled this revision from "Initial support for xattrs on kio copy/move" to "Support for xattrs on kio copy/move". cochise edited the summary of this revision. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D17816 To: cochise, dfaure Cc: abika, pino,

D17816: Initial support for xattrs on kio copy/move

cochise added a comment. In D17816#384056 , @pino wrote: > - NULL -> nullptr > - there is not just glibc I'm following the pattern in Baloo, that keeps NULL on Mac and *BSD. I don't have any of these systems to test, so I didn't touch

D17816: Initial support for xattrs on kio copy/move

cochise updated this revision to Diff 48410. cochise marked an inline comment as done. cochise added a comment. Bug resolved in a proper way with help of Tomaz Canabrava Some other small fixes. REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE

D17816: Initial support for xattrs on kio copy/move

cochise updated this revision to Diff 48384. cochise added a comment. As using qDebug supressed the error, I changed to qCWarning and managed to track the memory corruption. On my system, after getting the list of keys on source file, the local variable holding this file path gets corrupted.

D17816: Initial support for xattrs on kio copy/move

cochise updated this revision to Diff 48380. cochise added a comment. Refactored to a function, works on all use cases. Data corruption remains Summary: Found the place to put the call for files and directories on copyjob.cpp. Reverted almost all changes in file_unix, except a fix for

D17816: Initial support for xattrs on kio copy/move

cochise added a comment. In D17816#383421 , @davidedmundson wrote: > Why do we need the code in both CopyJob and File_unix? The code was initially put in file_unix, but testing I found it won't applies to directories, so I put on

D17816: Initial support for xattrs on kio copy/move

cochise added a comment. There is one use case (copy of dirs with rename, due name conflict) where the patches not work, and I can't test on *BSD or Mac. The first problem says I would need to add a very similar snippet in a third place, i trying to find right now, (any help from

D17816: Initial support for xattrs on kio copy/move

cochise updated this revision to Diff 48255. cochise added a comment. Support for preserving xattrs on dirs, bugfix REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D17816?vs=48239=48255 BRANCH xattr-copy-support (branched from master) REVISION DETAIL

D17816: Initial support for xattrs on kio copy/move

cochise added a comment. Tested coping from e to VFAT and btrfs. Successfully copy/move to and from VFAT, losing the xattrs due to lack of support, but don't crash. Successfully copy/move to and from btrfs, preserving xattrs. REPOSITORY R241 KIO REVISION DETAIL

D17816: Initial support for xattrs on kio copy/move

cochise created this revision. Herald added a project: Frameworks. Herald added a subscriber: kde-frameworks-devel. cochise requested review of this revision. REVISION SUMMARY Working for simple file copy. Not working for directories yet. Commands for MacOS and *BSD copied from Baloo and not