Changeset View
Changeset View
Standalone View
Standalone View
src/ioslaves/file/file_unix.cpp
| Show All 31 Lines | |||||
| 32 | #include <qplatformdefs.h> | 32 | #include <qplatformdefs.h> | ||
| 33 | 33 | | |||
| 34 | #include <QDebug> | 34 | #include <QDebug> | ||
| 35 | #include <kconfiggroup.h> | 35 | #include <kconfiggroup.h> | ||
| 36 | #include <klocalizedstring.h> | 36 | #include <klocalizedstring.h> | ||
| 37 | #include <kmountpoint.h> | 37 | #include <kmountpoint.h> | ||
| 38 | 38 | | |||
| 39 | #include <errno.h> | 39 | #include <errno.h> | ||
| 40 | #include <sys/xattr.h> | ||||
kossebau: This unconditional include sadly breaks the build at least on FreeBSD (see https://build.kde. | |||||
I'm sorry for breaking the build. Here is the fix: D11716 rominf: I'm sorry for breaking the build. Here is the fix: D11716 | |||||
| 40 | #include <utime.h> | 41 | #include <utime.h> | ||
| 41 | 42 | | |||
| 42 | #include <KAuth> | 43 | #include <KAuth> | ||
| 43 | 44 | | |||
| 44 | #include "fdreceiver.h" | 45 | #include "fdreceiver.h" | ||
| 45 | 46 | | |||
| 46 | //sendfile has different semantics in different platforms | 47 | //sendfile has different semantics in different platforms | ||
| 47 | #if defined HAVE_SENDFILE && defined Q_OS_LINUX | 48 | #if defined HAVE_SENDFILE && defined Q_OS_LINUX | ||
| ▲ Show 20 Lines • Show All 357 Lines • ▼ Show 20 Line(s) | 396 | { | |||
| 405 | hostname[ 0 ] = '\0'; | 406 | hostname[ 0 ] = '\0'; | ||
| 406 | if (!gethostname(hostname, 255)) { | 407 | if (!gethostname(hostname, 255)) { | ||
| 407 | hostname[sizeof(hostname) - 1] = '\0'; | 408 | hostname[sizeof(hostname) - 1] = '\0'; | ||
| 408 | } | 409 | } | ||
| 409 | 410 | | |||
| 410 | return (QString::compare(url.host(), QLatin1String(hostname), Qt::CaseInsensitive) == 0); | 411 | return (QString::compare(url.host(), QLatin1String(hostname), Qt::CaseInsensitive) == 0); | ||
| 411 | } | 412 | } | ||
| 412 | 413 | | |||
| 414 | #ifdef Q_OS_LINUX | ||||
| 415 | static bool isNtfsHidden(const QString &filename) | ||||
dfaure: static | |||||
| 416 | { | ||||
| 417 | constexpr auto attrName = "system.ntfs_attrib_be"; | ||||
| 418 | const auto filenameEncoded = QFile::encodeName(filename).data(); | ||||
Ouch, you can't call data() here, that's keeping a char* pointing to a deleted (temporary) QByteArray. dfaure: Ouch, you can't call data() here, that's keeping a char* pointing to a deleted (temporary)… | |||||
| 419 | auto length = getxattr(filenameEncoded, attrName, nullptr, 0); | ||||
| 420 | if (length <= 0) { | ||||
| 421 | return false; | ||||
| 422 | } | ||||
Doesn't this need to be static, for the next line to be standards compliant? Not 100% sure. dfaure: Doesn't this need to be static, for the next line to be standards compliant? Not 100% sure. | |||||
rominf: No, it's enough for array length to be `constexpr`. | |||||
dfaure: Hmm, that makes a lot of sense, actually :-) | |||||
| 423 | constexpr size_t xattr_size = 1024; | ||||
| 424 | char strAttr[xattr_size]; | ||||
but filename.toLocal8Bit() in a local QByteArray so you don't do this conversion twice. dfaure: but filename.toLocal8Bit() in a local QByteArray so you don't do this conversion twice.
And in… | |||||
| 425 | length = getxattr(filenameEncoded, attrName, strAttr, xattr_size); | ||||
| 426 | if (length <= 0) { | ||||
| 427 | return false; | ||||
| 428 | } | ||||
| 429 | | ||||
| 430 | // Decode result to hex string | ||||
| 431 | static const auto digits = "0123456789abcdef"; | ||||
dfaure: QVarLengthArray might be useful here to avoid an allocation every time | |||||
rominf: TIL about `QVarLengthArray`. Thank you. | |||||
| 432 | QVarLengthArray<char> hexAttr(static_cast<int>(length) * 2 + 4); | ||||
| 433 | char *c = strAttr, *e = hexAttr.data(); | ||||
| 434 | *e++='0'; *e++ = 'x'; | ||||
| 435 | for (auto n = 0; n < length; n++, c++) { | ||||
| 436 | *e++ = digits[(static_cast<uchar>(*c) >> 4)]; | ||||
| 437 | *e++ = digits[(static_cast<uchar>(*c) & 0x0F)]; | ||||
| 438 | } | ||||
| 439 | *e = '\0'; | ||||
| 440 | | ||||
| 441 | // Decode hex string to int | ||||
| 442 | auto intAttr = static_cast<uint>(strtol(hexAttr.data(), nullptr, 16)); | ||||
| 443 | | ||||
| 444 | constexpr auto FILE_ATTRIBUTE_HIDDEN = 0x2u; | ||||
| 445 | return static_cast<bool>(intAttr & FILE_ATTRIBUTE_HIDDEN); | ||||
| 446 | } | ||||
| 447 | #endif | ||||
| 448 | | ||||
| 449 | | ||||
| 413 | void FileProtocol::listDir(const QUrl &url) | 450 | void FileProtocol::listDir(const QUrl &url) | ||
| 414 | { | 451 | { | ||
| 415 | if (!isLocalFileSameHost(url)) { | 452 | if (!isLocalFileSameHost(url)) { | ||
| 416 | QUrl redir(url); | 453 | QUrl redir(url); | ||
| 417 | redir.setScheme(config()->readEntry("DefaultRemoteProtocol", "smb")); | 454 | redir.setScheme(config()->readEntry("DefaultRemoteProtocol", "smb")); | ||
| 418 | redirection(redir); | 455 | redirection(redir); | ||
| 419 | // qDebug() << "redirecting to " << redir; | 456 | // qDebug() << "redirecting to " << redir; | ||
| 420 | finished(); | 457 | finished(); | ||
| ▲ Show 20 Lines • Show All 80 Lines • ▼ Show 20 Line(s) | 537 | if (isSymLink) { | |||
| 501 | // for symlinks obey the UDSEntry contract and provide UDS_LINK_DEST | 538 | // for symlinks obey the UDSEntry contract and provide UDS_LINK_DEST | ||
| 502 | // even if we don't know the link dest (and DeleteJob doesn't care...) | 539 | // even if we don't know the link dest (and DeleteJob doesn't care...) | ||
| 503 | entry.insert(KIO::UDSEntry::UDS_LINK_DEST, QStringLiteral("Dummy Link Target")); | 540 | entry.insert(KIO::UDSEntry::UDS_LINK_DEST, QStringLiteral("Dummy Link Target")); | ||
| 504 | } | 541 | } | ||
| 505 | listEntry(entry); | 542 | listEntry(entry); | ||
| 506 | 543 | | |||
| 507 | } else { | 544 | } else { | ||
| 508 | if (createUDSEntry(filename, QByteArray(ep->d_name), entry, details)) { | 545 | if (createUDSEntry(filename, QByteArray(ep->d_name), entry, details)) { | ||
| 546 | #ifdef Q_OS_LINUX | ||||
| 547 | if (isNtfsHidden(filename)) { | ||||
| 548 | entry.insert(KIO::UDSEntry::UDS_HIDDEN, 1); | ||||
| 549 | } | ||||
| 550 | #endif | ||||
Why here? markg: Why here?
This should be done inside the createUDSEntry function (it's in file.cpp). | |||||
The logic behind this is that getxattr exists only on Linux (https://www.gnu.org/software/gnulib/manual/html_node/getxattr.html#getxattr) and file_unix.cpp is more Linux-y than file.cpp. If you think that I'm wrong, I can move it. rominf: The logic behind this is that `getxattr` exists only on Linux (https://www.gnu. | |||||
I "think" it belongs in createUDSEntry, but i'm not sure either. It would keep this else nice and clean. But i do see why you'd want to put it here... What's your take on this, @dfaure? markg: I "think" it belongs in createUDSEntry, but i'm not sure either. It would keep this else nice… | |||||
dfaure: Looks fine to me here, due to the file.cpp / file_unix.cpp split. | |||||
| 509 | listEntry(entry); | 551 | listEntry(entry); | ||
| 510 | } | 552 | } | ||
| 511 | } | 553 | } | ||
| 512 | } | 554 | } | ||
| 513 | 555 | | |||
| 514 | closedir(dp); | 556 | closedir(dp); | ||
| 515 | 557 | | |||
| 516 | // Restore the path | 558 | // Restore the path | ||
| ▲ Show 20 Lines • Show All 301 Lines • Show Last 20 Lines | |||||
This unconditional include sadly breaks the build at least on FreeBSD (see https://build.kde.org/view/Frameworks/job/Frameworks%20kio%20kf5-qt5%20FreeBSDQt5.9/164/console ) and other systems where this header is not part of the libc (?) headers.
So this needs to get some condition checking before using it. Compare other code doing that:
https://lxr.kde.org/search?_filestring=&_string=xattr.h&_casesensitive=1
While the xattr.h is already checked for in KIO in the FindACL.cmake file, an explicite check like
and passing HAVE_SYS_XATTR_H via config-kioslave-file.h.cmake into file_unix.cpp and checking the condition additionally to or instead of Q_OS_LINUX for the new code should solve this issue properly.