D17816: Support for xattrs on kio copy/move

[Nathaniel Graham]

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

D17816: Support for xattrs on kio copy/move

[Gleb Popov]

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, 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

[Stefan Brüns]

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 trouble, hopefully. :)
  
  
  This has landed long time ago. Unfortunately, in combination with SELINUX 
getfattr returns an error not mentioned in any man page, which causes an 
inifinite loop in the current code.
  
  Fixed by https://invent.kde.org/frameworks/kio/-/merge_requests/368

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

D17816: Support for xattrs on kio copy/move

[Nathaniel Graham]

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: arrowd, dfaure, chinmoyr, bruns, #frameworks, tmarshall, usta, cochise
Cc: kdudka, 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

[Kamil Dudka]

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 implement some advanced error handling, yet it 
is written in a way that it passed a review with a fundamental programming 
mistake in the basic error handling.
  
  The fix proposed in https://phabricator.kde.org/D17816#677448 is incomplete, 
as I understand it.  If we break the loop with `valuelen == -1`, the value will 
be passed as `size_t` to the `size` argument of `fsetxattr()`, which may lead 
to reading `value.constData()` out of bounds.

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

D17816: Support for xattrs on kio copy/move

[Stefan Brüns]

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 your choice.
  
  
  I never said its perfect. There is exactly one case which was missed (and 
only in this one location, for both listxattr and setxattr a return value of -1 
is handled correctly).
  
  > I do not think that KDE users will appreciate some **blindly** coded 
**fancy features** on top of that when the basic functionality gets **totally 
broken** as a result of these **experiments**.
  
  Such well chosen words ...
  
  Btw, there are plenty of more cases where kio deliberately deviates from the 
behavior of low level system tools.

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

D17816: Support for xattrs on kio copy/move

[Kamil Dudka]

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 like this:
  
execve("/bin/cp", ["cp", "--preserve=xattr", "/mnt/mmc/file", "."], 
0x7ffdf8529c88 /* 81 vars */) = 0
access("/etc/ld.so.preload", R_OK)  = -1 ENOENT (No such file or 
directory)
openat(AT_FDCWD, "/etc/ld.so.cache", O_RDONLY|O_CLOEXEC) = 3
openat(AT_FDCWD, "/lib64/libacl.so.1", O_RDONLY|O_CLOEXEC) = 3
openat(AT_FDCWD, "/lib64/libattr.so.1", O_RDONLY|O_CLOEXEC) = 3
openat(AT_FDCWD, "/lib64/libc.so.6", O_RDONLY|O_CLOEXEC) = 3
stat(".", {st_mode=S_IFDIR|S_ISVTX|0777, st_size=1522, ...}) = 0
newfstatat(AT_FDCWD, "/mnt/mmc/file", {st_mode=S_IFREG|0755, st_size=0, 
...}, 0) = 0
newfstatat(AT_FDCWD, "./file", 0x7ffea3f59f50, 0) = -1 ENOENT (No such file 
or directory)
openat(AT_FDCWD, "/mnt/mmc/file", O_RDONLY) = 3
openat(AT_FDCWD, "./file", O_WRONLY|O_CREAT|O_EXCL, 0755) = 4
flistxattr(3, NULL, 0)  = 17
flistxattr(3, "security.selinux\0", 17) = 17
openat(AT_FDCWD, "/etc/xattr.conf", O_RDONLY) = 5
fgetxattr(3, "security.selinux", NULL, 0) = -1 EOPNOTSUPP (Operation not 
supported)
cp: getting attribute 'security.selinux' of 'security.selinux': Operation 
not supported
+++ exited with 1 +++

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

D17816: Support for xattrs on kio copy/move

[Stefan Brüns]

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
  This code is quite mature and it is used by cp(1) on GNU/Linux for instance.  
I do not think that KDE users will appreciate some blindly coded fancy features 
on top of that when the basic functionality gets totally broken as a result of 
these experiments.  Please keep the code simple and reliable.
  
  
  Its not blindly coded. Please keep your arrogant comments to yourself.
  
  And obviously, there is something strange going on on your system - 
flistxattr returns a list of keys, but fgetxattr then returns ENOTSUP?

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

D17816: Support for xattrs on kio copy/move

[Kamil Dudka]

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 not think that KDE users will appreciate some blindly coded fancy features 
on top of that when the basic functionality gets totally broken as a result of 
these experiments.  Please keep the code simple and reliable.

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

