Fill UDSEntry::UDS_CREATION_TIME under linux when glibc >= 2.28
ClosedPublic

Authored by meven on Mar 28 2019, 5:27 PM.

Details

Summary

The birthtime can be retrieved using using the statx function, available since glibc 2.28. In case the kernel lacks support for the underlying syscall, glibc falls back to stat internally. The validity of the btime field is indicated in the mask field, e.g. when the kernel or the filesystem lacks support for btime.
For details, see http://man7.org/linux/man-pages/man2/statx.2.html

In dolphin, the creation time column is now filled in linux :

FEATURE: 381367
FIXED-IN: 5.58

Test Plan

Tested using kioslavetest and local build of dolphin.
Tested without HAVE_STATX under linux.

Diff Detail

Repository
R241 KIO
Branch
arcpatch-D20096
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 10584
Build 10602: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes
This revision now requires changes to proceed.Apr 6 2019, 9:59 AM
meven updated this revision to Diff 55539.Apr 6 2019, 11:16 AM
meven marked an inline comment as done.

Use stx_mask to check for btime availability

meven added inline comments.Apr 6 2019, 11:18 AM
src/ioslaves/file/file.cpp
1027

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

meven marked an inline comment as done.Apr 6 2019, 11:18 AM
fvogt added a comment.Apr 6 2019, 12:14 PM

Looking good to me, @bruns: any addiitonal comments?

bruns added a comment.Apr 6 2019, 5:11 PM

I think the code itself is good to go now.

Can you please update the summary somewhat (and better do it directly in phabricator, as arc will throw away any changes in the commit message, unless you do a - arc amend, update the commit message in git, arc diff --verbatim - cycle).

  1. describe the dolphin screenshot textually
  2. update the FIXED-IN, unless it lands before KF 5.57 is tagged
  3. in the test plan, say what is tested
  4. probably better first sentence:

The birthtime can be retrieved using using the statx function, available since glibc 2.28. In case the kernel lacks support for the underlying syscall, glibc falls back to stat internally. The validity of the btime field is indicated in the mask field, e.g. when the kernel or the filesystem lacks support for btime. For details, see <manpage url>.

Yeah, we should land this after 5.57 tagging so we have a month of pre-release testing, so it should be FIXED-IN: 5.58.

meven edited the summary of this revision. (Show Details)Apr 7 2019, 9:59 AM
meven edited the summary of this revision. (Show Details)
meven edited the test plan for this revision. (Show Details)
meven edited the summary of this revision. (Show Details)Apr 7 2019, 10:02 AM
meven updated this revision to Diff 55644.Apr 7 2019, 10:04 AM

Updating commit message

fvogt accepted this revision.Apr 7 2019, 10:07 AM
This revision is now accepted and ready to land.Apr 7 2019, 10:07 AM
pino added inline comments.Apr 7 2019, 10:08 AM
src/ioslaves/file/file.cpp
72–74 ↗(On Diff #55644)

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

BTW Frameworks 5.57 has been tagged, so whenever folks are good with this, it can be landed at any time. :)

meven updated this revision to Diff 55679.Apr 7 2019, 3:30 PM

Use proper cmake check, fix an issue with KIO::UDSEntry::UDS_DEVICE_ID having value of stat_mode instead of stat_dev

meven marked an inline comment as done.Apr 7 2019, 3:31 PM
meven added inline comments.
src/ioslaves/file/file.cpp
72–74 ↗(On Diff #55644)

It was not too easy to do.

meven marked an inline comment as done.Apr 7 2019, 3:34 PM

BTW Frameworks 5.57 has been tagged, so whenever folks are good with this, it can be landed at any time. :)

I think this would be best to wait for this for 5.58 to have a month of testing as you suggested.

meven edited the summary of this revision. (Show Details)Apr 7 2019, 3:36 PM
meven updated this revision to Diff 55683.Apr 7 2019, 3:37 PM

Remove reference to old differential revision from the commit comment

