Fill UDS_CREATION_TIME with the value of st_birthtime on FreeBSD
ClosedPublic

Authored by adridg on Mar 22 2017, 9:24 PM.

Details

Summary

It seems UDS_CREATION_TIME is never filled at all (as far as I could find in the short time I invested :D ).

From the manpage:

st_birthtim      Time when the inode was created.

This allows us to display file creation dates in (patched) dolphin.

This is with respect to:
https://bugs.kde.org/show_bug.cgi?id=374063

Diff Detail

Repository
R241 KIO
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
tcberner created this revision.Mar 22 2017, 9:24 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptMar 22 2017, 9:24 PM
Restricted Application added a subscriber: Frameworks. · View Herald Transcript
dfaure accepted this revision.Mar 23 2017, 7:42 AM

Feel like implementing it for Linux, too? ;-)

This revision is now accepted and ready to land.Mar 23 2017, 7:42 AM
arrowd accepted this revision.Mar 23 2017, 8:14 AM
adridg edited edge metadata.Mar 23 2017, 10:28 AM

Might want to add a comment about "this is the actual inode creation time; support depends on the underlying filesystem (e.g. UFS and ZFS have this information, NFS doesn't)", and change the comment about buff.st_ctime and KDE 2.0.

In addition, please insert the creation time information only if buff.st_birthtime is useful. On an FS that doesn't support the information, you'll get 0; but time_t is (on FreeBSD x86 anyway) signed, so 0 or < 0 are in theory legitimate timestamps; for simplicity probably use only positive timestamps.

On Linux (replying to DFaure), ext4 supports birthtime (crtime), but stat(2) doesn't have a way to report that information; I'll dig into GNU coreutils a little (e.g. 20 minutes, tops) to see how they get at, and report, the information.

The comment about ctime is still true ;-)

For Linux support, I suppose https://codereview.qt-project.org/167049 has what we need.

Further to the Linux bits: while ext4 and btrfs have the necessary fields, stat(2) doesn't expose them, xstat(2) doesn't exist yet (see LWN from a few years back), and extracting the info using other tools depends on fun stuff like knowing what kind of filesystem the file resides on (e.g. debugfs is a userland tool that can get the bits, if it's allowed to open the raw filesystem device *and* it's ext[234]).

So, Linux, sort out your *stat(2) interfaces first and we'll get back to you.

tcberner: points to be had for doing a cmake-time check for the presence of that field, since NetBSD has it as well.

adridg accepted this revision.Mar 23 2017, 11:37 AM
In D5138#97024, @adridg wrote:

So, Linux, sort out your *stat(2) interfaces first and we'll get back to you.

xstat patches have been merged into 4.11

Ah yes OK, the extraction on Linux is indeed not done in those Qt patches either. Nevermind ;)

tcberner updated this revision to Diff 12743.Mar 23 2017, 3:57 PM

Use cmake to check for st_birthime.

tcberner updated this revision to Diff 12744.Mar 23 2017, 4:14 PM

Add #cmakedefine01 HAVE_ST_BIRTHTIME to config-kioslave-file.h.cmake

kfunk added a subscriber: kfunk.Mar 23 2017, 4:22 PM
kfunk added inline comments.
src/ioslaves/file/config-kioslave-file.h.cmake
13 ↗(On Diff #12744)

Note: typo

tcberner updated this revision to Diff 12745.Mar 23 2017, 4:25 PM

Fix typo.

aacid added a subscriber: aacid.Apr 5 2017, 9:46 PM

tobias will you commit this? Or are we still waiting for some other approval?

We talked about it some more, and think that it is maybe better to not do any cmake magic.

#ifdef st_birthtime
  if (buff.st_birthtime > 0) {
            entry.insert(KIO::UDSEntry::UDS_CREATION_TIME, buff.st_birthtime);
        }
#endif
#ifdev __st_birthtime 
  if (buff.__st_birthtime > 0) {
            entry.insert(KIO::UDSEntry::UDS_CREATION_TIME, buff.__st_birthtime);
        }
#endif

and so on. The first would add it for FreeBSD/NetBSD and the second for OpenBSD -- given, that the syscall for linux will be completely different too.

@tcberner Can you mark this as "Request Changes" then so it doens't show up in the list of "this has been approved to land but still noone has commited it yet" list?

adridg updated this revision to Diff 13528.Apr 17 2017, 12:00 PM

Take over the review so as to land it, based on tcberner's comments
from april 6th, and Albert's reminder.

adridg commandeered this revision.Apr 17 2017, 12:06 PM
adridg edited reviewers, added: tcberner; removed: adridg.
adridg updated this revision to Diff 13529.Apr 17 2017, 12:07 PM
  • Brain-o on the OpenBSD code
  • Reduce whitespace changes
  • Update comment
This revision was automatically updated to reflect the committed changes.