Discussion:
D7423: [WIP/assistance needed] Populate UDS_CREATION_TIME on Linux if statx system call is available
Nathaniel Graham
2018-04-22 04:32:20 UTC
Permalink
ngraham added reviewers: Frameworks, Dolphin.

REPOSITORY
R241 KIO

REVISION DETAIL
https://phabricator.kde.org/D7423

To: ngraham, dfaure, broulik, elvisangelaccio, #frameworks, #dolphin
Cc: meven, ltoscano, #frameworks, michaelh, bruns
Stefan Brüns
2018-07-09 18:02:31 UTC
Permalink
Restricted Application added a subscriber: kde-frameworks-devel.

REPOSITORY
R241 KIO

REVISION DETAIL
https://phabricator.kde.org/D7423

To: ngraham, dfaure, broulik, elvisangelaccio, #frameworks, #dolphin
Cc: kde-frameworks-devel, bruns, meven, ltoscano, #frameworks, michaelh, ngraham
Fabian Vogt
2018-08-17 07:29:13 UTC
Permalink
fvogt requested changes to this revision.
fvogt added a comment.
This revision now requires changes to proceed.


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

REPOSITORY
R241 KIO

REVISION DETAIL
https://phabricator.kde.org/D7423

To: ngraham, dfaure, broulik, elvisangelaccio, #frameworks, #dolphin, fvogt
Cc: fvogt, kde-frameworks-devel, bruns, meven, ltoscano, #frameworks, michaelh, ngraham
Stefan Brüns
2018-08-17 16:22:02 UTC
Permalink
bruns added a comment.
Post by Fabian Vogt
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).

REPOSITORY
R241 KIO

REVISION DETAIL
https://phabricator.kde.org/D7423

To: ngraham, dfaure, broulik, elvisangelaccio, #frameworks, #dolphin, fvogt
Cc: fvogt, kde-frameworks-devel, bruns, meven, ltoscano, #frameworks, michaelh, ngraham
Rolf Eike Beer
2018-08-20 16:48:56 UTC
Permalink
dakon added inline comments.

INLINE COMMENTS
CMakeLists.txt:14
+ check_function_exists(statx HAVE_STATX)
+ set(HAVE_STATX ${HAVE_STATX})
endif()
This line looks needless.
file.cpp:86
+#include <sys/syscall.h>
+// 332 on my system
+#define STAT(path, buf) syscall(__NR_statx,(AT_FDCWD),(path),(AT_SYMLINK_NOFOLLOW),(0),(buf))
This comment isn't needed either.
file.cpp:950
#endif
+#ifdef stx_btime
+ /* As above, but it's called "stx_btime" in Linux kernel 4.11+
Where would that define come from? At least on glibc 2.28 I see it nowhere defined.

REPOSITORY
R241 KIO

REVISION DETAIL
https://phabricator.kde.org/D7423

To: ngraham, dfaure, broulik, elvisangelaccio, #frameworks, #dolphin, fvogt
Cc: dakon, fvogt, kde-frameworks-devel, bruns, meven, ltoscano, #frameworks, michaelh, ngraham
Stefan Brüns
2018-10-16 19:36:53 UTC
Permalink
bruns added a comment.


birthTime is supported by QTs QFileInfo::birthTime().
For the implementation, see here:
https://code.woboq.org/qt5/qtbase/src/corelib/io/qfilesystemengine_unix.cpp.html#329

REPOSITORY
R241 KIO

REVISION DETAIL
https://phabricator.kde.org/D7423

To: ngraham, dfaure, broulik, elvisangelaccio, #frameworks, #dolphin, fvogt
Cc: huftis, dakon, fvogt, kde-frameworks-devel, bruns, meven, ltoscano, #frameworks, michaelh, ngraham
Loading...