meven marked an inline comment as done.Apr 7 2019, 3:37 PM
meven added inline comments.Apr 7 2019, 3:40 PM
src/ioslaves/file/ConfigureChecks.cmake
19 ↗(On Diff #55683)

I am not sure this is acceptable, this variable is set by ECM in extra-cmake-modules/kde-modules/KDECompilerSettings.cmake

dfaure added a comment.Apr 7 2019, 3:42 PM

The month of testing starts today. Code pushed to master starting from today will end up in 5.58.

pino added inline comments.Apr 7 2019, 6:04 PM
src/ioslaves/file/ConfigureChecks.cmake
19 ↗(On Diff #55683)

just do the test unconditionally

24 ↗(On Diff #55683)

other than this define, try to do a simple usage of statx:

struct statx buf;
statx(AT_FDCWD, "/foo", AT_EMPTY_PATH, STATX_BASIC_STATS, &buf);

(it will not be run anyway)

src/ioslaves/file/file.cpp
68 ↗(On Diff #55644)

no need for the Q_OS_LINUX check here

meven updated this revision to Diff 55709.Apr 7 2019, 8:01 PM
meven marked 4 inline comments as done.

review feedback

pino added inline comments.Apr 7 2019, 8:05 PM
src/ioslaves/file/ConfigureChecks.cmake
19 ↗(On Diff #55683)

sorry, i realized that my sentence was ambiguous: i meant to check for the existance of statx no matter the platform

meven marked an inline comment as done.Apr 7 2019, 8:13 PM
meven added inline comments.
src/ioslaves/file/ConfigureChecks.cmake
19 ↗(On Diff #55683)

We discussed previously that we would restrict using statx to the platform where it was tested i.e GLIBC to avoid future bugs with different potential different implementation of statx.
https://phabricator.kde.org/D20096#440921

meven marked 2 inline comments as done.Apr 7 2019, 8:15 PM
meven edited the summary of this revision. (Show Details)Apr 9 2019, 10:10 AM

So are we ready to land this, or is there anything left to do?

anthonyfieroni added inline comments.
src/ioslaves/file/file.cpp
856–876 ↗(On Diff #55644)

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

meven added inline comments.Apr 9 2019, 5:26 PM
src/ioslaves/file/file.cpp
856–876 ↗(On Diff #55644)

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 marked 2 inline comments as done.Apr 9 2019, 5:31 PM
meven added inline comments.
src/ioslaves/file/file.cpp
856–876 ↗(On Diff #55644)

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 marked 2 inline comments as done.Apr 9 2019, 5:32 PM
pino added inline comments.Apr 9 2019, 5:44 PM
src/ioslaves/file/file.cpp
856–876 ↗(On Diff #55644)

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

inline static uint16_t stat_mode(struct statx &buf) { return buf.stx_mode; }
meven edited the test plan for this revision. (Show Details)Apr 9 2019, 6:22 PM
meven updated this revision to Diff 55849.Apr 9 2019, 6:26 PM

Use passing references to stat_xxx accessors, use a signed long long for file size

meven updated this revision to Diff 55850.Apr 9 2019, 6:27 PM

Fix src/ioslaves/file/ConfigureChecks.cmake

meven marked an inline comment as done.Apr 9 2019, 6:28 PM
meven added inline comments.
src/ioslaves/file/file.cpp
856–876 ↗(On Diff #55644)

Thanks

meven marked 2 inline comments as done.Apr 9 2019, 6:28 PM
meven added inline comments.Apr 10 2019, 8:58 AM
src/ioslaves/file/file.cpp
856–876 ↗(On Diff #55644)

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

I think he meant both.

meven marked an inline comment as done.Apr 10 2019, 8:58 AM
meven added a comment.Apr 10 2019, 3:39 PM

So are we ready to land this, or is there anything left to do?

Are there some more feedback ?

I would appreciate a second accept.

meven edited the summary of this revision. (Show Details)Apr 10 2019, 6:57 PM
bruns added inline comments.Apr 10 2019, 10:25 PM
src/ioslaves/file/file.cpp
896 ↗(On Diff #55644)

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.

945 ↗(On Diff #55644)

missing space, size + 1

meven updated this revision to Diff 55967.Apr 11 2019, 6:58 AM

Fix switch

meven marked an inline comment as done.Apr 11 2019, 6:59 AM
meven added inline comments.
src/ioslaves/file/file.cpp
896 ↗(On Diff #55644)

Thanks

meven marked 2 inline comments as done.Apr 11 2019, 6:59 AM

Any new feedback ? @fvogt @bruns @dfaure ?

fvogt accepted this revision.Apr 13 2019, 11:29 AM

Code still looks good to me - I can't comment on the cmake parts though.

bruns accepted this revision.Apr 13 2019, 1:17 PM

I think its better go give it sufficient testing time than to wait for more comments. The code has been around for sufficient time, and the last changes were just minor.

Thanks for working on this!

This revision was automatically updated to reflect the committed changes.
meven added a comment.Apr 13 2019, 1:31 PM

Thanks for working on this!

I am very glad :)

Thank you for your great reviews. I learned quite a bit along the way.

meven added a comment.Apr 13 2019, 1:50 PM

There is still a couple raw QT_LSTAT in KFileItem https://cgit.kde.org/kio.git/tree/src/core/kfileitem.cpp#n224 .
Do you think we should apply the same kind of solution there, with #idef ?

I would be in favor of reusing code instead.
Eventually moving FileProtocol::createUDSEntry out of FileProtocol in some other util so that it can be reused by KFileitem.

MSVC doesn't like this change i'm afraid - see https://build.kde.org/view/OS%20-%20Windows/job/Frameworks/job/kio/job/kf5-qt5%20WindowsMSVCQt5.11/259/console
Looks like you're making use of C++17 features?

Nah, no C++17 here, the compiler is just confused because it doesn't know the types uid_t and gid_t.

I'll add some #ifndef Q_OS_WIN.

MSVC doesn't like this change i'm afraid - see https://build.kde.org/view/OS%20-%20Windows/job/Frameworks/job/kio/job/kf5-qt5%20WindowsMSVCQt5.11/259/console
Looks like you're making use of C++17 features?

I am a bit surprised. I used std:c++11 as for the rest of current kde compli

Nah, no C++17 here, the compiler is just confused because it doesn't know the types uid_t and gid_t.

I'll add some #ifndef Q_OS_WIN.

Nah, no C++17 here, the compiler is just confused because it doesn't know the types uid_t and gid_t.

I'll add some #ifndef Q_OS_WIN.

@bcooksley
Could you test https://phabricator.kde.org/D20599 ?

This change also introduced regressions in two unittests: jobtest and kdirmodeltest. I'll let you look into those :-)

https://build.kde.org/view/OS%20-%20Windows/job/Frameworks/job/kio/job/kf5-qt5%20SUSEQt5.10/87/testReport/junit/projectroot/autotests/kiocore_jobtest/
https://build.kde.org/view/OS%20-%20Windows/job/Frameworks/job/kio/job/kf5-qt5%20SUSEQt5.10/87/testReport/projectroot/autotests/kiowidgets_kdirmodeltest/

The first one is probably just an extra file in the dir (I know, bad isolation of the tests), the second one is more.... unexpected.

meven added a comment.Apr 20 2019, 7:51 AM

This change also introduced regressions in two unittests: jobtest and kdirmodeltest. I'll let you look into those :-)

https://build.kde.org/view/OS%20-%20Windows/job/Frameworks/job/kio/job/kf5-qt5%20SUSEQt5.10/87/testReport/junit/projectroot/autotests/kiocore_jobtest/
https://build.kde.org/view/OS%20-%20Windows/job/Frameworks/job/kio/job/kf5-qt5%20SUSEQt5.10/87/testReport/projectroot/autotests/kiowidgets_kdirmodeltest/

The first one is probably just an extra file in the dir (I know, bad isolation of the tests), the second one is more.... unexpected.

The tests failures are not regressions as they are happened before the merge : https://build.kde.org/view/OS%20-%20Windows/job/Frameworks/job/kio/job/kf5-qt5%20SUSEQt5.12/72/testReport/ although on Qt 5.12.

Failures are:

FAIL! : KFileItemTest::testListProperties(two (text+html) files) Compared values are not the same

Actual   (props.mimeGroup()): ""
Expected (expectedMimeGroup): "text"
Loc: [/home/jenkins/workspace/Frameworks/kio/kf5-qt5 SUSEQt5.10/autotests/kfileitemtest.cpp(590)]

I don't reproduce locally.

FAIL! : KDirListerTest::testRenameItem() Compared values are not the same

Actual   (entry.second.mimetype()): "application/xhtml+xml"
Expected (QString("text/html"))   : "text/html"
Loc: [/home/jenkins/workspace/Frameworks/kio/kf5-qt5 SUSEQt5.10/autotests/kdirlistertest.cpp(557)]

I didn't reproduce locally, but I could see some issue in the test.
Here is a fix proposal for this test : D20692
Is it possible to test it on jenkins before merging ?

So it seems to me as sporadic failures of slightly unconsistent tests.

dfaure added a comment.EditedApr 20 2019, 8:28 AM

I'm talking about jobtest and kdirmodeltest regressing exactly in build 87, which is where this commit landed.
If you click on history for a given test you can see that those aren't sporadic failures: https://build.kde.org/view/OS%20-%20Windows/job/Frameworks/job/kio/job/kf5-qt5%20SUSEQt5.10/87/testReport/junit/projectroot/autotests/kiowidgets_kdirmodeltest/history/
and
https://build.kde.org/view/OS%20-%20Windows/job/Frameworks/job/kio/job/kf5-qt5%20SUSEQt5.10/87/testReport/junit/projectroot/autotests/kiocore_jobtest/history/

The failures in kfileitemtest and kdirlistertest (note: model != lister) are unrelated, I'll look into those.

I'm talking about jobtest and kdirmodeltest regressing exactly in build 87, which is where this commit landed.
If you click on history for a given test you can see that those aren't sporadic failures: https://build.kde.org/view/OS%20-%20Windows/job/Frameworks/job/kio/job/kf5-qt5%20SUSEQt5.10/87/testReport/junit/projectroot/autotests/kiowidgets_kdirmodeltest/history/
and
https://build.kde.org/view/OS%20-%20Windows/job/Frameworks/job/kio/job/kf5-qt5%20SUSEQt5.10/87/testReport/junit/projectroot/autotests/kiocore_jobtest/history/

The failures in kfileitemtest and kdirlistertest (note: model != lister) are unrelated, I'll look into those.

Thank you for making this clear.

The regression are associated with symlink resolution.
I think I have identified the issue.
Please see D20694 and could we test this on CI.

It makes me think that for some reviews, it would be good to test it on CI before merging.
At my past place of work we used this extensively, it was quite helpful.