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