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

[Nathaniel Graham]

ngraham abandoned this revision.
ngraham added a comment.


  Yep, I'm happy to. So glad to see that the feature's going in soon!

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

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

[Méven Car]

meven added a comment.


  @ngraham I guess we can mark this as abandoned in favor of D20096 
.

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

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

[Méven Car]

meven added a comment.


  glibc 2.28 was released in August 2018, it is at least in (k)ubuntu 18.10, 
and I would expect in most other distros as well.
  I would be happy to help test this.

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

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

[Stefan Brüns]

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

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

[Rolf Eike Beer]

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

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

[Stefan Brüns]

bruns added a comment.


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

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

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

[Fabian Vogt]

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

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

[Stefan Brüns]

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

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

[Nathaniel Graham]

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

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

[Nathaniel Graham]

ngraham marked an inline comment as done.
ngraham added a comment.


  I took a crack at accessing `statx` directly, but I'm waay 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.

REPOSITORY
  R241 KIO

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

To: ngraham, dfaure, broulik, elvisangelaccio
Cc: meven, ltoscano, #frameworks, michaelh, bruns

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

[Nathaniel Graham]

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".
ngraham edited the summary of this revision.
ngraham edited the test plan for this revision.

REPOSITORY
  R241 KIO

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

To: ngraham, dfaure, broulik, elvisangelaccio
Cc: meven, ltoscano, #frameworks, michaelh, bruns