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
