Differential D20096 Diff 55967 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​#if HAVE_STATX
pinoUnsubmitted
Done

no need for the Q_OS_LINUX check here

pino: no need for the Q_OS_LINUX check here
69​#include <sys/stat.h>
70​#endif
71​
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
68#if HAVE_VOLMGT 72​#if HAVE_VOLMGT
69#include <volmgt.h> 73​#include <volmgt.h>
70#include <sys/mnttab.h> 74​#include <sys/mnttab.h>
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.
71#endif 75​#endif
72 76​
73#include <kdirnotify.h> 77​#include <kdirnotify.h>
74#include <ioslave_defaults.h> 78​#include <ioslave_defaults.h>
75 79​
76Q_LOGGING_CATEGORY(KIO_FILE, "kf5.kio.kio_file") 80​Q_LOGGING_CATEGORY(KIO_FILE, "kf5.kio.kio_file")
77 81​
78// Pseudo plugin class to embed meta data 82​// Pseudo plugin class to embed meta data
▲ Show 20 Lines • Show All 754 Lines • ▼ Show 20 Line(s)834 if (it == mGroupcache.end()) {
833 if (name.isEmpty()) { 837​ if (name.isEmpty()) {
834 name = gid.toString(); 838​ name = gid.toString();
835 } 839​ }
836 it = mGroupcache.insert(gid, name); 840​ it = mGroupcache.insert(gid, name);
837 } 841​ }
838 return *it; 842​ return *it;
839} 843​}
840 844​
845​#if HAVE_STATX
846​// statx syscall is available
847​inline int STAT(const char* path, struct statx * buff) {
848​ return statx(AT_FDCWD, path, AT_SYMLINK_NOFOLLOW, STATX_BASIC_STATS | STATX_BTIME, buff);
849​}
850​inline static uint16_t stat_mode(struct statx &buf) { return buf.stx_mode; }
851​inline static uint32_t stat_dev(struct statx &buf) { return buf.stx_dev_major; }
852​inline static uint64_t stat_ino(struct statx &buf) { return buf.stx_ino; }
853​inline static uint64_t stat_size(struct statx &buf) { return buf.stx_size; }
854​inline static uint32_t stat_uid(struct statx &buf) { return buf.stx_uid; }
855​inline static uint32_t stat_gid(struct statx &buf) { return buf.stx_gid; }
856​inline static int64_t stat_atime(struct statx &buf) { return buf.stx_atime.tv_sec; }
857​inline static uint64_t stat_mtime(struct statx &buf) { return buf.stx_mtime.tv_sec; }
858​#else
859​// regular stat struct
860​inline int STAT(const char* path, QT_STATBUF * buff) {
861​ return QT_LSTAT(path, buff);
862​}
863​inline static mode_t stat_mode(QT_STATBUF &buf) { return buf.st_mode; }
864​inline static dev_t stat_dev(QT_STATBUF &buf) { return buf.st_dev; }
865​inline static ino_t stat_ino(QT_STATBUF &buf) { return buf.st_ino; }
866​inline static off_t stat_size(QT_STATBUF &buf) { return buf.st_size; }
867​inline static uid_t stat_uid(QT_STATBUF &buf) { return buf.st_uid; }
868​inline static gid_t stat_gid(QT_STATBUF &buf) { return buf.st_gid; }
869​inline static time_t stat_atime(QT_STATBUF &buf) { return buf.st_atime; }
870​inline static time_t stat_mtime(QT_STATBUF &buf) { return buf.st_mtime; }
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.
871​#endif
872​
841bool FileProtocol::createUDSEntry(const QString &filename, const QByteArray &path, UDSEntry &entry, 873​bool FileProtocol::createUDSEntry(const QString &filename, const QByteArray &path, UDSEntry &entry,
842 short int details) 874​ short int details)
843{ 875​{
844 assert(entry.count() == 0); // by contract :-) 876​ assert(entry.count() == 0); // by contract :-)
845 entry.reserve(8); 877​ switch (details) {
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 878​ case 0:
879​ // filename, access, type, size, linkdest
880​ entry.reserve(5);
881​ break;
882​ case 1:
883​ // uid, gid, atime, mtime, btime
884​ entry.reserve(10);
885​ break;
886​ case 2:
887​ // acl data
888​ entry.reserve(13);
889​ break;
890​ default: // case details > 2
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
891​ // dev, inode
892​ entry.reserve(15);
893​ break;
894​ }
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); 895​ entry.fastInsert(KIO::UDSEntry::UDS_NAME, filename);
848 896​
849 mode_t type; 897​ mode_t type;
850 mode_t access; 898​ mode_t access;
851 bool isBrokenSymLink = false; 899​ bool isBrokenSymLink = false;
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.
852 long long size = 0LL; 900​ signed long long size = 0LL;
853#if HAVE_POSIX_ACL 901​#if HAVE_POSIX_ACL
854 QByteArray targetPath = path; 902​ QByteArray targetPath = path;
855#endif 903​#endif
904​
905​#if HAVE_STATX
906​ // statx syscall is available
907​ struct statx buff;
908​#else
856 QT_STATBUF buff; 909​ QT_STATBUF buff;
910​#endif
857 911​
858 if (QT_LSTAT(path.data(), &buff) == 0) { 912​ if (STAT(path.data(), &buff) == 0) {
859 913​
860 if (details > 2) { 914​ if (details > 2) {
861 entry.fastInsert(KIO::UDSEntry::UDS_DEVICE_ID, buff.st_dev); 915​ entry.fastInsert(KIO::UDSEntry::UDS_DEVICE_ID, stat_dev(buff));
862 entry.fastInsert(KIO::UDSEntry::UDS_INODE, buff.st_ino); 916​ entry.fastInsert(KIO::UDSEntry::UDS_INODE, stat_ino(buff));
863 } 917​ }
864 918​
865 if ((buff.st_mode & QT_STAT_MASK) == QT_STAT_LNK) { 919​ if ((stat_mode(buff) & QT_STAT_MASK) == QT_STAT_LNK) {
866 920​
867#ifdef Q_OS_WIN 921​#ifdef Q_OS_WIN
868 const QString linkTarget = QFile::symLinkTarget(QFile::decodeName(path)); 922​ const QString linkTarget = QFile::symLinkTarget(QFile::decodeName(path));
869#else 923​#else
870 // Use readlink on Unix because symLinkTarget turns relative targets into absolute (#352927) 924​ // Use readlink on Unix because symLinkTarget turns relative targets into absolute (#352927)
871 const off_t lowerLimit = 1; 925​ #if HAVE_STATX
872 const off_t upperLimit = 1024; 926​ size_t lowerBound = 256;
873 size_t bufferSize = qBound(lowerLimit, buff.st_size, upperLimit); 927​ size_t higherBound = 1024;
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.
928​ uint64_t s = stat_size(buff);
929​ if (s > SIZE_MAX) {
930​ qCWarning(KIO_FILE) << "file size bigger than SIZE_MAX, too big for readlink use!" << path;
931​ return false;
932​ }
933​ size_t size = (size_t) s;
934​ #else
935​ off_t lowerBound = 256;
936​ off_t higherBound = 1024;
937​ off_t size = stat_size(buff);
brunsUnsubmitted
Done

This comment is no longer relevant.

bruns: This comment is no longer relevant.
938​ #endif
939​ auto bufferSize = qBound(lowerBound, size +1, higherBound);
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.
brunsUnsubmitted
Not Done

missing space, size + 1

bruns: missing space, `size + 1`
874 QByteArray linkTargetBuffer; 940​ QByteArray linkTargetBuffer;
875 linkTargetBuffer.resize(bufferSize); 941​ linkTargetBuffer.resize(bufferSize);
876 while (true) { 942​ while (true) {
877 ssize_t n = readlink(path.constData(), linkTargetBuffer.data(), bufferSize); 943​ ssize_t n = readlink(path.constData(), linkTargetBuffer.data(), bufferSize);
878 if (n < 0 && errno != ERANGE) { 944​ if (n < 0 && errno != ERANGE) {
879 qCWarning(KIO_FILE) << "readlink failed!" << path; 945​ qCWarning(KIO_FILE) << "readlink failed!" << path;
880 return false; 946​ return false;
881 } else if (n > 0 && static_cast<size_t>(n) != bufferSize) { 947​ } else if (n > 0 && static_cast<size_t>(n) != bufferSize) {
948​ // the buffer was not filled in the last iteration
949​ // we are finished reading, break the loop
882 linkTargetBuffer.truncate(n); 950​ linkTargetBuffer.truncate(n);
883 break; 951​ break;
884 } 952​ }
885 bufferSize *= 2; 953​ bufferSize *= 2;
886 linkTargetBuffer.resize(bufferSize); 954​ linkTargetBuffer.resize(bufferSize);
887 } 955​ }
888 const QString linkTarget = QFile::decodeName(linkTargetBuffer); 956​ const QString linkTarget = QFile::decodeName(linkTargetBuffer);
889#endif 957​#endif
890 entry.fastInsert(KIO::UDSEntry::UDS_LINK_DEST, linkTarget); 958​ entry.fastInsert(KIO::UDSEntry::UDS_LINK_DEST, linkTarget);
891 959​
892 // A symlink -> follow it only if details>1 960​ // A symlink -> follow it only if details>1
893 if (details > 1) { 961​ if (details > 1) {
894 if (QT_STAT(path.constData(), &buff) == -1) { 962​ if (STAT(path.constData(), &buff) == -1) {
895 isBrokenSymLink = true; 963​ isBrokenSymLink = true;
896 } else { 964​ } else {
897#if HAVE_POSIX_ACL 965​#if HAVE_POSIX_ACL
898 // valid symlink, will get the ACLs of the destination 966​ // valid symlink, will get the ACLs of the destination
899 targetPath = linkTargetBuffer; 967​ targetPath = linkTargetBuffer;
900#endif 968​#endif
901 } 969​ }
902 } 970​ }
903 } 971​ }
904 } else { 972​ } else {
905 // qCWarning(KIO_FILE) << "lstat didn't work on " << path.data(); 973​ // qCWarning(KIO_FILE) << "lstat didn't work on " << path.data();
906 return false; 974​ return false;
907 } 975​ }
908 976​
909 if (isBrokenSymLink) { 977​ if (isBrokenSymLink) {
910 // It is a link pointing to nowhere 978​ // It is a link pointing to nowhere
911 type = S_IFMT - 1; 979​ type = S_IFMT - 1;
912 access = S_IRWXU | S_IRWXG | S_IRWXO; 980​ access = S_IRWXU | S_IRWXG | S_IRWXO;
913 size = 0LL; 981​ size = 0LL;
914 } else { 982​ } else {
915 type = buff.st_mode & S_IFMT; // extract file type 983​ type = stat_mode(buff) & S_IFMT; // extract file type
916 access = buff.st_mode & 07777; // extract permissions 984​ access = stat_mode(buff) & 07777; // extract permissions
917 size = buff.st_size; 985​ size = stat_size(buff);
918 } 986​ }
919 987​
920 entry.fastInsert(KIO::UDSEntry::UDS_FILE_TYPE, type); 988​ entry.fastInsert(KIO::UDSEntry::UDS_FILE_TYPE, type);
921 entry.fastInsert(KIO::UDSEntry::UDS_ACCESS, access); 989​ entry.fastInsert(KIO::UDSEntry::UDS_ACCESS, access);
922 entry.fastInsert(KIO::UDSEntry::UDS_SIZE, size); 990​ entry.fastInsert(KIO::UDSEntry::UDS_SIZE, size);
923 991​
924#if HAVE_POSIX_ACL 992​#if HAVE_POSIX_ACL
925 if (details > 1) { 993​ if (details > 1) {
926 /* Append an atom indicating whether the file has extended acl information 994​ /* Append an atom indicating whether the file has extended acl information
927 * and if withACL is specified also one with the acl itself. If it's a directory 995​ * and if withACL is specified also one with the acl itself. If it's a directory
928 * and it has a default ACL, also append that. */ 996​ * and it has a default ACL, also append that. */
929 appendACLAtoms(targetPath, entry, type); 997​ appendACLAtoms(targetPath, entry, type);
930 } 998​ }
931#endif 999​#endif
932 1000​
933 if (details > 0) { 1001​ if (details > 0) {
934 entry.fastInsert(KIO::UDSEntry::UDS_MODIFICATION_TIME, buff.st_mtime); 1002​ entry.fastInsert(KIO::UDSEntry::UDS_MODIFICATION_TIME, stat_mtime(buff));
1003​ entry.fastInsert(KIO::UDSEntry::UDS_ACCESS_TIME, stat_atime(buff));
935#ifndef Q_OS_WIN 1004​#ifndef Q_OS_WIN
936 entry.fastInsert(KIO::UDSEntry::UDS_USER, getUserName(KUserId(buff.st_uid))); 1005​ entry.fastInsert(KIO::UDSEntry::UDS_USER, getUserName(KUserId(stat_uid(buff))));
937 entry.fastInsert(KIO::UDSEntry::UDS_GROUP, getGroupName(KGroupId(buff.st_gid))); 1006​ entry.fastInsert(KIO::UDSEntry::UDS_GROUP, getGroupName(KGroupId(stat_gid(buff))));
938#else 1007​#else
939#pragma message("TODO: st_uid and st_gid are always zero, use GetSecurityInfo to find the owner") 1008​#pragma message("TODO: st_uid and st_gid are always zero, use GetSecurityInfo to find the owner")
940#endif 1009​#endif
941 entry.fastInsert(KIO::UDSEntry::UDS_ACCESS_TIME, buff.st_atime); 1010​
942#ifdef st_birthtime 1011​#ifdef st_birthtime
943 /* For example FreeBSD's and NetBSD's stat contains a field for 1012​ /* For example FreeBSD's and NetBSD's stat contains a field for
944 * the inode birth time: st_birthtime 1013​ * the inode birth time: st_birthtime
945 * This however only works on UFS and ZFS, and not, on say, NFS. 1014​ * This however only works on UFS and ZFS, and not, on say, NFS.
946 * Instead of setting a bogus fallback like st_mtime, only use 1015​ * Instead of setting a bogus fallback like st_mtime, only use
947 * it if it is greater than 0. */ 1016​ * it if it is greater than 0. */
948 if (buff.st_birthtime > 0) { 1017​ if (buff.st_birthtime > 0) {
949 entry.fastInsert(KIO::UDSEntry::UDS_CREATION_TIME, buff.st_birthtime); 1018​ entry.fastInsert(KIO::UDSEntry::UDS_CREATION_TIME, buff.st_birthtime);
950 } 1019​ }
951#elif defined __st_birthtime 1020​#elif defined __st_birthtime
952 /* As above, but OpenBSD calls it slightly differently. */ 1021​ /* As above, but OpenBSD calls it slightly differently. */
953 if (buff.__st_birthtime > 0) { 1022​ if (buff.__st_birthtime > 0) {
954 entry.fastInsert(KIO::UDSEntry::UDS_CREATION_TIME, buff.__st_birthtime); 1023​ entry.fastInsert(KIO::UDSEntry::UDS_CREATION_TIME, buff.__st_birthtime);
955 } 1024​ }
1025​#elif HAVE_STATX
1026​ /* And linux version using statx syscall */
1027​ if (buff.stx_mask & STATX_BTIME) {
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.
1028​ entry.fastInsert(KIO::UDSEntry::UDS_CREATION_TIME, buff.stx_btime.tv_sec);
1029​ }
956#endif 1030​#endif
957 } 1031​ }
958 1032​
959 // Note: buff.st_ctime isn't the creation time ! ​
960 // 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. ​
962 // For FreeBSD and NetBSD, use st_birthtime. For OpenBSD, ​
963 // use __st_birthtime. ​
964 ​
965 return true; 1033​ return true;
966} 1034​}
967 1035​
968void FileProtocol::special(const QByteArray &data) 1036​void FileProtocol::special(const QByteArray &data)
969{ 1037​{
970 int tmp; 1038​ int tmp;
971 QDataStream stream(data); 1039​ QDataStream stream(data);
972 1040​
▲ Show 20 Lines • Show All 499 Lines • Show Last 20 Lines