D17816: Support for xattrs on kio copy/move

[Cochise César]

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 , @arrowd wrote:
  
  > At this stage, all tests in `jobtest` pass on FreeBSD.
  
  
  I'm very happy to see this working well on BSD as well.

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, davidedmundson, ngraham, atha.kane, spoorun, 
nicolasfella, kde-frameworks-devel, LeGast00n, cblack, michaelh

D17816: Support for xattrs on kio copy/move

[Cochise César]

D17816: Support for xattrs on kio copy/move

[Cochise César]

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
  autotests/jobtest.cpp
  autotests/jobtest.h
  src/ioslaves/file/ConfigureChecks.cmake
  src/ioslaves/file/file.h
  src/ioslaves/file/file_unix.cpp

To: cochise, dfaure, chinmoyr, bruns, #frameworks, tmarshall, usta
Cc: usta, scheirle, anthonyfieroni, tmarshall, arrowd, cfeck, bruns, phidrho, 
dhaumann, funkybomber, abika, pino, davidedmundson, ngraham, atha.kane, 
spoorun, nicolasfella, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh

D17816: Support for xattrs on kio copy/move

[Cochise César]

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, scheirle, anthonyfieroni, tmarshall, arrowd, cfeck, bruns, phidrho, 
dhaumann, funkybomber, abika, pino, davidedmundson, ngraham, atha.kane, 
spoorun, nicolasfella, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh

D17816: Support for xattrs on kio copy/move

[Cochise César]

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, bruns, #frameworks, tmarshall, usta
Cc: usta, scheirle, anthonyfieroni, tmarshall, arrowd, cfeck, bruns, phidrho, 
dhaumann, funkybomber, abika, pino, davidedmundson, ngraham, atha.kane, 
spoorun, nicolasfella, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh

D17816: Support for xattrs on kio copy/move

[Cochise César]

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
  autotests/jobtest.cpp
  autotests/jobtest.h
  src/ioslaves/file/ConfigureChecks.cmake
  src/ioslaves/file/file.h
  src/ioslaves/file/file_unix.cpp

To: cochise, dfaure, chinmoyr, bruns, #frameworks, tmarshall, usta
Cc: usta, scheirle, anthonyfieroni, tmarshall, arrowd, cfeck, bruns, phidrho, 
dhaumann, funkybomber, abika, pino, davidedmundson, ngraham, atha.kane, 
spoorun, nicolasfella, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh

D17816: Support for xattrs on kio copy/move

[Cochise César]

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, pino, davidedmundson, ngraham, atha.kane, 
spoorun, nicolasfella, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh

D17816: Support for xattrs on kio copy/move

[Cochise César]

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
  autotests/jobtest.cpp
  autotests/jobtest.h
  src/ioslaves/file/ConfigureChecks.cmake
  src/ioslaves/file/file.h
  src/ioslaves/file/file_unix.cpp

To: cochise, dfaure, chinmoyr, bruns, #frameworks, tmarshall, usta
Cc: usta, scheirle, anthonyfieroni, tmarshall, arrowd, cfeck, bruns, phidrho, 
dhaumann, funkybomber, abika, pino, davidedmundson, ngraham, atha.kane, 
spoorun, nicolasfella, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh

D17816: Support for xattrs on kio copy/move

[Cochise César]

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
  https://phabricator.kde.org/D17816

AFFECTED FILES
  autotests/jobtest.cpp
  autotests/jobtest.h
  src/ioslaves/file/ConfigureChecks.cmake
  src/ioslaves/file/file.h
  src/ioslaves/file/file_unix.cpp

To: cochise, dfaure, chinmoyr, bruns, #frameworks, tmarshall, usta
Cc: usta, scheirle, anthonyfieroni, tmarshall, arrowd, cfeck, bruns, phidrho, 
dhaumann, funkybomber, abika, pino, davidedmundson, ngraham, atha.kane, 
spoorun, nicolasfella, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh

D17816: Support for xattrs on kio copy/move

[Cochise César]

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, 
dhaumann, funkybomber, abika, pino, davidedmundson, ngraham, atha.kane, 
spoorun, nicolasfella, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh

D17816: Support for xattrs on kio copy/move

[Cochise César]

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 constructor so it doesn't 
> need to do the same.
> 
>   const QByteArray key(keyPtr, keyLen);
> 
> after the #endif, since it would now be the exact same line of code for both 
> cases.

