[WIP/assistance needed] Populate UDS_CREATION_TIME on Linux if statx system call is available
Needs RevisionPublic

Authored by ngraham on Aug 20 2017, 1:28 AM.

Details

Summary

FEATURE: 286689

Populate UDS_CREATION_TIME to make Dolphin display file creation times, if the supporting statx system call is available (in Linux Kernel 4.11 and later).

Diff Detail

Repository
R241 KIO
Branch
arcpatch-D7423
Lint
No Linters Available
Unit
No Unit Test Coverage
ngraham created this revision.Aug 20 2017, 1:28 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptAug 20 2017, 1:28 AM
Restricted Application added a subscriber: Frameworks. · View Herald Transcript
ngraham edited the summary of this revision. (Show Details)Aug 20 2017, 1:30 AM
dfaure edited edge metadata.Aug 20 2017, 8:12 PM

The commit log is inconsistent with the actual patch. It says "lack of a Qt version that supports this", but the patch doesn't use QFile at all, it uses statx. Either way, the patch can be tested, no?

ngraham added a comment.EditedAug 20 2017, 8:14 PM

This patch doesn't actually directly use statx(); it uses the existing approach, which goes through QFile, which is used to populate the statbuf object that we query. QT has gained support for populating the statbuf object using statx() in Linux where available in current git master, but that change hasn't landed in an actual released QT version yet.

If you can point me to a way to test this, I'll be happy to.

I can't comment on the content of the patch, but about the commit message, in order to close a bug, use the syntax:

BUG: nnnnnn

while in order to add the commit as comment of a bug without closing it, use

CCBUG: nnnnnn

Both keyword should be in a separate line.

Also, it's Qt, not QT.

ngraham retitled this revision from Populate UDS_CREATION_TIME on Linux if QT version and Linux kernel support it (no effect if they don't) to Populate UDS_CREATION_TIME on Linux if Qt version and Linux kernel support it (no effect if they don't).Aug 20 2017, 8:32 PM
ngraham retitled this revision from Populate UDS_CREATION_TIME on Linux if Qt version and Linux kernel support it (no effect if they don't) to Populate UDS_CREATION_TIME on Linux if Qt version and Linux kernel support it (no effect if they don't) BUG: 286689.Aug 20 2017, 8:34 PM

Noted! Thanks, Luigi. I'd like to add BUG: 286689 to the commit message on a separate line. Does the title of this revision become the commit message? If so, how do I add another line? The text field doesn't seem to support multi-line strings.

The title of the revision becomes the first line; all the rest (summary, test, reviewers) become the rest of the content, a blank separator line is added automatically. Please add the BUG: line in the summary, not in the title.
Also, as the title becomes the first line, it would be nice to follow the usual git rules of length.

Ah, OK, I see.
Well, still, you could test the patch by compiling qtbase dev (or by applying whichever Qt patch is necessary).

ngraham retitled this revision from Populate UDS_CREATION_TIME on Linux if Qt version and Linux kernel support it (no effect if they don't) BUG: 286689 to Populate UDS_CREATION_TIME on Linux if Qt version and kernel support it.Aug 20 2017, 9:32 PM
ngraham edited the summary of this revision. (Show Details)
ngraham edited the test plan for this revision. (Show Details)

Great, thanks guys. Thanks for bearing with me here. I'll see if I can test with the unreleased qtbase.

ngraham retitled this revision from Populate UDS_CREATION_TIME on Linux if Qt version and kernel support it to Populate UDS_CREATION_TIME on Linux if Qt and kernel versions support it.Aug 20 2017, 9:46 PM

I haven't forgotten about this, but life has intervened and I'm not going to have time to work on it for a few days, maybe even a couple of weeks. If anyone else wants to run with it and take over, I won't be in the least bit offended. If not, I'll resume work at some point in the near to medium term future.

ngraham updated this revision to Diff 21871.Nov 4 2017, 4:42 PM

Improve comment for clarity

ngraham edited the summary of this revision. (Show Details)Nov 4 2017, 6:35 PM
ngraham edited the test plan for this revision. (Show Details)
dfaure added a comment.Dec 2 2017, 3:50 PM

I don't get it. Which parts of this commit depend upon Qt 5.10? I only see buff.birthTime_ being used.

Unless I'm misunderstanding everything, birthTime_ is only set to a nonzero value as of https://github.com/qt/qtbase/commit/d3393ce25833c0afd7f0fa6b85fd6f3bd7ad520a#diff-e061e747264146cf47a86b371e512348R363, which only made it into Qt 5.10 and was not backported to 5.9.

dfaure added inline comments.Dec 2 2017, 4:11 PM
src/ioslaves/file/file.cpp
783

... but we fill buff using lstat(), not using QFile, so it doesn't matter what QFile does.

dfaure added a comment.Dec 2 2017, 4:24 PM

This patch doesn't actually directly use statx(); it uses the existing approach, which goes through QFile, which is used to populate the statbuf object that we query.

Can you show me where this code uses QFile to populate the statbuf object? One of us is missing something here.

One of us is missing something here.

I'm sure it's me. :-)
If this approach doesn't work, would you be able to offer some pointers regarding a better method of implementing it?