D17816: Support for xattrs on kio copy/move

[Stefan Brüns]

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 the second call then returns ERANGE you have to start from the 
beginning.

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

D17816: Support for xattrs on kio copy/move

[Kamil Dudka]

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, #frameworks, tmarshall, usta, cochise
Cc: kdudka, 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

[Stefan Brüns]

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 of calling fgetxattr(..., 0) again?  It is still 
going to busy-loop indefinitely in this case, doesn't it?  How many times do we 
actually need to call fgetxattr() on a single file descriptor?  Twice?  Then 
unbounded loop is not the best construction to begin with.
  
  
  Ever heard of a TOCTOU race?
  
  Quoting from man 2 getxattr:
  
  > If size is specified as zero, these calls return the current size of the 
named extended attribute (and leave value unchanged).  This can be used to 
determine the size of the buffer that should be supplied in a subsequent call.  
 (But,  bear  in  mind  that  there is a possibility that the attribute value 
may change between the two calls, so that it is still necessary to check the 
return status from the second call.)

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

D17816: Support for xattrs on kio copy/move

[Kamil Dudka]

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 in this case, doesn't it?  How many times do we 
actually need to call fgetxattr() on a single file descriptor?  Twice?  Then 
unbounded loop is not the best construction to begin with.

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

D17816: Support for xattrs on kio copy/move

[Gleb Popov]

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 progressing and the process ended up 
consuming 100% CPU forever.  Later I discovered that the cause of the breakage 
was this patch: If fgetxattr() fails with ENOTSUP, the code loops indefinitely 
calling fgetxattr().
  >
  > The following hotfix made copying of files in Krusader work again for me:
  >  ...
  >
  > However, the code might need bigger changes to cover all the cases.  I do 
not fully understand the (IMO over-complicated) loops around flistxattr() and 
fgetxattr().  Note that the fact that one of them uses `while (true) { ... }` 
whereas the other one uses `do { ... } while (true)` does not improve code 
readability either.
  
  
  I think, the correct fix is something like
  
diff --git a/src/ioslaves/file/file_unix.cpp 
b/src/ioslaves/file/file_unix.cpp
index 74767123..4bc87c57 100644
--- a/src/ioslaves/file/file_unix.cpp
+++ b/src/ioslaves/file/file_unix.cpp
@@ -614,8 +614,8 @@ bool FileProtocol::copyXattrs(const int src_fd, const 
int dest_fd)
 valuelen = 0;
 continue;
 }
-// happens when attr value is an empty string
-if (valuelen == 0) {
+// valuelen=0 happens when attr value is an empty string
+if (valuelen == 0 || valuelen == -1) {
 break;
 }
 } while (true);
  
  Can you check if this works for you too?

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

D17816: Support for xattrs on kio copy/move

[Kamil Dudka]

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 that the cause of the breakage 
was this patch: If fgetxattr() fails with ENOTSUP, the code loops indefinitely 
calling fgetxattr().
  
  The following hotfix made copying of files in Krusader work again for me:
  