This look really better.

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, davidedmundson, ngraham, atha.kane, 
spoorun, nicolasfella, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh

D17816: Support for xattrs on kio copy/move

[Cochise César]

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
  autotests/jobtest.cpp
  autotests/jobtest.h
  src/ioslaves/file/ConfigureChecks.cmake
  src/ioslaves/file/file.h
  src/ioslaves/file/file_unix.cpp

To: cochise, dfaure, chinmoyr, bruns, #frameworks, tmarshall, usta
Cc: usta, scheirle, anthonyfieroni, tmarshall, arrowd, cfeck, bruns, phidrho, 
dhaumann, funkybomber, abika, pino, davidedmundson, ngraham, atha.kane, 
spoorun, nicolasfella, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh

D17816: Support for xattrs on kio copy/move

[Cochise César]

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).
> 
> This can be simplified.
> Option 1: just remove the assignment, just do the qstrcpy. But it still 
> smells like C to me. And there's a security bug if keyLen is ever too small.
> Option 2: QByteArray key(offset); So simple. No need for strlen before, no 
> need for qstrcpy, it all happens internally in that constructor.

So many time looking for a way to get a copy until a null, and it's in one of 
the constructors.
This looks a lot better.

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, davidedmundson, ngraham, atha.kane, 
spoorun, nicolasfella, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh

D17816: Support for xattrs on kio copy/move

[Cochise César]

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
  https://phabricator.kde.org/D17816

AFFECTED FILES
  autotests/jobtest.cpp
  autotests/jobtest.h
  src/ioslaves/file/ConfigureChecks.cmake
  src/ioslaves/file/file.h
  src/ioslaves/file/file_unix.cpp

To: cochise, dfaure, chinmoyr, bruns, #frameworks, tmarshall, usta
Cc: usta, scheirle, anthonyfieroni, tmarshall, arrowd, cfeck, bruns, phidrho, 
dhaumann, funkybomber, abika, pino, davidedmundson, ngraham, atha.kane, 
spoorun, nicolasfella, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh

D17816: Support for xattrs on kio copy/move

[Cochise César]

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
  https://phabricator.kde.org/D17816

AFFECTED FILES
  autotests/jobtest.cpp
  autotests/jobtest.h
  src/ioslaves/file/ConfigureChecks.cmake
  src/ioslaves/file/file.h
  src/ioslaves/file/file_unix.cpp

To: cochise, dfaure, chinmoyr, bruns, #frameworks, tmarshall, usta
Cc: usta, scheirle, anthonyfieroni, tmarshall, arrowd, cfeck, bruns, phidrho, 
dhaumann, funkybomber, abika, pino, davidedmundson, ngraham, atha.kane, 
spoorun, nicolasfella, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh

D17816: Support for xattrs on kio copy/move

[Cochise César]

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

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, davidedmundson, ngraham, atha.kane, 
spoorun, nicolasfella, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh

D17816: Support for xattrs on kio copy/move

[Cochise César]

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
  arcpatch-D17816_1

REVISION DETAIL
  https://phabricator.kde.org/D17816

AFFECTED FILES
  autotests/jobtest.cpp
  autotests/jobtest.h
  src/ioslaves/file/ConfigureChecks.cmake
  src/ioslaves/file/file.h
  src/ioslaves/file/file_unix.cpp

To: cochise, dfaure, chinmoyr, bruns, #frameworks, tmarshall, usta
Cc: usta, scheirle, anthonyfieroni, tmarshall, arrowd, cfeck, bruns, phidrho, 
dhaumann, funkybomber, abika, pino, davidedmundson, ngraham, atha.kane, 
spoorun, nicolasfella, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh

D17816: Support for xattrs on kio copy/move

[Cochise César]

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, pino, davidedmundson, ngraham, atha.kane, 
spoorun, nicolasfella, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh

D17816: Support for xattrs on kio copy/move

[Cochise César]

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.
  So, accepted @bruns suggestion about the loop.
  
  On a side note, I think the mac ifdef was never getting true, as 
HAVE_SYS_XATTR should return true on mac, so changed from:
  
#if HAVE_SYS_XATTR_H && !defined(__stub_getxattr)
LINUX CODE
#elif defined(Q_OS_MAC)
MACOS CODE
  
  to
  
