D17816: Support for xattrs on kio copy/move

ngraham added a comment. Gotcha, thanks! REVISION DETAIL https://phabricator.kde.org/D17816 To: arrowd, dfaure, chinmoyr, bruns, #frameworks, tmarshall, usta, cochise Cc: kdudka, usta, scheirle, tmarshall, arrowd, cfeck, bruns, phidrho, dhaumann, funkybomber, abika, pino, davidedmundson,

D17816: Support for xattrs on kio copy/move

arrowd abandoned this revision. arrowd added a comment. Mark the revision as closed to reflect reality. REVISION DETAIL https://phabricator.kde.org/D17816 To: arrowd, dfaure, chinmoyr, bruns, #frameworks, tmarshall, usta, cochise Cc: kdudka, usta, scheirle, tmarshall, arrowd, cfeck, bruns,

D17816: Support for xattrs on kio copy/move

bruns added a comment. In D17816#677552 , @ngraham wrote: > What is the status of this? How do we move forwards? I know this has gone on a long time and we're all getting tired, but I think we can push this past the finish line without too much

D17816: Support for xattrs on kio copy/move

ngraham added a comment. What is the status of this? How do we move forwards? I know this has gone on a long time and we're all getting tired, but I think we can push this past the finish line without too much trouble, hopefully. :) REVISION DETAIL https://phabricator.kde.org/D17816 To:

D17816: Support for xattrs on kio copy/move

kdudka added a comment. I am fine with kio implementing things differently as long as the basic functionality keeps working. I used to use Krusader to get images out of my camera and it simply stopped working for me with no obvious indication of what went wrong. This code tries to

D17816: Support for xattrs on kio copy/move

bruns added a comment. In D17816#677455 , @kdudka wrote: > @bruns I find your attitude unnecessarily hostile. If you think that the code is perfect as it is, feel free to patch it case by case until it eventually works for everybody. That is

D17816: Support for xattrs on kio copy/move

kdudka added a comment. @bruns I find your attitude unnecessarily hostile. If you think that the code is perfect as it is, feel free to patch it case by case until it eventually works for everybody. That is your choice. Anyway, strace of `cp --preserve=xattr` on the same device looks

D17816: Support for xattrs on kio copy/move

bruns added a comment. In D17816#677453 , @kdudka wrote: > I do not think we have to. Please have a look at how attr_copy_fd() from is implemented: http://git.savannah.nongnu.org/cgit/attr.git/tree/libattr/attr_copy_fd.c?id=a4187bec#n111

D17816: Support for xattrs on kio copy/move

kdudka added a comment. I do not think we have to. Please have a look at how attr_copy_fd() from is implemented: http://git.savannah.nongnu.org/cgit/attr.git/tree/libattr/attr_copy_fd.c?id=a4187bec#n111 This code is quite mature and it is used by cp(1) on GNU/Linux for instance. I do

D17816: Support for xattrs on kio copy/move

bruns added a comment. In D17816#677451 , @kdudka wrote: > The man page says that one has to check return value of the second call. It does not say that the function needs to be called in a loop indefinitely until it succeeds. And if

D17816: Support for xattrs on kio copy/move

kdudka added a comment. The man page says that one has to check return value of the second call. It does not say that the function needs to be called in a loop indefinitely until it succeeds. REVISION DETAIL https://phabricator.kde.org/D17816 To: arrowd, dfaure, chinmoyr, bruns,

D17816: Support for xattrs on kio copy/move

bruns added a comment. In D17816#677449 , @kdudka wrote: > Even after applying the proposed patch, the code still looks problematic to me. I would prefer to have it explained first. When fgetxattr(..., 0) returns -1/ERANGE, what is the point

D17816: Support for xattrs on kio copy/move

kdudka added a comment. Even after applying the proposed patch, the code still looks problematic to me. I would prefer to have it explained first. When fgetxattr(..., 0) returns -1/ERANGE, what is the point of calling fgetxattr(..., 0) again? It is still going to busy-loop indefinitely

D17816: Support for xattrs on kio copy/move

arrowd added a comment. In D17816#677446 , @kdudka wrote: > I was wondering why copying files in Krusader or Dolphin from a vfat-formatted memory card stopped working for me after update. After copying the first file, the transfer stopped