--- a/src/ioslaves/file/file_unix.cpp
+++ b/src/ioslaves/file/file_unix.cpp
@@ -642,35 +642,35 @@ bool FileProtocol::copyXattrs(const int src_fd, const 
int dest_fd)
 ssize_t valuelen = 0;
 do {
 value.resize(valuelen);
 #if HAVE_SYS_XATTR_H && !defined(__stub_getxattr) && !defined(Q_OS_MAC)
 valuelen = fgetxattr(src_fd, key.constData(), value.data(), 
valuelen);
 #elif defined(Q_OS_MAC)
 valuelen = fgetxattr(src_fd, key.constData(), value.data(), 
valuelen, 0, 0);
 #elif HAVE_SYS_EXTATTR_H
 valuelen = extattr_get_fd(src_fd, EXTATTR_NAMESPACE_USER, 
key.constData(), valuelen == 0 ? nullptr : value.data(), valuelen);
 #endif
 if (valuelen > 0 && value.size() == 0) {
 continue;
 }
 if (valuelen > 0 && value.size() > 0) {
 break;
 }
-if (valuelen == -1 && errno == ERANGE) {
+if (valuelen == -1 && errno != ERANGE) {
 valuelen = 0;
-continue;
+break;
 }
 // happens when attr value is an empty string
 if (valuelen == 0) {
 break;
 }
 } while (true);

 // Write key:value pair on destination
 #if HAVE_SYS_XATTR_H && !defined(__stub_getxattr) && !defined(Q_OS_MAC)
 ssize_t destlen = fsetxattr(dest_fd, key.constData(), 
value.constData(), valuelen, 0);
 #elif defined(Q_OS_MAC)
 ssize_t destlen = fsetxattr(dest_fd, key.constData(), 
value.constData(), valuelen, 0, 0);
 #elif HAVE_SYS_EXTATTR_H
 ssize_t destlen = extattr_set_fd(dest_fd, EXTATTR_NAMESPACE_USER, 
key.constData(), value.constData(), valuelen);
 #endif
 if (destlen == -1 && errno == ENOTSUP) {
  
  However, the code might need bigger changes to cover all the cases.  I do not 
fully understand the (IMO over-complicated) loops around flistxattr() and 
fgetxattr().  Note that the fact that one of them uses `while (true) { ... }` 
whereas the other one uses `do { ... } while (true)` does not improve code 
readability either.

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

D17816: Support for xattrs on kio copy/move

[David Faure]

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 `set(HAVE_POSIX_ACL ...)` part into `FindACL.cmake` and use 
> this module everywhere. Does that sound OK?

Yep.

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

[Gleb Popov]

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, without the use of extattr, it uses statfs instead.
> So I think I was wrong, this part is fine.
> 
> I guess the only thing that's a bit weird is that kio_file's attr code 
> (unrelated to ACLs) relies on FindACL linking to the right lib (on Linux). It 
> would be cleaner if the attr stuff was separate. But no big deal I guess.

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 `set(HAVE_POSIX_ACL ...)` part into `FindACL.cmake` and use this 
module everywhere. Does that sound OK?

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

[David Faure]

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 extattr headers are found?
> 
> Or should I change checks in kpropertiesdialog.cpp from `HAVE_POSIX_ACL` to 
> `HAVE_*ATTR_H`?

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, without the use of extattr, it uses statfs instead.
So I think I was wrong, this part is fine.

I guess the only thing that's a bit weird is that kio_file's attr code 
(unrelated to ACLs) relies on FindACL linking to the right lib (on Linux). It 
would be cleaner if the attr stuff was separate. But no big deal I guess.

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

[Gleb Popov]

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 up into "find extended attribute stuff" and "find 
> ACL stuff", the latter relying on the former.
> But this merge request has been pending for long enough, let's do buildsystem 
> refactoring as part of it.
> 
> Let's leave this part as is for now.
> 
> If you feel like it, I suggest followup commits to 1) enable the ACL stuff on 
> systems with extattr, see the little bit of code in kpropertiesdialog.cpp, 
> and 2) separate the cmake stuff for ACLs from the cmake stuff for extended 
> attributes.

> 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 extattr headers are found?

Or should I change checks in kpropertiesdialog.cpp from `HAVE_POSIX_ACL` to 
`HAVE_*ATTR_H`?

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

[Nathaniel Graham]

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, #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

[Nathaniel Graham]

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

D17816: Support for xattrs on kio copy/move

[David Faure]

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 extra libraries are required.
> 
> So, what should be done here, then? Just move `HAVE_SYS_EXTATTR_H` check to 
> `FindACL.cmake` module?

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 up into "find extended attribute stuff" and "find ACL 
stuff", the latter relying on the former.
But this merge request has been pending for long enough, let's do buildsystem 
refactoring as part of it.

Let's leave this part as is for now.

If you feel like it, I suggest followup commits to 1) enable the ACL stuff on 
systems with extattr, see the little bit of code in kpropertiesdialog.cpp, and 
2) separate the cmake stuff for ACLs from the cmake stuff for extended 
attributes.

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

[Gleb Popov]

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.
> 
> Even though this isn't technically about ACLs, I'm guessing it all links 
> *because* FindACL.cmake links to libattr, right?
> And then this might also mean that the condition in FindACL.cmake needs to be 
> updated, it currently *requires* HAVE_SYS_XATTR_H instead of allowing both 
> variants.
> 
> But then I guess this means kpropertiesdialog.cpp needs to be ported to the 
> sys/extattr.h API.
> 
> Has this been tested on a system with sys/extattr.h? I'm wondering if what 
> will happen is: FindACL didn't find sys/attr.h so it doesn't link to libattr, 
> and then the new code here fails to link. Or am I missing something?

