Differential D13898

Use non deprecated fastInsert in file.cpp (first of many to come)
ClosedPublic
Actions

Authored by jtamate on Jul 5 2018, 9:12 AM.

Details

Reviewers
dfaure
Group Reviewers
Frameworks
Commits
R241:6b452ae9892d: Use non deprecated fastInsert in file.cpp (first of many to come)
Summary

Avoid the goto using local variables.
Use fastInsert instead of insert.
Fill the ACL of the linked destination.

Test Plan

The tests pass.
Dolphin works with broken symbolic links.

Diff Detail

Repository
R241 KIO
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
jtamate created this revision.Jul 5 2018, 9:12 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptJul 5 2018, 9:12 AM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
jtamate requested review of this revision.Jul 5 2018, 9:12 AM
dfaure requested changes to this revision.Jul 5 2018, 10:49 AM

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.

src/ioslaves/file/file.cpp
841

Call it isBrokenSymlink, it's only set to true for *broken* symlinks.

This revision now requires changes to proceed.Jul 5 2018, 10:49 AM
dfaure added inline comments.Jul 5 2018, 10:50 AM
src/ioslaves/file/file.cpp
882–883

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.

jtamate updated this revision to Diff 37184.Jul 5 2018, 11:46 AM
jtamate edited the summary of this revision. (Show Details)

Renamed isSymLink to isBrokenSymLink.

dfaure accepted this revision.Jul 5 2018, 12:58 PM
This revision is now accepted and ready to land.Jul 5 2018, 12:58 PM
bruns added a subscriber: bruns.Jul 5 2018, 4:29 PM
bruns added inline comments.
src/ioslaves/file/file.cpp
867

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.

bruns added inline comments.Jul 5 2018, 4:54 PM
src/ioslaves/file/file.cpp
919

For UDS_ACCESS, _USER, _GROUP, we follow the symlink (i.e use the corresponding buff), for ACL we do not?

923

all the fields below are pointless in case of isBrokenSymlink, buff may contain anything after the failed stat call.

jtamate updated this revision to Diff 37216.Jul 6 2018, 6:39 AM
jtamate marked 3 inline comments as done.

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.

src/ioslaves/file/file.cpp
867

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.
bruns added inline comments.Jul 6 2018, 9:56 AM
src/ioslaves/file/file.cpp
867

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);
jtamate updated this revision to Diff 37240.EditedJul 6 2018, 10:48 AM
jtamate marked 2 inline comments as done.

Incorporated Stefan code.
Fill again the extra data like User/Group.
I don't see any change and I don't want to break anything more.

bruns added inline comments.Jul 6 2018, 3:34 PM
src/ioslaves/file/file.cpp
886

isn't this the same as linkTargetBuffer?

dfaure requested changes to this revision.Jul 7 2018, 9:55 PM
dfaure added inline comments.
src/ioslaves/file/file.cpp
867

Given that this fix is completely unrelated to this commit, it should rather be separated into a different commit...

886

Yes, and in fact linkTargetBuffer is more correct. toLocal8Bit() is incorrect, QFile::encodeName/decodeName must be used for QByteArray<->QString conversions for filenames.

This revision now requires changes to proceed.Jul 7 2018, 9:55 PM
jtamate updated this revision to Diff 37424.EditedJul 9 2018, 7:44 AM
jtamate edited the summary of this revision. (Show Details)

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().

dfaure added a comment.Jul 9 2018, 7:47 AM

The comments indicate that this isn't supposed to happen, but just to make sure, you can use #elif on line 941/942.

jtamate updated this revision to Diff 37427.Jul 9 2018, 8:00 AM

Use

st_birthtime

or

__st_birthtime

but not both.

dfaure accepted this revision.Jul 9 2018, 8:05 AM

Thanks!

This revision is now accepted and ready to land.Jul 9 2018, 8:05 AM
Closed by commit R241:6b452ae9892d: Use non deprecated fastInsert in file.cpp (first of many to come) (authored by jtamate). · Explain WhyJul 9 2018, 2:35 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Msrc/ioslaves/file/file.cpp (75 lines)

Diff 37458

View Options

src/ioslaves/file/file.cpp

Loading...