D17816: Support for xattrs on kio copy/move

kdudka added a comment. I was wondering why copying files in Krusader or Dolphin from a vfat-formatted memory card stopped working for me after update. After copying the first file, the transfer stopped progressing and the process ended up consuming 100% CPU forever. Later I discovered

D17816: Support for xattrs on kio copy/move

dfaure added inline comments. INLINE COMMENTS > arrowd wrote in ConfigureChecks.cmake:12 > There are other parts of code that are guarded with `HAVE_POSIX_ACL`s, and > these aren't enabled ATM for FreeBSD. So, the change is needed and I'm > willing to implement it. > > I plan to move

D17816: Support for xattrs on kio copy/move

arrowd added inline comments. INLINE COMMENTS > dfaure wrote in ConfigureChecks.cmake:12 > I'm talking about the implementation of fileSystemSupportsACL in > kpropertiesdialog.cpp, which uses getxattr. > But now that I take another look at it, I see that it's already implemented > on FreeBSD,

D17816: Support for xattrs on kio copy/move

dfaure added inline comments. INLINE COMMENTS > arrowd wrote in ConfigureChecks.cmake:12 > > 1. enable the ACL stuff on systems with extattr, see the little bit of code > > in kpropertiesdialog.cpp > > By that you mean that I should edit the CMake module to define > `HAVE_POSIX_ACL` when

D17816: Support for xattrs on kio copy/move

arrowd added inline comments. INLINE COMMENTS > dfaure wrote in ConfigureChecks.cmake:12 > Ah, I see, OK. No extra lib needed makes it simple. > > The FindACL.cmake stuff is a bit of a mess now, with the need for extended > attributes outside the ACL related code. > Maybe this could be split

D17816: Support for xattrs on kio copy/move

ngraham added a comment. Phabricator didn't actually close the revision after I landed it because @bruns forgot to change his status to accepted. You can close this now, @arrowd. Great work! REVISION DETAIL https://phabricator.kde.org/D17816 To: arrowd, dfaure, chinmoyr, bruns,

D17816: Support for xattrs on kio copy/move

ngraham added a comment. After almost two years, I'm so happy to see this land! REVISION DETAIL https://phabricator.kde.org/D17816 To: arrowd, dfaure, chinmoyr, bruns, #frameworks, tmarshall, usta, cochise Cc: usta, scheirle, tmarshall, arrowd, cfeck, bruns, phidrho, dhaumann,

D17816: Support for xattrs on kio copy/move

dfaure accepted this revision. dfaure added inline comments. INLINE COMMENTS > arrowd wrote in ConfigureChecks.cmake:12 > > Has this been tested on a system with sys/extattr.h? > > I was working on this revision on such system all the time. FreeBSD contains > extattr bits in its `libc`, so no

D17816: Support for xattrs on kio copy/move

arrowd added inline comments. INLINE COMMENTS > dfaure wrote in ConfigureChecks.cmake:12 > This is already done by `cmake/FindACL.cmake`, why not add the other one > (extattr.h) to that file as well? > > The CMakeLists.txt in this directory calls `find_package(ACL)` so it'll be > used. > >

D17816: Support for xattrs on kio copy/move

arrowd updated this revision to Diff 83375. arrowd marked an inline comment as done. arrowd added a comment. - Put FileProtocol::copyXattrs inside a #if CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D17816?vs=83366=83375 BRANCH arcpatch-D17816 REVISION DETAIL

D17816: Support for xattrs on kio copy/move

dfaure added inline comments. INLINE COMMENTS > ConfigureChecks.cmake:12 > check_include_files(limits.h HAVE_LIMITS_H) > +check_include_files(sys/xattr.h HAVE_SYS_XATTR_H) > +check_include_files("sys/types.h;sys/extattr.h" HAVE_SYS_EXTATTR_H) This is already done by

D17816: Support for xattrs on kio copy/move

ngraham added a comment. Can you change your status to approved? @dfaure, one final look maybe? REVISION DETAIL https://phabricator.kde.org/D17816 To: arrowd, dfaure, chinmoyr, bruns, #frameworks, tmarshall, usta, cochise Cc: usta, scheirle, tmarshall, arrowd, cfeck, bruns, phidrho,

