Differential D20096 Diff 55066 src/ioslaves/file/file.cpp

Changeset View

Standalone View

View Options

src/ioslaves/file/file.cpp

Show First 20 Lines • Show All 59 Lines • ▼ Show 20 Line(s)
60#include <kconfiggroup.h> 60​#include <kconfiggroup.h>
61#include <kshell.h> 61​#include <kshell.h>
62#include <kmountpoint.h> 62​#include <kmountpoint.h>
63#include <klocalizedstring.h> 63​#include <klocalizedstring.h>
64#include <QMimeDatabase> 64​#include <QMimeDatabase>
65#include <QStandardPaths> 65​#include <QStandardPaths>
66#include <QDataStream> 66​#include <QDataStream>
67 67​
68​#ifdef Q_OS_LINUX
pinoUnsubmitted
Done

no need for the Q_OS_LINUX check here

pino: no need for the Q_OS_LINUX check here
69​#include <errno.h>
70​#include <fcntl.h>
71​#include <sys/stat.h>
fvogtUnsubmitted
Done

This won't work as expected, USE_STATX is unconditionally defined.

fvogt: This won't work as expected, `USE_STATX` is unconditionally defined.
mevenAuthorUnsubmitted
Done

Thanks

meven: Thanks
72​#include <sys/sysmacros.h>
73​#endif
74​
pinoUnsubmitted
Done

TBH, instead of this static define, I'd do a proper cmake check (see ConfigureChecks.cmake, and config-kioslave-file.h.cmake in src/ioslaves/file).

pino: TBH, instead of this static define, I'd do a proper cmake check (see ConfigureChecks.cmake, and…
mevenAuthorUnsubmitted
Done

It was not too easy to do.