#if HAVE_SYS_XATTR_H && !defined(__stub_getxattr) && !defined(Q_OS_MAC)
LINUX CODE
#elif defined(Q_OS_MAC)
MACOS CODE

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17816?vs=77955=77962

BRANCH
  arcpatch-D17816_1

REVISION DETAIL
  https://phabricator.kde.org/D17816

AFFECTED FILES
  autotests/jobtest.cpp
  autotests/jobtest.h
  src/ioslaves/file/ConfigureChecks.cmake
  src/ioslaves/file/file.h
  src/ioslaves/file/file_unix.cpp

To: cochise, dfaure, chinmoyr, bruns, #frameworks, tmarshall, usta
Cc: usta, scheirle, anthonyfieroni, tmarshall, arrowd, cfeck, bruns, phidrho, 
dhaumann, funkybomber, abika, pino, davidedmundson, ngraham, atha.kane, 
spoorun, nicolasfella, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh

D17816: Support for xattrs on kio copy/move

[Cochise César]

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 tests in all other cases

The check is to build infrastructure for anyone with access to the platforms 
that want to add their test.
Not sure if this would ever happens, but it's here.

> dfaure wrote in file_unix.cpp:164
> Can this *ever* return an empty list, because keylist was empty?
> 
> (Then last() will assert on the next line)

Not because if the file don't have a xattr the function returns on 153. The 
returned keylist should have at least one key.
Removing last, if empy is done because in my tests the keylist includes a empty 
attribute that is preserved, and appended after each copy.

  if (listlen == 0) {
  qCDebug(KIO_FILE) << "file " << src_fd << " don't have any xattr";
  return false;
  }

> bruns wrote in file_unix.cpp:164
> Why a temporary list at all?
> 
>   off_t offset = 0; size_t keyLen;
>   while (offset < keylist.size()) {
>   #if BSD
>  keyLen = static_cast(keylist[offset]);
>  offset++;
>   #elif LINUX
>   keyLen = strlen(keylist[offset]
>   #endif
>   key = keylist.mid(offset, keyLen);
>   /* copy */
>   ...
>   #if BSD
>   offset += keyLen;
>   #elif LINUX
>   offset += keyLen + 1;
>   #endif
>   }

As the system functions need individual keys to get values and a individual key 
value pair to set, the list is to make more easy to iterate over keys. We can 
iterate over the buffer, reading until '\0', but this is a little more bug 
prone.

> bruns wrote in file_unix.cpp:164
> This is wrong for the BSD implementation:
> 
> > extattr_list_file() returns a list  of attributes present in the requested
> >  namespace.  Each list entry consists of a **single byte containing the
> >  length of the attribute name**, followed by the attribute name.  The   
> > attri-
> >  bute name is **not terminated by ASCII 0 (nul).**

Well... This will need some platform specif code to read the buffer and place 
in the m_keyList, ad the .split() will not work. Using the list is mandatory 
now, as we have different format buffers.
Committing other fixes, ad they are really small and postponed this one.

> bruns wrote in file_unix.cpp:180
> Allocate outside the loop

The value change on every pass and we only get the size inside the loop. 
Allocating outside the loop is more performant than resizing for each pass?

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, davidedmundson, ngraham, atha.kane, 
spoorun, nicolasfella, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh

D17816: Support for xattrs on kio copy/move

[Cochise César]

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
  https://phabricator.kde.org/D17816

AFFECTED FILES
  autotests/jobtest.cpp
  autotests/jobtest.h
  src/ioslaves/file/ConfigureChecks.cmake
  src/ioslaves/file/file.h
  src/ioslaves/file/file_unix.cpp

To: cochise, dfaure, chinmoyr, bruns, #frameworks, tmarshall, usta
Cc: usta, scheirle, anthonyfieroni, tmarshall, arrowd, cfeck, bruns, phidrho, 
dhaumann, funkybomber, abika, pino, davidedmundson, ngraham, atha.kane, 
spoorun, nicolasfella, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh

D17816: Support for xattrs on kio copy/move

[Cochise César]

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, anthonyfieroni, tmarshall, arrowd, cfeck, bruns, phidrho, 
dhaumann, funkybomber, abika, pino, davidedmundson, ngraham, atha.kane, 
spoorun, nicolasfella, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh

D17816: Support for xattrs on kio copy/move

[Cochise César]

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, chinmoyr, bruns, #frameworks, tmarshall, usta
Cc: usta, scheirle, anthonyfieroni, tmarshall, arrowd, cfeck, bruns, phidrho, 
dhaumann, funkybomber, abika, pino, davidedmundson, ngraham, atha.kane, 
spoorun, nicolasfella, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh

D17816: Support for xattrs on kio copy/move

[Cochise César]

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 source filesystem. If not able sets to skip.
  >  If destination is in another partition, repeat on dest.
  >  As we don't wanna skip the whole copy test, skipping is done by not 
writing xattr on source file.
  
  Cleanup.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17816?vs=77559=77804

BRANCH
  arcpatch-D17816_1

REVISION DETAIL
  https://phabricator.kde.org/D17816

AFFECTED FILES
  autotests/jobtest.cpp
  autotests/jobtest.h
  src/ioslaves/file/ConfigureChecks.cmake
  src/ioslaves/file/file.h
  src/ioslaves/file/file_unix.cpp

To: cochise, dfaure, chinmoyr, bruns, #frameworks, tmarshall, usta
Cc: usta, scheirle, anthonyfieroni, tmarshall, arrowd, cfeck, bruns, phidrho, 
dhaumann, funkybomber, abika, pino, davidedmundson, ngraham, atha.kane, 
spoorun, nicolasfella, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh

D17816: Support for xattrs on kio copy/move

[Cochise César]

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 xattr(7)), but user 
extended attributes are not permitted.
  
  http://man7.org/linux/man-pages/man5/tmpfs.5.html
  
  > tmpfs can support extended attributes if you enable CONFIG_TMPFS_XATTR in 