D17816: Support for xattrs on kio copy/move

bruns added a comment. Looks ok to me now. Can someone else please double check? REVISION DETAIL https://phabricator.kde.org/D17816 To: arrowd, dfaure, chinmoyr, bruns, #frameworks, tmarshall, usta, cochise Cc: usta, scheirle, tmarshall, arrowd, cfeck, bruns, phidrho, dhaumann,

D17816: Support for xattrs on kio copy/move

ngraham added a comment. @bruns? REVISION DETAIL https://phabricator.kde.org/D17816 To: arrowd, dfaure, chinmoyr, bruns, #frameworks, tmarshall, usta, cochise Cc: usta, scheirle, tmarshall, arrowd, cfeck, bruns, phidrho, dhaumann, funkybomber, abika, pino, davidedmundson, ngraham,

D17816: Support for xattrs on kio copy/move

arrowd added a comment. Another bump. REVISION DETAIL https://phabricator.kde.org/D17816 To: arrowd, dfaure, chinmoyr, bruns, #frameworks, tmarshall, usta, cochise Cc: usta, scheirle, tmarshall, arrowd, cfeck, bruns, phidrho, dhaumann, funkybomber, abika, pino, davidedmundson, ngraham,

D17816: Support for xattrs on kio copy/move

arrowd updated this revision to Diff 83366. arrowd marked 5 inline comments as done. arrowd added a comment. - Address comments. CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D17816?vs=83365=83366 BRANCH arcpatch-D17816 REVISION DETAIL https://phabricator.kde.org/D17816

D17816: Support for xattrs on kio copy/move

