D17816: Support for xattrs on kio copy/move

[Cochise César]

cochise added a comment.


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

INLINE COMMENTS

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

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

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

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

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

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

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

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

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

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

REPOSITORY
  R241 KIO

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

To: cochise, dfaure
Cc: phidrho, dhaumann, funkybomber, abika, pino, davidedmundson, ngraham, atha.kane, spoorun, nicolasfella, kde-frameworks-devel, michaelh, bruns
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20190105/cdfd6d50/attachment-0001.html>