> 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 extra libraries are required.

So, what should be done here, then? Just move `HAVE_SYS_EXTATTR_H` check to 
`FindACL.cmake` module?

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

[Gleb Popov]

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&id=83375

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/config-kioslave-file.h.cmake
  src/ioslaves/file/file.h
  src/ioslaves/file/file_unix.cpp

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

[David Faure]

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 `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.

Even though this isn't technically about ACLs, I'm guessing it all links 
*because* FindACL.cmake links to libattr, right?
And then this might also mean that the condition in FindACL.cmake needs to be 
updated, it currently *requires* HAVE_SYS_XATTR_H instead of allowing both 
variants.

But then I guess this means kpropertiesdialog.cpp needs to be ported to the 
sys/extattr.h API.

Has this been tested on a system with sys/extattr.h? I'm wondering if what will 
happen is: FindACL didn't find sys/attr.h so it doesn't link to libattr, and 
then the new code here fails to link. Or am I missing something?

> file_unix.cpp:535
> +
> +bool FileProtocol::copyXattrs(const int src_fd, const int dest_fd)
> +{

This whole method should be in `#if HAVE_SYS_XATTR_H || HAVE_SYS_EXTATTR_H`.

Otherwise, on systems with neither defined, a lot of compiler warnings will 
happen, like "keyLen isn't initialized" on line 591.

You can leave the method in the .h without #if (those defines aren't known 
there).
Here you can either #if the method itself (declared and not defined and not 
used, is OK, although some might say, a bit unclean)
or #if the whole method *contents*, but then in the #else you need 
`Q_UNUSED(src_fd); Q_UNUSED(dest_fd);` to avoid compiler warnings about unused 
parameters.

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

[Nathaniel Graham]

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

D17816: Support for xattrs on kio copy/move

[Stefan Brüns]

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

D17816: Support for xattrs on kio copy/move

[Nathaniel Graham]

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

D17816: Support for xattrs on kio copy/move

[Gleb Popov]

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

D17816: Support for xattrs on kio copy/move

[Gleb Popov]

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&id=83366

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/config-kioslave-file.h.cmake
  src/ioslaves/file/file.h
  src/ioslaves/file/file_unix.cpp

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

[Stefan Brüns]

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 &src)
> +{

should be `dest`.

> jobtest.cpp:489
> +qDebug() << resultdest;
> +m_SkipXattr = true;
> +}

Should have a `break` or `return` here.

> arrowd wrote in jobtest.cpp:780
> 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 want to skip xattr stuff if either source or 
> destination doesn't support it

