Details

Reviewers

dfaure
fvogt
bruns
broulik

Group Reviewers

Frameworks

Commits

R241:f3b0f220a78d: Fill UDSEntry::UDS_CREATION_TIME under linux when glibc >= 2.28

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

Use stx_mask to check for btime availability

Harbormaster completed remote builds in B10508: Diff 55539.Apr 6 2019, 11:16 AM

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

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

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

describe the dolphin screenshot textually
update the FIXED-IN, unless it lands before KF 5.57 is tagged
in the test plan, say what is tested
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

Updating commit message

Harbormaster completed remote builds in B10563: Diff 55644.Apr 7 2019, 10:04 AM

meven mentioned this in D7423: [WIP/assistance needed] Populate UDS_CREATION_TIME on Linux if statx system call is available.Apr 7 2019, 10:05 AM

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

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

Harbormaster completed remote builds in B10584: Diff 55679.Apr 7 2019, 3:30 PM

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.

In D20096#445410, @ngraham wrote:

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

Remove reference to old differential revision from the commit comment

Harbormaster completed remote builds in B10585: Diff 55683.Apr 7 2019, 3:37 PM

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

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

review feedback

Harbormaster completed remote builds in B10598: Diff 55709.Apr 7 2019, 8:01 PM

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

meven mentioned this in D20404: Allow the baloo widgets to display creation date and access date..Apr 9 2019, 11:58 AM

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

anthonyfieroni added a subscriber: anthonyfieroni.Apr 9 2019, 5:12 PM

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

ngraham added a dependent revision: D20404: Allow the baloo widgets to display creation date and access date..Apr 9 2019, 5:15 PM

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

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

Harbormaster completed remote builds in B10675: Diff 55849.Apr 9 2019, 6:27 PM

Fix src/ioslaves/file/ConfigureChecks.cmake

Harbormaster completed remote builds in B10676: Diff 55850.Apr 9 2019, 6:27 PM

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

In D20096#446742, @ngraham wrote:

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`

Fix switch

Harbormaster completed remote builds in B10728: Diff 55967.Apr 11 2019, 6:58 AM

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 ?

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

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!

Closed by commit R241:f3b0f220a78d: Fill UDSEntry::UDS_CREATION_TIME under linux when glibc >= 2.28 (authored by meven). · Explain WhyApr 13 2019, 1:30 PM

This revision was automatically updated to reflect the committed changes.

In D20096#449198, @bruns wrote:

Thanks for working on this!

I am very glad :)

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

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.

In D20096#451148, @bcooksley wrote:

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

In D20096#451150, @dfaure wrote:

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 mentioned this in T8456: Consolidate creation of UDSEntry, currently replicated in the various baloo kioslaves.Apr 18 2019, 7:06 AM

In D20096#451170, @dfaure wrote:

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.

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.

In D20096#453194, @dfaure wrote:

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.

			Path	Packages
M			src/ioslaves/file/ConfigureChecks.cmake (14 lines)
M			src/ioslaves/file/config-kioslave-file.h.cmake (3 lines)
M			src/ioslaves/file/file.cpp (114 lines)
M			tests/kioslavetest.cpp (17 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	54996	ed15849		Mar 28 2019, 5:27 PM	★	★
Diff 2	54997	ed15849	Removed empty file.	Mar 28 2019, 5:29 PM	★	★
Diff 3	55064	ed15849	Fix timestamp value	Mar 30 2019, 5:35 PM	★	★
Diff 4	55066	ed15849	Clean up	Mar 30 2019, 5:42 PM	★	★
Diff 5	55067	ed15849	Clean up	Mar 30 2019, 5:42 PM	★	★
Diff 6	55068	ed15849	Avoid obsolete method	Mar 30 2019, 5:48 PM	★	★
Diff 7	55074	ed15849	Avoid duplicating code	Mar 30 2019, 6:40 PM	★	★
Diff 8	55077	ed15849	Fix in case of symlinks resolution, add inlines	Mar 30 2019, 6:55 PM	★	★
Diff 9	55079	ed15849	Rename function get_stat to STAT	Mar 30 2019, 7:05 PM	★	★
Diff 10	55082	ed15849	Use switch statement, +1 to the qbound comparison of the buffer size	Mar 30 2019, 7:30 PM	★	★
Diff 11	55110	ed15849	Add details == 2 and correct switch	Mar 31 2019, 8:15 AM	★	★
Diff 12	55111	ed15849	Use a better lower limit for the buffer size, add some comment	Mar 31 2019, 8:22 AM	★	★
Diff 13	55120	ed15849	Remove unused includes	Mar 31 2019, 10:56 AM	★	★
Diff 14	55123	ed15849	Restrict statx use when c standard library is Glibc	Mar 31 2019, 11:35 AM	★	★
Diff 15	55124	ed15849	Conditionnaly define USE_STATX	Mar 31 2019, 11:44 AM	★	★
Diff 16	55132	ed15849	Avoid including twice errno.h	Mar 31 2019, 1:48 PM	★	★
Diff 17	55146	ed15849	Handle properly respective types for stat and statx syscalls	Mar 31 2019, 7:23 PM	★	★
Diff 18	55147	ed15849	Use time_t instead of __time_t and fix higherBound	Mar 31 2019, 7:30 PM	★	★
Diff 19	55182	ed15849	Add AT_SYMLINK_NOFOLLOW flag to statx call matching lstat behavior, use size_t…	Apr 1 2019, 10:42 AM	★	★
Diff 20	55183	ed15849	Fix position of default in switch statement	Apr 1 2019, 10:46 AM	★	★
Diff 21	55539	ed15849	Use stx_mask to check for btime availability	Apr 6 2019, 11:16 AM	★	★
Diff 22	55644	ed15849	Updating commit message	Apr 7 2019, 10:04 AM	★	★
Diff 23	55679	ed15849	Use proper cmake check, fix an issue with KIO::UDSEntry::UDS_DEVICE_ID having…	Apr 7 2019, 3:30 PM	★	★
Diff 24	55683	ed15849	Remove reference to old differential revision from the commit comment	Apr 7 2019, 3:37 PM	★	★
Diff 25	55709	ed15849	review feedback	Apr 7 2019, 8:01 PM	★	★
Diff 26	55849	ed15849	Use passing references to stat_xxx accessors, use a signed long long for file…	Apr 9 2019, 6:26 PM	★	★
Diff 27	55850	ed15849	Fix src/ioslaves/file/ConfigureChecks.cmake	Apr 9 2019, 6:27 PM	★	★
Diff 28	55967	ed15849	Fix switch	Apr 11 2019, 6:58 AM	★	★
Diff 29	56137	4d7ebf7	R241:f3b0f220a78d4e5a5b94b46e931779d755ac19ce	Apr 13 2019, 1:30 PM	★	★

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

Details

Diff Detail

Revision Contents
Changeset List

Diff 55679

src/ioslaves/file/ConfigureChecks.cmake

src/ioslaves/file/config-kioslave-file.h.cmake

src/ioslaves/file/file.cpp

tests/kioslavetest.cpp

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

Details

Diff Detail

Revision ContentsChangeset List

Diff 55679

src/ioslaves/file/ConfigureChecks.cmake

src/ioslaves/file/config-kioslave-file.h.cmake

src/ioslaves/file/file.cpp

tests/kioslavetest.cpp

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

Revision Contents
Changeset List