meven: It was not too easy to do.
68#if HAVE_VOLMGT 75​#if HAVE_VOLMGT
69#include <volmgt.h> 76​#include <volmgt.h>
70#include <sys/mnttab.h> 77​#include <sys/mnttab.h>
71#endif 78​#endif
72 79​
73#include <kdirnotify.h> 80​#include <kdirnotify.h>
74#include <ioslave_defaults.h> 81​#include <ioslave_defaults.h>
75 82​
▲ Show 20 Lines • Show All 757 Lines • ▼ Show 20 Line(s)837 if (it == mGroupcache.end()) {
833 if (name.isEmpty()) { 840​ if (name.isEmpty()) {
834 name = gid.toString(); 841​ name = gid.toString();
835 } 842​ }
836 it = mGroupcache.insert(gid, name); 843​ it = mGroupcache.insert(gid, name);
837 } 844​ }
838 return *it; 845​ return *it;
839} 846​}
840 847​
848​#ifdef STATX_BASIC_STATS
849​// statx syscall is available
850​
841bool FileProtocol::createUDSEntry(const QString &filename, const QByteArray &path, UDSEntry &entry, 851​bool FileProtocol::createUDSEntry(const QString &filename, const QByteArray &path, UDSEntry &entry,
842 short int details) 852​ short int details)
843{ 853​{
844 assert(entry.count() == 0); // by contract :-) 854​ assert(entry.count() == 0); // by contract :-)
845 entry.reserve(8); 855​ entry.reserve(8);
856​ entry.fastInsert(KIO::UDSEntry::UDS_NAME, filename);
857​
858​ mode_t type;
859​ mode_t access;
860​ bool isBrokenSymLink = false;
861​ long long size = 0LL;
862​#if HAVE_POSIX_ACL
863​ QByteArray targetPath = path;
864​#endif
865​ struct statx buff;
866​
867​ if (statx(AT_FDCWD, path.data(), 0, STATX_BASIC_STATS | STATX_BTIME, &buff) == 0) {
868​ if (details > 2) {
869​ entry.fastInsert(KIO::UDSEntry::UDS_DEVICE_ID, buff.stx_dev_major);
870​ entry.fastInsert(KIO::UDSEntry::UDS_INODE, buff.stx_ino);
871​ }
872​
873​ if ((buff.stx_mode & QT_STAT_MASK) == QT_STAT_LNK) {
anthonyfieroniUnsubmitted
Done

We can get all stat_xxx functions buf as reference, struct is not needed when you use C++.

anthonyfieroni: We can get all stat_xxx functions buf as reference, struct is not needed when you use C++.
mevenAuthorUnsubmitted
Done

Do you mean the struct keyword in the argument in "inline static uint16_t stat_mode(struct statx buf) { return buf.stx_mode; } " for instance ?

meven: Do you mean the struct keyword in the argument in "inline static uint16_t stat_mode(struct…
mevenAuthorUnsubmitted
Done

Unfortunately this is not possible here : statx is also a function, the compiler gets messed up when removing the struct keyword interpreting it as a function call.

/file/file.cpp:850:34: warning: inline variables are only available with -std=c++17 or -std=gnu++17

377   │  inline static uint16_t stat_mode(statx buf) { return buf.stx_mode; }
meven: Unfortunately this is not possible here : statx is also a function, the compiler gets messed up…
pinoUnsubmitted
Done

No, he means using a const& for the argument, e.g:

inline static uint16_t stat_mode(struct statx &buf) { return buf.stx_mode; }
pino: No, he means using a const& for the argument, e.g: ```lang=c++ inline static uint16_t stat_mode…
mevenAuthorUnsubmitted
Done

Thanks

meven: Thanks
mevenAuthorUnsubmitted
Done

@pino
No, he means using a const& for the argument, e.g:

I think he meant both.

meven: > @pino > No, he means using a const& for the argument, e.g: I think he meant both.
874​
875​ // Use readlink on Unix because symLinkTarget turns relative targets into absolute (#352927)
876​ const __uint64_t lowerLimit = 1;
877​ const __uint64_t upperLimit = 1024;
878​ size_t bufferSize = qBound(lowerLimit, buff.stx_size, upperLimit);
brunsUnsubmitted
Done

You should start with a more sensible size for lowerLimit, e.g. 256, maybe more - the scope is local, i.e. any excess size is hardly relevant, and allocating 1 and 256 byte cost the same.

lowerLimit is not used below, I think you don't have to name it.

You should use (buff.stx_size + 1), as the size is without trailing null byte.

bruns: You should start with a more sensible size for lowerLimit, e.g. 256, maybe more - the scope is…
mevenAuthorUnsubmitted
Done

I have 256 as I'd rather allocate the minimal size without adding cost here.

meven: I have 256 as I'd rather allocate the minimal size without adding cost here.
879​ QByteArray linkTargetBuffer;
880​ linkTargetBuffer.resize(bufferSize);
881​ while (true) {
882​ ssize_t n = readlink(path.constData(), linkTargetBuffer.data(), bufferSize);
883​ if (n < 0 && errno != ERANGE) {
884​ qCWarning(KIO_FILE) << "readlink failed!" << path;
885​ return false;
886​ } else if (n > 0 && static_cast<size_t>(n) != bufferSize) {
887​ linkTargetBuffer.truncate(n);
888​ break;
889​ }
890​ bufferSize *= 2;
891​ linkTargetBuffer.resize(bufferSize);
892​ }
893​ const QString linkTarget = QFile::decodeName(linkTargetBuffer);
894​ entry.fastInsert(KIO::UDSEntry::UDS_LINK_DEST, linkTarget);
brunsUnsubmitted
Done

This can be up to 12 now, if I counted correctly. Conditionalize this on details.

bruns: This can be up to 12 now, if I counted correctly. Conditionalize this on details.
mevenAuthorUnsubmitted
Done

This code was just copied, but I will be happy to take care of this along the way.

meven: This code was just copied, but I will be happy to take care of this along the way.
846 895​
896​ // A symlink -> follow it only if details>1
897​ if (details > 1) {
898​ if (statx(AT_FDCWD, path.constData(), 0, STATX_BASIC_STATS | STATX_BTIME, &buff) == -1) {
899​ isBrokenSymLink = true;
900​ } else {
901​#if HAVE_POSIX_ACL
902​ // valid symlink, will get the ACLs of the destination
903​ targetPath = linkTargetBuffer;
904​#endif
905​ }
906​ }
907​ }
908​ } else {
brunsUnsubmitted
Done

This is wrong in case someone uses details > 3, should be case 0: reserve(5), case 3: default: reserve(15) .
all checks below do e.g if (details > 2), so handle 5 the same as 3.

bruns: This is wrong in case someone uses details > 3, should be `case 0: reserve(5)`, `case 3…
mevenAuthorUnsubmitted
Done

Thanks

meven: Thanks
909​ // qCWarning(KIO_FILE) << "statx didn't work on " << path.data();
910​ return false;
911​ }
912​
913​ if (isBrokenSymLink) {
914​ // It is a link pointing to nowhere
915​ type = S_IFMT - 1;
916​ access = S_IRWXU | S_IRWXG | S_IRWXO;
917​ size = 0LL;
918​ } else {
919​ type = buff.stx_mode & S_IFMT; // extract file type
920​ access = buff.stx_mode & 07777; // extract permissions
921​ size = buff.stx_size;
922​ }
923​
924​ entry.fastInsert(KIO::UDSEntry::UDS_FILE_TYPE, type);
925​ entry.fastInsert(KIO::UDSEntry::UDS_ACCESS, access);
926​ entry.fastInsert(KIO::UDSEntry::UDS_SIZE, size);
927​
928​#if HAVE_POSIX_ACL
929​ if (details > 1) {
930​ /* Append an atom indicating whether the file has extended acl information
931​ * and if withACL is specified also one with the acl itself. If it's a directory
932​ * and it has a default ACL, also append that. */
933​ appendACLAtoms(targetPath, entry, type);
934​ }
935​#endif
936​
937​ if (details > 0) {
938​ entry.fastInsert(KIO::UDSEntry::UDS_USER, getUserName(KUserId(buff.stx_uid)));
939​ entry.fastInsert(KIO::UDSEntry::UDS_GROUP, getGroupName(KGroupId(buff.stx_gid)));
940​
941​ entry.fastInsert(KIO::UDSEntry::UDS_MODIFICATION_TIME, buff.stx_mtime.tv_sec);
942​ entry.fastInsert(KIO::UDSEntry::UDS_ACCESS_TIME, buff.stx_atime.tv_sec);
943​ entry.fastInsert(KIO::UDSEntry::UDS_CREATION_TIME, buff.stx_btime.tv_sec);
944​ }
945​
946​ // Note: buff.st_ctime isn't the creation time !
947​ // We made that mistake for KDE 2.0, but it's in fact the
948​ // "file status" change time, which we don't care about.
949​ // For FreeBSD and NetBSD, use st_birthtime. For OpenBSD,
950​ // use __st_birthtime.
951​
952​ return true;
953​}
954​#else
955​
brunsUnsubmitted
Done

This comment is no longer relevant.

bruns: This comment is no longer relevant.
956​bool FileProtocol::createUDSEntry(const QString &filename, const QByteArray &path, UDSEntry &entry,
957​ short int details)
958​{
959​ assert(entry.count() == 0); // by contract :-)
960​ entry.reserve(8);
brunsUnsubmitted
Done

switch (details) { ...}

bruns: `switch (details) { ...}`
brunsUnsubmitted
Done

You are off by 1 for dev/inode, these are only added with details >= 3.

details >= 2 is ACL data. As the ACL are commonly empty, add +0.

Handle 0 explicitly, and make 3 (>=3) the default, to match the remaining code.

bruns: You are off by 1 for dev/inode, these are only added with details >= 3. details >= 2 is ACL…
847 entry.fastInsert(KIO::UDSEntry::UDS_NAME, filename); 961​ entry.fastInsert(KIO::UDSEntry::UDS_NAME, filename);
848 962​
849 mode_t type; 963​ mode_t type;
850 mode_t access; 964​ mode_t access;
851 bool isBrokenSymLink = false; 965​ bool isBrokenSymLink = false;
852 long long size = 0LL; 966​ long long size = 0LL;
853#if HAVE_POSIX_ACL 967​#if HAVE_POSIX_ACL
854 QByteArray targetPath = path; 968​ QByteArray targetPath = path;
Show All 10 Lines972 if (QT_LSTAT(path.data(), &buff) == 0) {
865 if ((buff.st_mode & QT_STAT_MASK) == QT_STAT_LNK) { 979​ if ((buff.st_mode & QT_STAT_MASK) == QT_STAT_LNK) {
866 980​
867#ifdef Q_OS_WIN 981​#ifdef Q_OS_WIN
868 const QString linkTarget = QFile::symLinkTarget(QFile::decodeName(path)); 982​ const QString linkTarget = QFile::symLinkTarget(QFile::decodeName(path));
869#else 983​#else
870 // Use readlink on Unix because symLinkTarget turns relative targets into absolute (#352927) 984​ // Use readlink on Unix because symLinkTarget turns relative targets into absolute (#352927)
871 const off_t lowerLimit = 1; 985​ const off_t lowerLimit = 1;
872 const off_t upperLimit = 1024; 986​ const off_t upperLimit = 1024;
873 size_t bufferSize = qBound(lowerLimit, buff.st_size, upperLimit); 987​ size_t bufferSize = qBound(lowerLimit, buff.st_size, upperLimit);
brunsUnsubmitted
Done

stat_size + 1. Otherwise, you will always take the n == bufferSize path below at least once.

bruns: stat_size + 1. Otherwise, you will always take the `n == bufferSize` path below at least once.
874 QByteArray linkTargetBuffer; 988​ QByteArray linkTargetBuffer;
875 linkTargetBuffer.resize(bufferSize); 989​ linkTargetBuffer.resize(bufferSize);
brunsUnsubmitted
Done

statx.stx_size is __u64, and readlink uses (unsigned) size_t for the buffer size.

bruns: statx.stx_size is __u64, and readlink uses (unsigned) size_t for the buffer size.
mevenAuthorUnsubmitted
Done

Let me know if I have handled this correctly.

meven: Let me know if I have handled this correctly.
876 while (true) { 990​ while (true) {
877 ssize_t n = readlink(path.constData(), linkTargetBuffer.data(), bufferSize); 991​ ssize_t n = readlink(path.constData(), linkTargetBuffer.data(), bufferSize);
878 if (n < 0 && errno != ERANGE) { 992​ if (n < 0 && errno != ERANGE) {
879 qCWarning(KIO_FILE) << "readlink failed!" << path; 993​ qCWarning(KIO_FILE) << "readlink failed!" << path;
880 return false; 994​ return false;
881 } else if (n > 0 && static_cast<size_t>(n) != bufferSize) { 995​ } else if (n > 0 && static_cast<size_t>(n) != bufferSize) {
882 linkTargetBuffer.truncate(n); 996​ linkTargetBuffer.truncate(n);
883 break; 997​ break;
884 } 998​ }
885 bufferSize *= 2; 999​ bufferSize *= 2;
886 linkTargetBuffer.resize(bufferSize); 1000​ linkTargetBuffer.resize(bufferSize);
887 } 1001​ }
brunsUnsubmitted
Not Done

missing space, size + 1

bruns: missing space, `size + 1`
888 const QString linkTarget = QFile::decodeName(linkTargetBuffer); 1002​ const QString linkTarget = QFile::decodeName(linkTargetBuffer);
889#endif 1003​#endif
890 entry.fastInsert(KIO::UDSEntry::UDS_LINK_DEST, linkTarget); 1004​ entry.fastInsert(KIO::UDSEntry::UDS_LINK_DEST, linkTarget);
891 1005​
892 // A symlink -> follow it only if details>1 1006​ // A symlink -> follow it only if details>1
893 if (details > 1) { 1007​ if (details > 1) {
894 if (QT_STAT(path.constData(), &buff) == -1) { 1008​ if (QT_STAT(path.constData(), &buff) == -1) {
895 isBrokenSymLink = true; 1009​ isBrokenSymLink = true;
▲ Show 20 Lines • Show All 54 Lines • ▼ Show 20 Line(s)1056#ifdef st_birthtime
950 } 1064​ }
951#elif defined __st_birthtime 1065​#elif defined __st_birthtime
952 /* As above, but OpenBSD calls it slightly differently. */ 1066​ /* As above, but OpenBSD calls it slightly differently. */
953 if (buff.__st_birthtime > 0) { 1067​ if (buff.__st_birthtime > 0) {
954 entry.fastInsert(KIO::UDSEntry::UDS_CREATION_TIME, buff.__st_birthtime); 1068​ entry.fastInsert(KIO::UDSEntry::UDS_CREATION_TIME, buff.__st_birthtime);
955 } 1069​ }
956#endif 1070​#endif
957 } 1071​ }
958 1072​
fvogtUnsubmitted
Done

This check seems to be wrong with me - there can be files with legitimate zero tv_nsec.

Use buff.stx_mask & STATX_BTIME instead.

fvogt: This check seems to be wrong with me - there can be files with legitimate zero `tv_nsec`. Use…
mevenAuthorUnsubmitted
Done

Thanks for the feedback, I read about stx_mask after writing this.

meven: Thanks for the feedback, I read about stx_mask after writing this.
959 // Note: buff.st_ctime isn't the creation time ! 1073​ // Note: buff.st_ctime isn't the creation time !
960 // We made that mistake for KDE 2.0, but it's in fact the 1074​ // We made that mistake for KDE 2.0, but it's in fact the
961 // "file status" change time, which we don't care about. 1075​ // "file status" change time, which we don't care about.
962 // For FreeBSD and NetBSD, use st_birthtime. For OpenBSD, 1076​ // For FreeBSD and NetBSD, use st_birthtime. For OpenBSD,
963 // use __st_birthtime. 1077​ // use __st_birthtime.
964 1078​
965 return true; 1079​ return true;
966} 1080​}
1081​#endif
967 1082​
968void FileProtocol::special(const QByteArray &data) 1083​void FileProtocol::special(const QByteArray &data)
969{ 1084​{
970 int tmp; 1085​ int tmp;
971 QDataStream stream(data); 1086​ QDataStream stream(data);
972 1087​
973 stream >> tmp; 1088​ stream >> tmp;
974 switch (tmp) { 1089​ switch (tmp) {
▲ Show 20 Lines • Show All 497 Lines • Show Last 20 Lines