`setXattr`, called from `checkXattrFsSupport`, unconditionally sets 
`m_SkipXattr`. 
You want something like

  bool canRead =  checkXattrFsSupport(homeDir);
  bool canWrite =  checkXattrFsSupport(otherHomeDir);
  if (canRead && canWrite) {

and get rid of the `m_SkipXAttr` variable.

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

[Gleb Popov]

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 blamed the code down to svn->git import and this bug 
was present all the time.

To properly fix this I'd start with a test for `KIO::chmod` job that changes 
ACL permissions, but I can't test this on FreeBSD as we don't have 
`acl_from_mode`.

Anyways, it is out of scope of this review.

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

[Gleb Popov]

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&id=83365

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/config-kioslave-file.h.cmake
  src/ioslaves/file/file.h
  src/ioslaves/file/file_unix.cpp

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

[Gleb Popov]

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 want to skip xattr stuff if either source or 
destination doesn't support it

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

[Stefan Brüns]

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

[Ömer Fadıl Usta]

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) == 0) { // let's be safe

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.

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

[Gleb Popov]

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&id=83364

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/config-kioslave-file.h.cmake
  src/ioslaves/file/file.h
  src/ioslaves/file/file_unix.cpp

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

[Stefan Brüns]

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 split/join dance.

> jobtest.cpp:780
> +checkXattrFsSupport(filePath);
> +checkXattrFsSupport(dest);
> +if (!m_SkipXattr) {

This will set `m_SkipXattr` unconditionally even if the source FS does not 
support Xattrs.

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

[Gleb Popov]

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&id=83361

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/config-kioslave-file.h.cmake
  src/ioslaves/file/file.h
  src/ioslaves/file/file_unix.cpp

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

[Stefan Brüns]

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, #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

[Nathaniel Graham]

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

D17816: Support for xattrs on kio copy/move

[Gleb Popov]

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

D17816: Support for xattrs on kio copy/move

[Gleb Popov]

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&id=83334

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/config-kioslave-file.h.cmake
  src/ioslaves/file/file.h
  src/ioslaves/file/file_unix.cpp

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

[Gleb Popov]

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

[Gleb Popov]

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&id=83319

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/config-kioslave-file.h.cmake
  src/ioslaves/file/file.h
  src/ioslaves/file/file_unix.cpp

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

[Stefan Brüns]

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 << " " << value;
> +if (valuelen > 0 && value.size() == 0) {

No extra spaces for qDebug, inserts spaces itself.

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

[Gleb Popov]

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&id=83318

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/config-kioslave-file.h.cmake
  src/ioslaves/file/file.h
  src/ioslaves/file/file_unix.cpp

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

[Stefan Brüns]

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.

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

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

and preferably add a test case ...

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

[Gleb Popov]

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 find out sufficient value.

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

[Gleb Popov]

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&id=83309

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/config-kioslave-file.h.cmake
  src/ioslaves/file/file.h
  src/ioslaves/file/file_unix.cpp

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

[Stefan Brüns]

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
> +continue;
> +}
> +} while (true);

Infinite loop on valuelen == 0

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

[Nathaniel Graham]

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

D17816: Support for xattrs on kio copy/move

[Gleb Popov]

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&id=83302

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/config-kioslave-file.h.cmake
  src/ioslaves/file/file.h
  src/ioslaves/file/file_unix.cpp

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

[Gleb Popov]

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, 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

[Gleb Popov]

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&id=83207

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/config-kioslave-file.h.cmake
  src/ioslaves/file/file.h
  src/ioslaves/file/file_unix.cpp

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

[Stefan Brüns]

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 {
> +keylist.resize(listlen);

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

> file_unix.cpp:164
> +if (errno == ENOTSUP) {
> +qCDebug(KIO_FILE) << "source filesystem don't support 
> xattrs";
> +}

"does not"

> file_unix.cpp:175
> +keylist.resize(listlen);
> +keylist.squeeze();
> +

why `.squeeze()` ?

> file_unix.cpp:177
> +
> +// Linux and MacOS return = list of null terminated string, each string 
> = [data,'\0']
> +// BSD return = list of items, each item prepended of 1 byte size = 
> [size, data]

"return a list of null terminated strings"

> file_unix.cpp:178
> +// Linux and MacOS return = list of null terminated string, each string 
> = [data,'\0']
> +// BSD return = list of items, each item prepended of 1 byte size = 
> [size, data]
> +QByteArray::const_iterator keyPtr = keylist.cbegin();

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

> file_unix.cpp:185
> +while (keyPtr != keylist.cend()) {
> +// Get the size of key
> +#if HAVE_SYS_XATTR_H

"key size" or "size of the key"

> file_unix.cpp:226
> +if (destlen == -1 && errno == ENOTSUP) {
> +qCDebug(KIO_FILE) << "Destination filesystem don't support 
> xattrs";
> +return false;

"does not"

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

[Gleb Popov]

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, 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

[Gleb Popov]

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&id=83203

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/config-kioslave-file.h.cmake
  src/ioslaves/file/file.h
  src/ioslaves/file/file_unix.cpp

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

[Stefan Brüns]

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 !!!

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

[Gleb Popov]

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

D17816: Support for xattrs on kio copy/move

[Gleb Popov]

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&id=83192

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/config-kioslave-file.h.cmake
  src/ioslaves/file/file.h
  src/ioslaves/file/file_unix.cpp

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

[Stefan Brüns]

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;

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

> file_unix.cpp:155
> +if (errno == ERANGE) {
> +keylist.resize(listlen + 512);
> +}

This is still definitely wrong ...

> file_unix.cpp:200
> +if (valuelen == -1 && errno == ERANGE) {
> +value.resize(valuelen + 512);
> +}

also wrong ...

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

[Gleb Popov]

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, 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

[Gleb Popov]

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&id=83166

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/config-kioslave-file.h.cmake
  src/ioslaves/file/file.h
  src/ioslaves/file/file_unix.cpp

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

[Stefan Brüns]

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 = [](const QString& attrName, const QString& value, 
const QString& fileName) {
  return QStringList{QLatin1String("-n"), attrName, QLatin1String("-v"), 
value, fileName};
  }

and do it when doing the path lookup, once.

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

[Gleb Popov]

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
  https://phabricator.kde.org/D17816?vs=83068&id=83143

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/config-kioslave-file.h.cmake
  src/ioslaves/file/file.h
  src/ioslaves/file/file_unix.cpp

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]

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

[Gleb Popov]

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
  https://phabricator.kde.org/D17816?vs=83016&id=83068

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/config-kioslave-file.h.cmake
  src/ioslaves/file/file.h
  src/ioslaves/file/file_unix.cpp

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

[Gleb Popov]

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
  https://phabricator.kde.org/D17816?vs=82960&id=83016

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/config-kioslave-file.h.cmake
  src/ioslaves/file/file.h
  src/ioslaves/file/file_unix.cpp

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

[Gleb Popov]

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 
copied. I'll look into that later.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17816?vs=80336&id=82960

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/config-kioslave-file.h.cmake
  src/ioslaves/file/file.h
  src/ioslaves/file/file_unix.cpp

To: arrowd, dfaure, chinmoyr, bruns, #frameworks, tmarshall, usta, cochise
Cc: usta, scheirle, anthonyfieroni, 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

[Gleb Popov]

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, 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

[Nathaniel Graham]

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

D17816: Support for xattrs on kio copy/move

[Stefan Brüns]

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, the implementation is wrong.

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

> file_unix.cpp:153
> +if (errno == ERANGE) {
> +keylist.resize(listlen + 512);
> +}

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

> file_unix.cpp:163
> +}
> +} while (listlen == -1 && errno == ERANGE);
> +

man 2 flistxattr:

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

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

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

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

> file_unix.cpp:173
> +// For each key
> +keylist.squeeze();
> +while (keyPtr != keylist.cend()) {

Thats bad. Thats really bad.

squeeze may reallocate. This invalidates keyPtr.

> file_unix.cpp:186
> +ssize_t valuelen = 512;
> +QByteArray value(valuelen, Qt::Uninitialized);
> +do {

`QByteArray value` should be created outside the keyPtr loop.

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

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

> file_unix.cpp:196
> +if (valuelen == -1 && errno == ERANGE) {
> +value.resize(valuelen + 512);
> +}

-1 + 512 = 511 ...

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

> file_unix.cpp:198
> +}
> +} while (valuelen == -1 && errno == ERANGE);
> +

Again, valuelen may be -1 here ...

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, michaelh

D17816: Support for xattrs on kio copy/move

[Cochise César]

D17816: Support for xattrs on kio copy/move

[Stefan Brüns]

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.

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

[Stefan Brüns]

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 example  this one looks like a warning instead of a debug info for 
me ( but not sure what your opinions are) :
  >  qCDebug(KIO_FILE) << "libc failed to extract list of xattr from file";
  >
  >   or this one :
  >   qCDebug(KIO_FILE) << "cant copy Extended attributes";
  >
  > other than those for me it looks ok
  
  
  These ones are fine, as it depends on the XAttr support of the file system - 
we don't want a warning when someone
  copies from a FAT USB-Stick.

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

[Stefan Brüns]

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, 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

[Nathaniel Graham]

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, 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 79054.
cochise added a comment.


  qCDebug -> qCWarning

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17816?vs=78777&id=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

[Ömer Fadıl Usta]

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

D17816: Support for xattrs on kio copy/move

[Nathaniel Graham]

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 example  this one looks like a warning instead of a debug info for 
me ( but not sure what your opinions are) :
  >  qCDebug(KIO_FILE) << "libc failed to extract list of xattr from file";
  >
  >   or this one :
  >   qCDebug(KIO_FILE) << "cant copy Extended attributes";
  >
  > other than those for me it looks ok
  
  
  You should change your status to "Accepted" once you're happy with it

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

[Ömer Fadıl Usta]

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 your opinions are) :
  qCDebug(KIO_FILE) << "libc failed to extract list of xattr from file";
   or this one :
   qCDebug(KIO_FILE) << "cant copy Extended attributes";
  other than those for me it looks ok

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