dfaure added a comment.Dec 2 2017, 7:20 PM

Well, this hasn't been tested, has it?

AFAIU this requires calling statx(..., AT_SYMLINK_NOFOLLOW, ...) instead of lstat, on systems where statx is available.

meven added a subscriber: meven.Feb 10 2018, 1:38 PM
ngraham updated this revision to Diff 32762.Apr 22 2018, 4:21 AM

Attempt so far unsuccessfully to use statx directly

ngraham retitled this revision from Populate UDS_CREATION_TIME on Linux if Qt and kernel versions support it to [WIP/assistance needed] Populate UDS_CREATION_TIME on Linux if statx system call is available.Apr 22 2018, 4:23 AM
ngraham edited the summary of this revision. (Show Details)
ngraham edited the test plan for this revision. (Show Details)
ngraham marked an inline comment as done.Apr 22 2018, 4:29 AM

I took a crack at accessing statx directly, but I'm waaaaaay out of my depth here and will need some help making this actually work. There doesn't seem to be a glibc wrapper for statx yet, so we have to access it directly, which is messy and I haven't figured out how to make it work yet, though I think I might be on the right track.

As such, this patch is presented--currently in non-working form, sadly--for discussion and hopefully instructional purposes. If someone else with more domain knowledge wants to grab this and do it right themselves, please feel free!

Problems that need to be overcome:

  • On my Neon test system with kernel 5.13, __NR_statx isn't actually available in /usr/include/unistd.h; it's only available in `/usr/src/linux-headers-4.13.0-38-generic/arch/x86/include/generated/uapi/asm/unistd_64.h
  • The check_function_exists(statx HAVE_STATX) CMake command returns 0 even though statx is available.
  • The file KIOSlave crashes in ../sysdeps/unix/syscall-template.S:84 when I try to run Dolphin with this patch for KIO.
bruns added a subscriber: bruns.Jul 9 2018, 6:02 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald TranscriptJul 9 2018, 6:02 PM
fvogt requested changes to this revision.Aug 17 2018, 7:29 AM
fvogt added a subscriber: fvogt.

On neon it won't work as the kernel everything is built against (so the minimum API/ABI) is too old. You'll either have to hack around that by messing with include paths or use something more recent.

statx does not have a glibc wrapper, which means there is no statx(...) function. So HAVE_STATX is rightfully false.

What Qt does is different: If SYS_statx is defined, it determines during runtime whether the kernel supports it. QT_STATBUF is in any case a struct stat/stat64, so if your code calls statx, it'll corrupt memory.
The only reason your code even compiled is that stx_btime is not a macro, so that code wasn't compiled in.

There is a bigger design issue with this approach even: There are either "stx_" members or "st_" members available. So every access to "buff" needs to be guarded by both an #if HAVE_STATX and an if(statx_available).

You also need to note that the kernel syscall interface is different from glibc wrappers/posix functions: There is no errno, the error is directly in the return value.
So stat(...) == -1 is not equal to syscall(SYS_statx, ...) == -1. The latter is equivalent to stat(...) == -1 && errno == EPERM (EPERM is 1).

I suggest rewriting this to make use of a wrapper function instead of macros, like Qt does. You can find that in src/corelib/io/qfilesystemengine_unix.cpp.

This revision now requires changes to proceed.Aug 17 2018, 7:29 AM
bruns added a comment.Aug 17 2018, 4:22 PM
In D7423#310509, @fvogt wrote:

On neon it won't work as the kernel everything is built against (so the minimum API/ABI) is too old. You'll either have to hack around that by messing with include paths or use something more recent.

statx does not have a glibc wrapper, which means there is no statx(...) function. So HAVE_STATX is rightfully false.

filesystemengine_unix.cpp`.

This is no longer true with glibc 2.28, released two weeks ago. The interface is defined here:

https://sourceware.org/git/?p=glibc.git;a=blob;f=io/bits/statx.h;h=e31254e3617bb17b1d4ba1dc5365529e376e257d;hb=HEAD

So, first question which arises is, should we make it depend on glibc 2.28? There is no kernel dependency,
as glibc fills out the statx struct using other syscalls if statx is not available (of course, no birthtime then).

dakon added a subscriber: dakon.Aug 20 2018, 4:48 PM
dakon added inline comments.
src/ioslaves/file/CMakeLists.txt
14 ↗(On Diff #32762)

This line looks needless.

src/ioslaves/file/file.cpp
86

This comment isn't needed either.

878

Where would that define come from? At least on glibc 2.28 I see it nowhere defined.

huftis added a subscriber: huftis.Aug 21 2018, 7:19 PM