D17816: Initial support for xattrs on kio copy/move

[Pino Toscano]

pino added a comment.


  general notes:
  
  - NULL -> nullptr
  - there is not just glibc
  - the changes to `file_unix.cpp` seem unrelated to you patch now, so better split them in an own patch
  - use `constData()` instead of `data()` every time the data needed is read-only

INLINE COMMENTS

> config-kiocore.h.cmake:8
> +/* Defined if system has extended file attributes support. */
> +#cmakedefine01 HAVE_SYS_XATTR_H
>  

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

> copyjob.cpp:27-33
> +#if HAVE_SYS_XATTR_H
> +#if defined(Q_OS_LINUX) || defined(__GLIBC__) || defined(Q_OS_MAC)
> +#include <sys/xattr.h>
> +#elif defined(Q_OS_FREEBSD) || defined(Q_OS_NETBSD)
> +#include <sys/extattr.h>
> +#endif
> +#endif

this `#include` block has a slightly messy logic:

- if `sys/attr.h` is available, just include it directly with no other checks
- `sys/extattr.h` needs its own cmake check, including it if found

> copyjob.cpp:2191-2193
> +#if !(HAVE_SYS_XATTR_H)
> +    return;
> +#endif

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

> copyjob.cpp:2195
> +    long listlen, valuelen, destlen;
> +    const QByteArray sourcearray = source.path().toLocal8Bit().data();
> +    const char *xattrsrc = sourcearray.data();

- 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

> copyjob.cpp:2201
> +#elif defined(Q_OS_MAC)
> +    listlen = listxattr(xattrsrc, NULL, 0, 0);
> +#elif defined(Q_OS_FREEBSD) || defined(Q_OS_NETBSD)

NULL -> nullptr

> copyjob.cpp:2203
> +#elif defined(Q_OS_FREEBSD) || defined(Q_OS_NETBSD)
> +    listlen = extattr_list_file(xattr_src, EXTATTR_NAMESPACE_USER, NULL, 0);
> +#endif

NULL -> nullptr

> copyjob.cpp:2206
> +    if (listlen < 0) {
> +        qCWarning(KIO_COPYJOB_DEBUG) << "Glibc failed to extract xattr.";
> +    } else if (listlen == 0) {

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)

> copyjob.cpp:2227
> +#elif defined(Q_OS_MAC)
> +            valuelen = listxattr(xattrsrc, xattrkey.data(), NULL, 0, 0, 0);
> +#elif defined(Q_OS_FREEBSD) || defined(Q_OS_NETBSD)

NULL -> nullptr

> copyjob.cpp:2229
> +#elif defined(Q_OS_FREEBSD) || defined(Q_OS_NETBSD)
> +            valuelen = extattr_get_file(xattrsrc, EXTATTR_NAMESPACE_USER, xattrkey.data(), NULL, 0);
> +#endif

NULL -> nullptr

> copyjob.cpp:2257-2259
> +            if (destlen < 0) {
> +                qCWarning(KIO_COPYJOB_DEBUG) << "Glibc failed to write xattr on a file.";
> +            }

as noted above: checking that `errno` is `ENOTSUP` means that the destination filesystem does not support xattrs, so there is little point trying to set the other attributes

> file_unix.cpp:42
>  #if HAVE_SYS_XATTR_H
> +#if defined(Q_OS_LINUX) || defined(__GLIBC__) || defined(Q_OS_MAC)
>  #include <sys/xattr.h>

`HAVE_SYS_XATTR_H` from `config-kioslave-file.h` can be used here

> file_unix.cpp:44
>  #include <sys/xattr.h>
> +#elif defined(Q_OS_FREEBSD) || defined(Q_OS_NETBSD)
> +#include <sys/extattr.h>

better check for `sys/extattr.h` instead

REPOSITORY
  R241 KIO

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

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