[David Faure]

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
  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&id=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

[Ömer Fadıl Usta]

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

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 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&id=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

[Ömer Fadıl Usta]

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 variables in kde ? is so xattrwriter => 
xattrWriter

> jobtest.cpp:531
> +
> +QProcess xattrreader;
> +xattrreader.setProcessChannelMode(QProcess::MergedChannels);

camelCase ?  xattrreader => xattrReader

> file_unix.cpp:167
> +// Linux and MacOS return = list of null terminated string, each string 
> = [data,'\0']
> +// BSD return = list of itens, each item prepended of 1 byte size = 
> [size, data]
> +

small typo in comment : itens => items

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 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&id=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

[Stefan Brüns]

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 and args, see 
https://doc.qt.io/qt-5/qprocess.html#details

  struct ProgramWithArgs { QString program; QStringList args; };
  QVector xattrprograms = {
  { {"getfattr", {}},   {"setfattr", {}} },
  { {"getextattr", {}}, {"setextattr", {}} },
  { {"xattr", {"-p}},   {"xattr", {"-w"}} },
  };
  for (auto& getset : xattrprograms) {
  m_getXattrCmd.program = 
QStandardPaths::findExecutable(getset.first.program);
  m_setXattrCmd.program = 
QStandardPaths::findExecutable(getset.second.program);
  if (!m_getXattrCmd.programm.isEmpty() && 
!m_setXattrCmd.programm.isEmpty()) {
  m_getXattrCmd.args = getset.first.args;
  m_setXattrCmd.args = getset.second.args;
  break;
  }
  } 
  if (m_getXattrCmd.isEmpty() || m_getXattrCmd.isEmpty()) {
  qWarning() << "Neither getfattr, getextattr nor xattr was found.";
  }

> jobtest.cpp:512
> +QVERIFY(xattrwriter.waitForStarted());
> +QCOMPARE(xattrwriter.state(), QProcess::Running);
> +QVERIFY(xattrwriter.waitForFinished(-1));

This seem bogus to me - what guarantees the process has not already finished?

> jobtest.cpp:524
> +
> +void JobTest::compareXattr(const QString &src, const QString &dest)
> +{

void JobTest::compareXattr(const QString &src, const QString &dest)
  {
  auto srcAttrs = readXattr(src);
  auto destAttrs = readXattr(dest);
  QCOMPARE(srcAttrs, destAttrs);
  }
  
  QList JobTest::readXattr(const QString &path)
  {

> jobtest.cpp:536
> +QVERIFY(xattrreader.waitForStarted());
> +QCOMPARE(xattrreader.state(), QProcess::Running);
> +QVERIFY(xattrreader.waitForFinished(-1));

dito

> file_unix.cpp:180
> +#elif HAVE_SYS_EXTATTR_H
> +keyLen = *keyPtr;
> +keyPtr++;

`*keyPtr` needs cast to unsigned char.

> file_unix.cpp:232
> +keyPtr += keyLen;
> +#endif
> +}

what about `#elif defined(Q_OS_MAC)`?

> file_unix.cpp:148
> +#endif
> +if (listlen == -1) {
> +qCDebug(KIO_FILE) << "libc failed to extract list of xattr from 
> file";

You never check if the source FS supports xattrs.

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&id=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

[David Faure]

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

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

> 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&id=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

[David Faure]

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) {
> +qCDebug(KIO_FILE) << "file " << src_fd << " don't have any xattr";
> +return false;

No need for spaces (after "file" and before "don't"), q[C]Debug inserts spaces 
automatically

> file_unix.cpp:166
> +#endif
> +// Linux and MacOS return = list of null terminated string, each srting 
> = [data,'\0']
> +// BSD return = list of itens, each item prepended of 1 byte size = 
> [size, data]

typo: srting => string

> file_unix.cpp:169
> +
> +QByteArray::const_iterator offset = keylist.cbegin();
> +size_t keyLen;

To me an offset is an integer that represents a relative index.

This is more like a source pointer, or keyData or something like that.

> file_unix.cpp:184
> +#if HAVE_SYS_XATTR_H
> +key = qstrcpy(key.data(), offset);
> +#elif HAVE_SYS_EXTATTR_H

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.

> file_unix.cpp:186
> +#elif HAVE_SYS_EXTATTR_H
> +key = qstrncpy(key.data(), offset, keyLen);
> +#endif

Same problem here. Similar solution: QByteArray key(offset, keyLen);

This removes the need for key.resize() before. Which also removes the need for 
the two parallel series of ifdefs, you can just merge them.

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 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&id=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