Kernel config. But Currently this enables support for the trusted.* and 
security.* namespaces.
  
  
https://stackoverflow.com/questions/30760177/xattr-extended-attributes-not-settable-for-file-in-tmp-while-in-home-on-same-m

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D17816

To: cochise, dfaure, chinmoyr, bruns, #frameworks, tmarshall
Cc: usta, scheirle, anthonyfieroni, tmarshall, arrowd, cfeck, bruns, phidrho, 
dhaumann, funkybomber, abika, pino, davidedmundson, ngraham, atha.kane, 
spoorun, nicolasfella, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh

D17816: Support for xattrs on kio copy/move

[Cochise César]

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 and write.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D17816

To: cochise, dfaure, chinmoyr, bruns, #frameworks, tmarshall
Cc: usta, scheirle, anthonyfieroni, tmarshall, arrowd, cfeck, bruns, phidrho, 
dhaumann, funkybomber, abika, pino, davidedmundson, ngraham, atha.kane, 
spoorun, nicolasfella, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh

D17816: Support for xattrs on kio copy/move

[Cochise César]

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, and on most distros today 
it's a tmpfs, that don't support xattr. But checking support is a good idea.

INLINE COMMENTS

> usta wrote in jobtest.cpp:573
> I am not sure but it looks like there is a typo here
> getextatr --->
> getextattr
> but not sure , can someone make a comment about this ?

{set,get}fattr = linux command
{set.get}extattr = BSD command
{set.get}xtattr = MacOS command

This command could have a little more consistency...

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D17816

To: cochise, dfaure, chinmoyr, bruns, #frameworks, tmarshall
Cc: usta, scheirle, anthonyfieroni, tmarshall, arrowd, cfeck, bruns, phidrho, 
dhaumann, funkybomber, abika, pino, davidedmundson, ngraham, atha.kane, 
spoorun, nicolasfella, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh

D17816: Support for xattrs on kio copy/move

[Cochise César]

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 tests pass.
  
  Minor changes:
  
  - Added tests for links, as the code works with them
  
  - Removed tests for copy/move to another partition, as this shpould fail. The 
destination don't support xattrs.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17816?vs=72144=77559

BRANCH
  arcpatch-D17816

REVISION DETAIL
  https://phabricator.kde.org/D17816

AFFECTED FILES
  autotests/jobtest.cpp
  autotests/jobtest.h
  src/ioslaves/file/ConfigureChecks.cmake
  src/ioslaves/file/file.h
  src/ioslaves/file/file_unix.cpp