bruns requested changes to this revision. bruns added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > jobtest.cpp:471 > + > +void JobTest::setXattr(const QString ) > +{ should be `dest`. > jobtest.cpp:489 > +qDebug() << resultdest; > +

D17816: Support for xattrs on kio copy/move

arrowd added inline comments. INLINE COMMENTS > usta wrote in file_unix.cpp:1517 > isnt this ignoring acl_from_mode part ? I mean not sure but i think we need > to check if it is nullptr or not before assigning it otherwise we will ignore > the acl_from_mode part. You seem to be right. I

D17816: Support for xattrs on kio copy/move

arrowd updated this revision to Diff 83365. arrowd marked an inline comment as done. arrowd added a comment. - More const QString. CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D17816?vs=83364=83365 BRANCH arcpatch-D17816 REVISION DETAIL https://phabricator.kde.org/D17816

D17816: Support for xattrs on kio copy/move

arrowd added inline comments. INLINE COMMENTS > bruns wrote in jobtest.cpp:780 > And now you **only** check the source FS, but no longer the destination FS. To be honest, I was confused by your first comment. What was wrong with the first version of this code? My understanding was that we

D17816: Support for xattrs on kio copy/move

bruns added inline comments. INLINE COMMENTS > bruns wrote in jobtest.cpp:780 > This will set `m_SkipXattr` unconditionally even if the source FS does not > support Xattrs. And now you **only** check the source FS, but no longer the destination FS. REVISION DETAIL

D17816: Support for xattrs on kio copy/move

usta added inline comments. INLINE COMMENTS > jobtest.cpp:465 > +{ > +QString writeTest = dir + "/fsXattrTestFile"; > +createTestFile(writeTest); const ? > file_unix.cpp:1517 > } > acl = acl_from_text(ACLString.toLatin1().constData()); > if (acl_valid(acl) ==

D17816: Support for xattrs on kio copy/move

arrowd updated this revision to Diff 83364. arrowd marked 2 inline comments as done. arrowd added a comment. - Make checkXattrFsSupport accept directory as a parameter. - Consitify QStrings. CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D17816?vs=83361=83364 BRANCH

D17816: Support for xattrs on kio copy/move

bruns added inline comments. INLINE COMMENTS > jobtest.cpp:467 > +path.removeLast(); > +QString writeTest = path.join("/") + "/fsXattrTestFile"; > +createTestFile(writeTest); const QString And if you just call this with the target directory (e.g. ` homeTmpDir()`), you can skip the

D17816: Support for xattrs on kio copy/move

arrowd updated this revision to Diff 83361. arrowd marked an inline comment as done. arrowd added a comment. - Return "true" if the file doesn't have any xattrs. CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D17816?vs=83334=83361 BRANCH arcpatch-D17816 REVISION DETAIL

D17816: Support for xattrs on kio copy/move

bruns added inline comments. INLINE COMMENTS > file_unix.cpp:583 > +qCDebug(KIO_FILE) << "the file don't have any xattr"; > +return false; > +} Should be `true` REVISION DETAIL https://phabricator.kde.org/D17816 To: arrowd, dfaure, chinmoyr, bruns,

D17816: Support for xattrs on kio copy/move

ngraham added a comment. Good now, @bruns? REVISION DETAIL https://phabricator.kde.org/D17816 To: arrowd, dfaure, chinmoyr, bruns, #frameworks, tmarshall, usta, cochise Cc: usta, scheirle, tmarshall, arrowd, cfeck, bruns, phidrho, dhaumann, funkybomber, abika, pino, davidedmundson,

D17816: Support for xattrs on kio copy/move

arrowd added a comment. Bump. REVISION DETAIL https://phabricator.kde.org/D17816 To: arrowd, dfaure, chinmoyr, bruns, #frameworks, tmarshall, usta, cochise Cc: usta, scheirle, tmarshall, arrowd, cfeck, bruns, phidrho, dhaumann, funkybomber, abika, pino, davidedmundson, ngraham, atha.kane,

D17816: Support for xattrs on kio copy/move

arrowd updated this revision to Diff 83334. arrowd added a comment. Rebase on master. CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D17816?vs=83319=83334 BRANCH arcpatch-D17816 REVISION DETAIL https://phabricator.kde.org/D17816 AFFECTED FILES autotests/jobtest.cpp

D17816: Support for xattrs on kio copy/move

arrowd added inline comments. INLINE COMMENTS > bruns wrote in jobtest.cpp:487 > not done ... There are no arrays of `512` size anymore. Or am I missing something? REVISION DETAIL https://phabricator.kde.org/D17816 To: arrowd, dfaure, chinmoyr, bruns, #frameworks, tmarshall, usta, cochise

D17816: Support for xattrs on kio copy/move

arrowd updated this revision to Diff 83319. arrowd marked an inline comment as done. arrowd added a comment. - Remove extraneous debugging output. CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D17816?vs=83318=83319 BRANCH arcpatch-D17816 REVISION DETAIL

D17816: Support for xattrs on kio copy/move

bruns added inline comments. INLINE COMMENTS > bruns wrote in jobtest.cpp:487 > this obviously needs test cases with a key ley/value len > 512, to check the > the array-to-short/resize path. not done ... > file_unix.cpp:620 > +#endif > +qDebug(KIO_FILE) << valuelen << " " << key

D17816: Support for xattrs on kio copy/move

arrowd updated this revision to Diff 83318. arrowd marked 3 inline comments as done. arrowd added a comment. - Handle attrs with empty values. - Add test for it. - Fix syscalls for FreeBSD case. CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D17816?vs=83309=83318 BRANCH

D17816: Support for xattrs on kio copy/move

bruns added inline comments. INLINE COMMENTS > arrowd wrote in 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.

D17816: Support for xattrs on kio copy/move

arrowd added inline comments. INLINE COMMENTS > bruns wrote in file_unix.cpp:625 > Infinite loop on valuelen == 0 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

D17816: Support for xattrs on kio copy/move

arrowd updated this revision to Diff 83309. arrowd marked an inline comment as done. arrowd added a comment. - Improve comment. CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D17816?vs=83302=83309 BRANCH arcpatch-D17816 REVISION DETAIL https://phabricator.kde.org/D17816

D17816: Support for xattrs on kio copy/move

bruns added inline comments. INLINE COMMENTS > file_unix.cpp:580 > +if (listlen == 0) { > +qCDebug(KIO_FILE) << "file" << src_fd << "don't have any xattr"; > +return false; `src_fd` is quite meaningless as debug output > file_unix.cpp:625 > +

D17816: Support for xattrs on kio copy/move

ngraham added a comment. @bruns? REVISION DETAIL https://phabricator.kde.org/D17816 To: arrowd, dfaure, chinmoyr, bruns, #frameworks, tmarshall, usta, cochise Cc: usta, scheirle, tmarshall, arrowd, cfeck, bruns, phidrho, dhaumann, funkybomber, abika, pino, davidedmundson, ngraham,

D17816: Support for xattrs on kio copy/move

arrowd updated this revision to Diff 83302. arrowd added a comment. - Rebase on master. CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D17816?vs=83207=83302 BRANCH arcpatch-D17816 REVISION DETAIL https://phabricator.kde.org/D17816 AFFECTED FILES autotests/jobtest.cpp

D17816: Support for xattrs on kio copy/move

arrowd added a comment. I'd appreciate if someone test this on Linux and MacOS. I got FreeBSD covered. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D17816 To: arrowd, dfaure, chinmoyr, bruns, #frameworks, tmarshall, usta, cochise Cc: usta, scheirle, tmarshall, arrowd,

D17816: Support for xattrs on kio copy/move

arrowd updated this revision to Diff 83207. arrowd marked 8 inline comments as done. arrowd added a comment. - Address comments. REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D17816?vs=83203=83207 BRANCH arcpatch-D17816 REVISION DETAIL

D17816: Support for xattrs on kio copy/move

bruns added inline comments. INLINE COMMENTS > file_unix.cpp:142 > +ssize_t listlen = 0; > +QByteArray keylist(listlen, Qt::Uninitialized); > +do { just `QByteArray keylist;` > file_unix.cpp:143 > +QByteArray keylist(listlen, Qt::Uninitialized); > +do { > +

D17816: Support for xattrs on kio copy/move

arrowd added a comment. Almost every comment have been addressed. Please, give this another look. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D17816 To: arrowd, dfaure, chinmoyr, bruns, #frameworks, tmarshall, usta, cochise Cc: usta, scheirle, tmarshall, arrowd,

D17816: Support for xattrs on kio copy/move

arrowd updated this revision to Diff 83203. arrowd marked 17 inline comments as done. arrowd added a comment. - Address comments. REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D17816?vs=83192=83203 BRANCH arcpatch-D17816 REVISION DETAIL

D17816: Support for xattrs on kio copy/move

bruns requested changes to this revision. bruns added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > file_unix.cpp:206 > +if (valuelen == -1 && errno == ERANGE) { > +value.resize(valuelen + 512); > +} WRONG WRONG WRONG

D17816: Support for xattrs on kio copy/move

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

D17816: Support for xattrs on kio copy/move

arrowd updated this revision to Diff 83192. arrowd marked 2 inline comments as done. arrowd added a comment. - Change the algorithm in FileProtocol::copyXattrs() REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D17816?vs=83166=83192 BRANCH arcpatch-D17816

D17816: Support for xattrs on kio copy/move

bruns requested changes to this revision. bruns added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > jobtest.cpp:487 > +attrs["user.fattr.with.a.lot.of.namespaces"] = "true"; > +attrs["user.name with space"] = "value with spaces"; > +return attrs;

D17816: Support for xattrs on kio copy/move

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

D17816: Support for xattrs on kio copy/move

arrowd updated this revision to Diff 83166. arrowd marked an inline comment as done. arrowd added a comment. - Use std::function to store a generator function for m_setXattrCmd's arguments. REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D17816?vs=83143=83166

D17816: Support for xattrs on kio copy/move

bruns added inline comments. INLINE COMMENTS > jobtest.cpp:502 > +// arguments 0:"-n" 1:"name" 2:"-v", 3:"value" 4:src > +arguments = QStringList {"-n", "", "-v", "", "-h", src}; > +keyIndex = 1; `std::function m_setXattrFormatArgs;` ... m_setXattrFormatArgs =

D17816: Support for xattrs on kio copy/move

arrowd updated this revision to Diff 83143. arrowd marked 5 inline comments as done. arrowd added a comment. - Refactor common code from compareXattr() into readXattr() as requested in comments. REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE

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

arrowd updated this revision to Diff 83068. arrowd marked 18 inline comments as done. arrowd added a comment. - Use full paths to command line utilities and pass xattr args correctly. - Mark some stale comments as done. REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE

D17816: Support for xattrs on kio copy/move

arrowd updated this revision to Diff 83016. arrowd added a comment. - Fix tests on FreeBSD. - Fix bug: Move `keylist.squeeze()` call before acquiring an interator. At this stage, all tests in `jobtest` pass on FreeBSD. REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE

D17816: Support for xattrs on kio copy/move

arrowd updated this revision to Diff 82960. arrowd added a comment. I decided to help with this a bit. - Fix detection of header. It requires 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

D17816: Support for xattrs on kio copy/move

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

D17816: Support for xattrs on kio copy/move

ngraham added a comment. Ping @cochise 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

bruns requested changes to this revision. bruns added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > file_unix.cpp:141 > +// Get the list of keys > +ssize_t listlen = 512; > +QByteArray keylist(listlen, Qt::Uninitialized); The idea is almost right,

D17816: Support for xattrs on kio copy/move

D17816: Support for xattrs on kio copy/move

bruns added inline comments. INLINE COMMENTS > file_unix.cpp:149 > +if (listlen == -1) { > +qCWarning(KIO_FILE) << "libc failed to extract list of xattr from > file"; > +return false; Now try this when the source is e.g. FAT formatted ... Make this dependent on errno.

D17816: Support for xattrs on kio copy/move

bruns added a comment. In D17816#639272 , @usta wrote: > is there a KDE policy about usage of qCWarning( , qCDebug( ? > Because afaik debug one can be ignored by system and some of those might be important to show to user to see > So for

D17816: Support for xattrs on kio copy/move

bruns added a comment. There are still several comments which have not yet been addressed. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D17816 To: cochise, dfaure, chinmoyr, bruns, #frameworks, tmarshall, usta Cc: usta, scheirle, anthonyfieroni, tmarshall, arrowd,

D17816: Support for xattrs on kio copy/move

ngraham added a comment. @bruns? 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, pino,

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

usta accepted this revision. 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, pino,

D17816: Support for xattrs on kio copy/move

ngraham added a comment. In D17816#639272 , @usta wrote: > is there a KDE policy about usage of qCWarning( , qCDebug( ? > Because afaik debug one can be ignored by system and some of those might be important to show to user to see > So for

D17816: Support for xattrs on kio copy/move

usta added a comment. is there a KDE policy about usage of qCWarning( , qCDebug( ? Because afaik debug one can be ignored by system and some of those might be important to show to user to see So for example this one looks like a warning instead of a debug info for me ( but not sure what

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

dfaure accepted this revision. dfaure added a comment. Thanks. I agree :-) INLINE COMMENTS > bruns wrote in file_unix.cpp:232 > what about `#elif defined(Q_OS_MAC)`? @bruns HAVE_SYS_XATTR_H covers mac and non-mac here. REPOSITORY R241 KIO REVISION DETAIL

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

usta requested changes to this revision. usta added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > jobtest.cpp:531 > + > +QProcess xattRreader; > +xattRreader.setProcessChannelMode(QProcess::MergedChannels); typo :) xattRreader => xattrReader

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

usta requested changes to this revision. usta added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > jobtest.cpp:500 > + > +QProcess xattrwriter; > +xattrwriter.setProcessChannelMode(QProcess::MergedChannels); aren't we using camelcase rules for

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

bruns added inline comments. INLINE COMMENTS > jobtest.cpp:105 > + / > +m_getXattrCmd = > QStandardPaths::findExecutable("getfattr").split("/").last(); > +if (m_getXattrCmd == "getfattr") { Whats the reason to split off the path? Also, you can not just concatenate the command

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

dfaure added inline comments. INLINE COMMENTS > file_unix.cpp:179 > +keyLen = strlen(keyPtr); > +QByteArray key(keyPtr); > +#elif HAVE_SYS_EXTATTR_H 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

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

dfaure added a comment. Almost there ;) INLINE COMMENTS > file_unix.cpp:140 > +{ > +// Get the size of the list of keys from soure file > +#if HAVE_SYS_XATTR_H && !defined(__stub_getxattr) && !defined(Q_OS_MAC) typo: soure => source > file_unix.cpp:153 > +if (listlen == 0) { > +

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

  1   2   >