D13898: Use non deprecated fastInsert in file.cpp (first of many to come)
This revision was automatically updated to reflect the committed changes. Closed by commit R241:6b452ae9892d: Use non deprecated fastInsert in file.cpp (first of many to come) (authored by jtamate). REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D13898?vs=37427=37458 REVISION DETAIL https://phabricator.kde.org/D13898 AFFECTED FILES src/ioslaves/file/file.cpp To: jtamate, dfaure, #frameworks Cc: bruns, kde-frameworks-devel, michaelh, ngraham
D13898: Use non deprecated fastInsert in file.cpp (first of many to come)
dfaure accepted this revision. dfaure added a comment. This revision is now accepted and ready to land. Thanks! REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D13898 To: jtamate, dfaure, #frameworks Cc: bruns, kde-frameworks-devel, michaelh, ngraham
D13898: Use non deprecated fastInsert in file.cpp (first of many to come)
jtamate updated this revision to Diff 37427. jtamate added a comment. Use st_birthtime or __st_birthtime but not both. REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D13898?vs=37424=37427 REVISION DETAIL https://phabricator.kde.org/D13898 AFFECTED FILES src/ioslaves/file/file.cpp To: jtamate, dfaure, #frameworks Cc: bruns, kde-frameworks-devel, michaelh, ngraham
D13898: Use non deprecated fastInsert in file.cpp (first of many to come)
dfaure added a comment. The comments indicate that this isn't supposed to happen, but just to make sure, you can use #elif on line 941/942. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D13898 To: jtamate, dfaure, #frameworks Cc: bruns, kde-frameworks-devel, michaelh, ngraham
D13898: Use non deprecated fastInsert in file.cpp (first of many to come)
jtamate updated this revision to Diff 37424. jtamate edited the summary of this revision. jtamate added a comment. Fixed targetPath of links. Question: If there any chance that st_birthtime and __st_birthtime are both present in the same OS? If affirmative, the entry of __st_birthtime sould be filled with replace(). REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D13898?vs=37240=37424 REVISION DETAIL https://phabricator.kde.org/D13898 AFFECTED FILES src/ioslaves/file/file.cpp To: jtamate, dfaure, #frameworks Cc: bruns, kde-frameworks-devel, michaelh, ngraham
D13898: Use non deprecated fastInsert in file.cpp (first of many to come)
dfaure requested changes to this revision. dfaure added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > bruns wrote in file.cpp:889 > isn't this the same as linkTargetBuffer? Yes, and in fact linkTargetBuffer is more correct. toLocal8Bit() is incorrect, QFile::encodeName/decodeName must be used for QByteArray<->QString conversions for filenames. > bruns wrote in file.cpp:864 > The comment does not solve the issue, neither does it help to clarify > anything ... > > `buffersize` has to be larger than buff.st_size, otherwise there is no > possibility to diffentiate the 'fits exactly' and 'was truncated' cases, both > will return `n == buffersize`. > > the correct fix is to use: > > // Add one to the size, to be able to detect truncation - > // in case n == bufferSize, truncation *may* have occured > size_t bufferSize = qBound(lowerLimit, buff.st_size + 1, 1024); Given that this fix is completely unrelated to this commit, it should rather be separated into a different commit... REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D13898 To: jtamate, dfaure, #frameworks Cc: bruns, kde-frameworks-devel, michaelh, ngraham
D13898: Use non deprecated fastInsert in file.cpp (first of many to come)
bruns added inline comments. INLINE COMMENTS > file.cpp:889 > +#if HAVE_POSIX_ACL > +targetPath = linkTarget.toLocal8Bit(); > +#endif isn't this the same as linkTargetBuffer? REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D13898 To: jtamate, dfaure, #frameworks Cc: bruns, kde-frameworks-devel, michaelh, ngraham
D13898: Use non deprecated fastInsert in file.cpp (first of many to come)
jtamate updated this revision to Diff 37240. jtamate marked 2 inline comments as done. jtamate added a comment. Incorporated Stephan code. Fill again the extra data like User/Group. I don't see any change and I don't want to break anything more. REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D13898?vs=37216=37240 REVISION DETAIL https://phabricator.kde.org/D13898 AFFECTED FILES src/ioslaves/file/file.cpp To: jtamate, dfaure, #frameworks Cc: bruns, kde-frameworks-devel, michaelh, ngraham
D13898: Use non deprecated fastInsert in file.cpp (first of many to come)
bruns added inline comments. INLINE COMMENTS > jtamate wrote in file.cpp:864 > According to the man page (2+3), readlink() //does not append a null byte to > buf//. > > It will (silently) truncate the contents (to a length of bufsiz > characters), in case the buffer is too small to hold all of the contents. The comment does not solve the issue, neither does it help to clarify anything ... `buffersize` has to be larger than buff.st_size, otherwise there is no possibility to diffentiate the 'fits exactly' and 'was truncated' cases, both will return `n == buffersize`. the correct fix is to use: // Add one to the size, to be able to detect truncation - // in case n == bufferSize, truncation *may* have occured size_t bufferSize = qBound(lowerLimit, buff.st_size + 1, 1024); REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D13898 To: jtamate, dfaure, #frameworks Cc: bruns, kde-frameworks-devel, michaelh, ngraham
D13898: Use non deprecated fastInsert in file.cpp (first of many to come)
jtamate added inline comments. INLINE COMMENTS > bruns wrote in file.cpp:864 > This is broken (although not new). > > `buff.st_size` is the size of the target name **without** null byte. > readlink(..., .., bufferSize) thus will typically read exactly bufferSize > bytes, thus n == bufferSize > As a result, the 'good' case in the branches below will not pass, and a > resize(bufferSize *= 2) and another readlink will happen. According to the man page (2+3), readlink() //does not append a null byte to buf//. It will (silently) truncate the contents (to a length of bufsiz characters), in case the buffer is too small to hold all of the contents. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D13898 To: jtamate, dfaure, #frameworks Cc: bruns, kde-frameworks-devel, michaelh, ngraham
D13898: Use non deprecated fastInsert in file.cpp (first of many to come)
jtamate updated this revision to Diff 37216. jtamate marked 3 inline comments as done. jtamate added a comment. Added a comment: // readlink doesn't append a null byte to linkTargetBuffer.data() Using target path to read ACL in a non broken symbolic link. Do not fill remaining data in case of broken link, doesn't affect dolphin. REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D13898?vs=37184=37216 REVISION DETAIL https://phabricator.kde.org/D13898 AFFECTED FILES src/ioslaves/file/file.cpp To: jtamate, dfaure, #frameworks Cc: bruns, kde-frameworks-devel, michaelh, ngraham
D13898: Use non deprecated fastInsert in file.cpp (first of many to come)
bruns added inline comments.
INLINE COMMENTS
> file.cpp:909
> * and it has a default ACL, also append that. */
> appendACLAtoms(path, entry, type);
> }
For UDS_ACCESS, _USER, _GROUP, we follow the symlink (i.e use the corresponding
buff), for ACL we do not?
> file.cpp:913
>
> -notype:
> if (details > 0) {
> +entry.fastInsert(KIO::UDSEntry::UDS_MODIFICATION_TIME,
> buff.st_mtime);
all the fields below are pointless in case of isBrokenSymlink, buff may contain
anything after the failed stat call.
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D13898
To: jtamate, dfaure, #frameworks
Cc: bruns, kde-frameworks-devel, michaelh, ngraham
D13898: Use non deprecated fastInsert in file.cpp (first of many to come)
bruns added inline comments.
INLINE COMMENTS
> file.cpp:864
> while (true) {
> ssize_t n = readlink(path.constData(),
> linkTargetBuffer.data(), bufferSize);
> if (n < 0 && errno != ERANGE) {
This is broken (although not new).
`buff.st_size` is the size of the target name **without** null byte.
readlink(..., .., bufferSize) thus will typically read exactly bufferSize
bytes, thus n == bufferSize
As a result, the 'good' case in the branches below will not pass, and a
resize(bufferSize *= 2) and another readlink will happen.
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D13898
To: jtamate, dfaure, #frameworks
Cc: bruns, kde-frameworks-devel, michaelh, ngraham
D13898: Use non deprecated fastInsert in file.cpp (first of many to come)
dfaure accepted this revision. This revision is now accepted and ready to land. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D13898 To: jtamate, dfaure, #frameworks Cc: kde-frameworks-devel, michaelh, ngraham, bruns
D13898: Use non deprecated fastInsert in file.cpp (first of many to come)
jtamate updated this revision to Diff 37184. jtamate edited the summary of this revision. jtamate added a comment. Renamed isSymLink to isBrokenSymLink. REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D13898?vs=37178=37184 REVISION DETAIL https://phabricator.kde.org/D13898 AFFECTED FILES src/ioslaves/file/file.cpp To: jtamate, dfaure, #frameworks Cc: kde-frameworks-devel, michaelh, ngraham, bruns
D13898: Use non deprecated fastInsert in file.cpp (first of many to come)
dfaure added inline comments.
INLINE COMMENTS
> file.cpp:880
> // A symlink -> follow it only if details>1
> if (details > 1 && QT_STAT(path.constData(), ) == -1) {
> +isSymLink = true;
BTW the point is that we follow the link (by filling "buff" with this QT_STAT
call), but then we also check that call for errors: if it returned -1, then the
link is broken.
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D13898
To: jtamate, dfaure, #frameworks
Cc: kde-frameworks-devel, michaelh, ngraham, bruns
D13898: Use non deprecated fastInsert in file.cpp (first of many to come)
dfaure requested changes to this revision. dfaure added a comment. This revision now requires changes to proceed. Yes I guess broken symlinks can have acl informations themselves too, so I don't mind the acl code being called now and not before. INLINE COMMENTS > file.cpp:841 > mode_t access; > +bool isSymLink = false; > +long long size = 0LL; Call it isBrokenSymlink, it's only set to true for *broken* symlinks. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D13898 To: jtamate, dfaure, #frameworks Cc: kde-frameworks-devel, michaelh, ngraham, bruns
D13898: Use non deprecated fastInsert in file.cpp (first of many to come)
jtamate created this revision. jtamate added reviewers: dfaure, Frameworks. Restricted Application added a project: Frameworks. Restricted Application added a subscriber: kde-frameworks-devel. jtamate requested review of this revision. REVISION SUMMARY Avoid the goto using local variables. Use fastInsert instead of insert. NOTE: In this case the comments are confusing, is it a symbolic link or a broken symbolic link? NOTE: Does make sense to have ACLs for a (broken) symbolic link? TEST PLAN The tests pass. Dolphin works with broken symbolic links. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D13898 AFFECTED FILES src/ioslaves/file/file.cpp To: jtamate, dfaure, #frameworks Cc: kde-frameworks-devel, michaelh, ngraham, bruns