To: cochise, dfaure, chinmoyr, bruns, #frameworks, tmarshall
Cc: scheirle, anthonyfieroni, tmarshall, arrowd, cfeck, bruns, phidrho, 
dhaumann, funkybomber, abika, pino, davidedmundson, ngraham, atha.kane, 
spoorun, nicolasfella, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh

D17816: Support for xattrs on kio copy/move

[Cochise César]

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, bruns, phidrho, 
dhaumann, funkybomber, abika, pino, davidedmundson, ngraham, atha.kane, 
spoorun, nicolasfella, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh

D17816: Support for xattrs on kio copy/move

[Cochise César]

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 does the 
error handling and there isn't much to do if the xattrs can't be copied for 
whatever reason.
  
  If I recall correctly, `start()` and `exec()` works to me on manual tests, 
but `start()` broke the automated tests. As the maintainer vetoed a sync 
solution, but the async one cant pass the tests, the patch stalled.
  
  If all tests are passing to you with a async call, I think it's shipabble.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D17816

To: tmarshall, dfaure, chinmoyr, bruns, #frameworks, cochise
Cc: tmarshall, arrowd, cfeck, bruns, phidrho, dhaumann, funkybomber, abika, 
pino, davidedmundson, ngraham, atha.kane, spoorun, nicolasfella, 
kde-frameworks-devel, LeGast00n, GB_2, michaelh

D17816: Support for xattrs on kio copy/move

[Cochise César]

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: cochise, dfaure
Cc: cfeck, bruns, phidrho, dhaumann, funkybomber, abika, pino, davidedmundson, 
ngraham, atha.kane, spoorun, nicolasfella, kde-frameworks-devel, michaelh

D17816: Support for xattrs on kio copy/move

[Cochise César]

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, spoorun, nicolasfella, kde-frameworks-devel, michaelh

D17816: Support for xattrs on kio copy/move

[Cochise César]

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 copied test directory. Manually testing recursive copy I can't 
find problems, but the autotest fail. So, I think some parts of the copy job 
are executed after the copyjob finishes if I add a subjob.
  As I said, we can use start, instead of exec, to be asynchronous, but using a 
subjob seems to need some refactor, maybe adding a new state for the xattr 
subjob.

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, spoorun, nicolasfella, kde-frameworks-devel, michaelh

D17816: Support for xattrs on kio copy/move

[Cochise César]

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 `copyLocalDirectory` test can't find the 
file inside the copied test directory. Manually testing recursive copy I can't 
find problems, but the autotest fail. So, I think some parts of the copy job 
are executed after the copyjob finishes if I add a subjob. 
  As I said, we can use start, instead of exec, to be asynchronous, but using a 
subjob seems to need some refactor, maybe adding a new state for the xattr 
subjob.

INLINE COMMENTS

> cfeck wrote in copyjob.cpp:1119
> Here, too.

I tried various ways to call this as a subjob, but all of them leads to 
breakage of non xattr related tests. Maybe some major changes to the class are 
needed.

But the call can be asynchronous with little effort. All tests still pass if 
`start()` is called, instead of `exec()`.

> bruns wrote in copyjob.cpp:2199
> you can (typically) avoid the double (syscall, i.e. expensive) by 
> preallocating the array and only reallocating if you get `errno == ERANGE`. 
> Dito for getxattr.

In theory, xattrs have unlimited size on some filesystems. Ext limits it to a 
fs block (4 Kb on most end user setups). Allocating full 4 kb is a overkill, as 
most of xattr ae small.

But as most of files don't have xattr, and the function will return here, we 
have to pay the cost of the second call only if we will use it. If we 
preallocate memory, we have to pay a cost for every file.

> bruns wrote in copyjob.cpp:2221
> `... always ...`

On Linux, at least. Each item gets a `\0` terminator and the list itself too. 
Not tested on other systems.

> bruns wrote in copyjob.cpp:2225
> There should probably be a `if (!xattrkey.startsWith("user.")) continue;` 
> here.

Doing some research and test...

> Currently, the security, system, trusted, and user extended attribute classes 
> are defined as described below.  Additional classes may be added in the 
> future.

If a copy a file with kioclient as user, any attribute outside `user.` is lost 
and I get a warning for the ones I'm unable to preserve, but if I copy as root, 
all are preserved.

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, spoorun, nicolasfella, kde-frameworks-devel, michaelh

D17816: Support for xattrs on kio copy/move

