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