[Cochise César]

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
  https://phabricator.kde.org/D17816?vs=48960=49487

BRANCH
  xattr-copy-support (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D17816

AFFECTED FILES
  autotests/jobtest.cpp
  autotests/jobtest.h
  src/core/CMakeLists.txt
  src/core/ConfigureChecks.cmake
  src/core/config-kiocore.h.cmake
  src/core/copyjob.cpp
  src/core/copyxattrjob.cpp
  src/core/copyxattrjob.h
  src/core/filecopyjob.cpp

To: cochise, dfaure
Cc: cfeck, bruns, phidrho, dhaumann, funkybomber, abika, pino, davidedmundson, 
ngraham, atha.kane, spoorun, nicolasfella, kde-frameworks-devel, michaelh

D17816: Support for xattrs on kio copy/move

[Cochise César]

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
  https://phabricator.kde.org/D17816

AFFECTED FILES
  autotests/jobtest.cpp
  autotests/jobtest.h
  src/core/CMakeLists.txt
  src/core/ConfigureChecks.cmake
  src/core/config-kiocore.h.cmake
  src/core/copyjob.cpp
  src/core/copyxattrjob.cpp
  src/core/copyxattrjob.h
  src/core/filecopyjob.cpp

To: cochise, dfaure
Cc: bruns, phidrho, dhaumann, funkybomber, abika, pino, davidedmundson, 
ngraham, atha.kane, spoorun, nicolasfella, kde-frameworks-devel, michaelh

D17816: Support for xattrs on kio copy/move

[Cochise César]

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 directories.
  Due the last problems with the method to copy xattrs being private, I
  created a new KIO::copy_xattr. Not very async, but the operation should
  be fast enough.
  Some more work on the tests, removed the ones that don't make sense for
  the case and added checks to preent breackage if xattrs ar not supported.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17816?vs=48733=48959

BRANCH
  xattr-copy-support (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D17816

AFFECTED FILES
  autotests/jobtest.cpp
  autotests/jobtest.h
  src/core/CMakeLists.txt
  src/core/ConfigureChecks.cmake
  src/core/config-kiocore.h.cmake
  src/core/copyjob.cpp
  src/core/filecopyjob.cpp

To: cochise, dfaure
Cc: bruns, phidrho, dhaumann, funkybomber, abika, pino, davidedmundson, 
ngraham, atha.kane, spoorun, nicolasfella, kde-frameworks-devel, michaelh

D17816: Support for xattrs on kio copy/move

[Cochise César]

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:  
https://github.com/KDE/ark/search?q=qsavefile_q=qsavefile
  
  Many  KDE 
applications use this class (the most prominent ones being Krita and Ark), and 
it provides a useful feature:
  
  > QSaveFile is an I/O device for writing text and binary files, without 
losing existing data if the writing operation fails.
  >  While writing, the contents will be written to a temporary file, and if no 
error happened, commit() will move it to the final file.
  
  But as it causes loss of ACLs and xattrs as side effect, I think it's use 
deserves a proper discussion in a dedicated topic.
  
  **On the support for KIO::file_copy:**
  
  Trying to add support on the low level `KIO::file_copy` I found that it would 
be hard without code duplication or exposing the function to copy xattrs that 
are currently on `KIO::CopyJobPrivate`, but this would change the API, adding a 
non virtual method, what I **think** wont break the ABI.
  
  I think the best way to do this is:
  
  1. Put copyXattr as a public method of `FileCopyJob`.
  2. Call copyXattr for files in `FileCopyJob::file_copy`, because 
`CopyJob::copy` uses it internally.
  3. Call copyXattr for directories on CopyJob, because it's the lowest 
abstraction level that knows that the mkdir have a source.
  
  I can't find uses of file_copy related to user files in a **very** 
superficial search, only backups, config files and network stuff. So, in 
theory, the current patch is good enough and we don't need to change the API, 
if this is a problem.

INLINE COMMENTS

> pino wrote in jobtest.cpp:534
> you can use `QStandardPaths::findExecutable()` to check whether `setfattr` is 
> available, and QSKIP the test if not

On linux, the command line bin are list/get/set**f**attr, on BSD 
list/get/set**x**attr, on mac are xattr -l/-p/-w, so this part will need a lot 
of ifdefs too, to work on many platforms.

> pino wrote in config-kiocore.h.cmake:8
> `HAVE_SYS_XATTR_H` is available here as side-effect of using the 
> FindACL.cmake module.
> Better explicitly look for `sys/xattr.h`, like 
> `src/ioslaves/file/CMakeLists.txt` does.

Didn't get it defined until added the snippet. Will research 
`src/ioslaves/file/CMakeLists.txt` to see if I can simplify the include for 
*BSD too.

> pino wrote in copyjob.cpp:2191-2193
> if `HAVE_SYS_XATTR_H` is not defined, the first instruction in `copyXattrs` 
> will be a return, and some compilers may warn/error out because the rest of 
> the function is unreachable; better stub out the whole function instead

I think looks better this way, but if it throw a warning I will revert to 
encapsulating the whole function.

> pino wrote in copyjob.cpp:2195
> - you don't need to call `data()` here, the return value of `toLocal8Bit()` 
> is already a QByteArray
> - `toLocal8Bit()` is the wrong function here: please use 
> `QFile::encodeName()`, which does the right job for QString -> local 
> filesystem paths

I need `data()`, because the compiler complains about non private cast. but 
using `encodeName()` should be better either way.

> pino wrote in copyjob.cpp:2206
> there is not just glibc; also, better check for `errno` as `ENOTSUP`, because 
> that means the source filesystem does not support xattrs, and thus you can 
> directly skip the rest of the function (as it will not work anyway)

If a error is found at this point, all the rest of the function will not run, 
because it is on the a statement, but doing a early check for destination 
filesystem support of xattr is a good idea.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D17816

To: cochise, dfaure
Cc: phidrho, dhaumann, funkybomber, abika, pino, davidedmundson, ngraham, 
atha.kane, spoorun, nicolasfella, kde-frameworks-devel, michaelh, bruns

D17816: Support for xattrs on kio copy/move

[Cochise César]

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

AFFECTED FILES
  autotests/jobtest.cpp
  autotests/jobtest.h
  src/core/ConfigureChecks.cmake
  src/core/config-kiocore.h.cmake
  src/core/copyjob.cpp

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 César]

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 César]

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 
work.
  Include checks moved to ConfigureChecks.cmake.
  Warning about destination FS not supporting xattrs demoted to debug.
  As listxattr function returns 0 if the source FS don't support xattrs, no 
ENOTSUP check is made at this stage.
  Nesting reduced using a switch for listlen. After some tests, I think this is 
the option with better redability.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17816?vs=48586=48731

BRANCH
  xattr-copy-support (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D17816

AFFECTED FILES
  autotests/jobtest.cpp
  autotests/jobtest.h
  src/core/ConfigureChecks.cmake
  src/core/config-kiocore.h.cmake
  src/core/copyjob.cpp
  src/ioslaves/file/CMakeLists.txt

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 César]

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 César]

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 were not tested outside linux.
  The debug messages were slighly improved, various @pino sugestions included 
and changes to file_unix reverted for a posterior PR.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17816?vs=48410=48586

BRANCH
  xattr-copy-support (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D17816

AFFECTED FILES
  autotests/jobtest.cpp
  autotests/jobtest.h
  src/core/CMakeLists.txt
  src/core/config-kiocore.h.cmake
  src/core/copyjob.cpp
  src/ioslaves/file/CMakeLists.txt

To: cochise, dfaure
Cc: abika, pino, davidedmundson, ngraham, atha.kane, spoorun, nicolasfella, 
kde-frameworks-devel, michaelh, bruns

D17816: Support for xattrs on kio copy/move

[Cochise César]

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, davidedmundson, ngraham, atha.kane, spoorun, nicolasfella, 
kde-frameworks-devel, michaelh, bruns

D17816: Initial support for xattrs on kio copy/move

[Cochise César]

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 it.
  Didn't searched yet about compatibility of these functions on libc 
alternatives. Frameworks officially supports a subset of them I should check?
  
  > - the changes to `file_unix.cpp` seem unrelated to you patch now, so better 
split them in an own patch
  
  OK, will do it.
  
  > - use `constData()` instead of `data()` every time the data needed is 
read-only
  
  OK, will do it.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D17816

To: cochise, dfaure
Cc: pino, davidedmundson, ngraham, atha.kane, spoorun, nicolasfella, 
kde-frameworks-devel, michaelh, bruns

D17816: Initial support for xattrs on kio copy/move

[Cochise César]

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
  https://phabricator.kde.org/D17816?vs=48384=48410

BRANCH
  xattr-copy-support (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D17816

AFFECTED FILES
  src/core/config-kiocore.h.cmake
  src/core/copyjob.cpp
  src/ioslaves/file/file_unix.cpp

To: cochise, dfaure
Cc: davidedmundson, ngraham, atha.kane, spoorun, nicolasfella, 
kde-frameworks-devel, michaelh, bruns

D17816: Initial support for xattrs on kio copy/move

[Cochise César]

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. I'm 
using a code almost identical to official example [1], and enlarging the 
buffer, or allocating it on the heap didn't resoled the issue.
  A hackish workaround is to not store the file path after conversion (`const 
char*`), and using `QUrl.path().toLocal8Bit().data();` in all nine calls.
  I'm not happy with this solution, but I think I reached my limit here. 
Commiting this hackish version for review, and going to make some tests.
  
  1 - http://man7.org/linux/man-pages/man2/listxattr.2.html

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17816?vs=48380=48384

BRANCH
  xattr-copy-support (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D17816

AFFECTED FILES
  src/core/config-kiocore.h.cmake
  src/core/copyjob.cpp
  src/ioslaves/file/file_unix.cpp

To: cochise, dfaure
Cc: davidedmundson, ngraham, atha.kane, spoorun, nicolasfella, 
kde-frameworks-devel, michaelh, bruns

D17816: Initial support for xattrs on kio copy/move

[Cochise César]

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 *BSD headers.
  Cureently, manual tests works on copy, move, and copy/move with renames.
  I think the results and design are aceptavle so far.
  
  If the qDebug calls are disabled, xattr_src (path from source file) gets
  corrupted on my system. I suspect there is someting nasty happening when
  glibc writes on the buffers (val and key). Will do some debugging now,
  and then write some unit tests.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17816?vs=48255=48380

BRANCH
  xattr-copy-support (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D17816

AFFECTED FILES
  src/core/config-kiocore.h.cmake
  src/core/copyjob.cpp
  src/ioslaves/file/file_unix.cpp

To: cochise, dfaure
Cc: davidedmundson, ngraham, atha.kane, spoorun, nicolasfella, 
kde-frameworks-devel, michaelh, bruns

D17816: Initial support for xattrs on kio copy/move

[Cochise César]

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 CopyJob, and found that it not applies to dirs 
renamed because of name conflicts.
  I plan to find the call points to tackle all the use cases and then refactor 
into a function, to avoid code duplication.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D17816

To: cochise, dfaure
Cc: davidedmundson, ngraham, atha.kane, spoorun, nicolasfella, 
kde-frameworks-devel, michaelh, bruns

D17816: Initial support for xattrs on kio copy/move

[Cochise César]

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 someone that knows KIO better 
than me is appreciated) but after this, we would need to rewrite as a function, 
to remove code duplication (and I'm open for suggestions of where to place it, 
as KIO is a rather big codebase). Baloo uses some very similar functions, from 
where i  so maybe we can unify the implementations in KIO level. So I'm 
planning to change things, but need some review to do this.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D17816

To: cochise, dfaure
Cc: ngraham, atha.kane, spoorun, nicolasfella, kde-frameworks-devel, michaelh, 
bruns

D17816: Initial support for xattrs on kio copy/move

[Cochise César]

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
  https://phabricator.kde.org/D17816

AFFECTED FILES
  src/core/config-kiocore.h.cmake
  src/core/copyjob.cpp
  src/ioslaves/file/file_unix.cpp

To: cochise, dfaure
Cc: nicolasfella, kde-frameworks-devel, michaelh, ngraham, bruns

D17816: Initial support for xattrs on kio copy/move

[Cochise César]

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
  https://phabricator.kde.org/D17816

To: cochise
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D17816: Initial support for xattrs on kio copy/move

[Cochise César]

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 tested.
  
  Relevant bugs:
  BUG: 116617
  BUG: 224635
  BUG: 274327
  BUG: 342152
  BUG: 370371
  BUG: 370543

REPOSITORY
  R241 KIO

BRANCH
  xattr-copy-support (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D17816

AFFECTED FILES
  src/ioslaves/file/file_unix.cpp

To: cochise
Cc: kde-frameworks-devel, michaelh, ngraham, bruns