Ticket #4542 (accepted defect)

Opened 2 months ago

Last modified 7 days ago

Timestamps with nanosecond precision support is broken on macOS

Reported by: zaytsev Owned by: zaytsev
Priority: major Milestone: 4.8.32
Component: mc-vfs Version: master
Keywords: Cc: ag
Blocked By: Blocking: #3960
Branch state: on review Votes for changeset:

Description

Followup ticket to #3575 and PR on GitHub? https://github.com/MidnightCommander/mc/pull/130

Sadly no-one came around to fix this, so packagers still disable the support manually:

https://github.com/Homebrew/homebrew-core/blob/master/Formula/m/midnight-commander.rb
https://github.com/macports/macports-ports/blame/master/sysutils/mc/Portfile

Here are the notes how to possible fix this:

---

Unfortunately, I think that the easiest way to access XNU sources from macOS 10.13 right now would be to get hired as a kernel developer by Apple. The system is not even officially released to the public yet (target date is 25th of September), and sources are usually released some time after the final release.

Anyways, I believe that source code access is not really necessary in the first place, as I would speculate that all one needs to do is to add

AC_CHECK_MEMBERS([struct stat.st_blksize, struct stat.st_rdev, struct stat.st_mtim, struct stat.st_mtimespec])

and wherever we use HAVE_STRUCT_STAT_ST_MTIM *or* HAVE_UTIMENSAT replace it with

#ifdef HAVE_STRUCT_STAT_ST_MTIM
    st.st_atim.tv_nsec = st.st_mtim.tv_nsec = st.st_ctim.tv_nsec = 0;
#elif defined HAVE_STRUCT_STAT_ST_MTIMESPEC
    st.st_atimespec.tv_nsec = st.st_mtimespec.tv_nsec = st.st_mtimespec.tv_nsec = 0;
#endif

or something along these lines and you will magically not only fix the build, but gain nanosecond precision support for macOS 10.13+ *and* BSD at the same time. Of course, cruft keeps accumulating, so ideally one should think about defining clever struct manipulation functions limiting define proliferation...

Better yet would be to undertake a major refactoring to clean up our time mess afterwards, but that's, of course, just wishful thinking.

Change History

comment:1 Changed 2 months ago by zaytsev

  • Status changed from new to accepted
  • Owner set to zaytsev

comment:2 Changed 7 weeks ago by zaytsev

Apparently, macOS fixed itself: https://github.com/Homebrew/homebrew-core/pull/173306

Need to check what's up in the BSD land.

comment:3 Changed 7 weeks ago by zaytsev

stat -f %Fm filename can be used if ls is unable to show nanosecond timestamps.

comment:5 Changed 7 weeks ago by zaytsev

Related code: #3960

comment:6 Changed 7 weeks ago by zaytsev

Related code: #3927

comment:7 Changed 7 weeks ago by zaytsev

  • Blocking 3960 added

comment:8 Changed 7 weeks ago by zaytsev

  • Branch state changed from no branch to on review
  • Milestone changed from Future Releases to 4.8.32

Branch: 4542_nanoseconds_cleanup
Initial changeset:934efd96383900ce38fb55d5592f6ac2aecb1280

Hi Andrew, appreciate reviewing this stuff, maybe you can check on Linux? I've checked it on macOS and it works, will be trying on AIX shortly, it's a bit of a pain to build. I hope my idea is clear. If not just ask. Please feel free to commit fixups directly if you don't like something. I tried my best to keep to your style.

Last edited 7 weeks ago by zaytsev (previous) (diff)

comment:9 Changed 7 weeks ago by andrew_b

Let's move cleanup stuff to new branch 4524_cleanup or commit it directly to master.

Move #include <config.h> to .h is not a good idea. config.h must be included first in .c files, but indirectly includes via other headers became it not first.
As a result, ENABLE_EXT2FS_ATTR is not defined in local.c here:

#ifdef ENABLE_EXT2FS_ATTR
#include <e2p/e2p.h>            /* fgetflags(), fsetflags() */
#endif

but defined here:

  CC       local.lo
src/vfs/local/local.c: In function 'local_fgetflags':
src/vfs/local/local.c:198:12: warning: implicit declaration of function 'fgetflags'; did you mean 'mc_fgetflags'? [-Wimplicit-function-declaration]
     return fgetflags (path, flags);
            ^~~~~~~~~
            mc_fgetflags
src/vfs/local/local.c: In function 'local_fsetflags':
src/vfs/local/local.c:209:12: warning: implicit declaration of function 'fsetflags'; did you mean 'mc_fsetflags'? [-Wimplicit-function-declaration]
     return fsetflags (path, flags);
            ^~~~~~~~~
            mc_fsetflags
Last edited 7 weeks ago by andrew_b (previous) (diff)

comment:10 follow-up: ↓ 11 Changed 7 weeks ago by zaytsev

Let's move cleanup stuff to new branch 4524_cleanup or commit it directly to master.

Sorry, I don't get what you want. I think it's better to have a separate branch for nanoseconds stuff instead of doing everything in cleanup. It's a new feature supporting it correctly on AIX & BSDs. The commits doing the refactoring make it possible to implement the new feature centrally, so I would also keep them to this branch.

If you want to take changeset:607fe8 and changeset:c50106 elsewhere, no problem, but I would prefer master, so that I can rebase this branch and continue testing. Regarding changeset:c50106, I'm not sure if something else has to be changed / removed. The function was present before glib 2.31 according to git blame of their tree, so maybe something went wrong at some point?

Move #include <config.h> to .h is not a good idea. config.h must be included first in .c files, but indirectly includes via other headers became it not first.

Ah, ok, thank you, I put it back. I thought it was unused, but I was wrong.

But am I allowed also to use config.h in h-files? Are there any rules for that? I need to know the structure of the struct and I want the functions to be in h-files, so that they can be inlined everywhere, where they are used.

comment:11 in reply to: ↑ 10 Changed 7 weeks ago by andrew_b

Replying to zaytsev:

If you want to take changeset:607fe8 and changeset:c50106 elsewhere, no problem, but I would prefer master,

Ok.

Regarding changeset:c50106, I'm not sure if something else has to be changed / removed. The function was present before glib 2.31 according to git blame of their tree, so maybe something went wrong at some point?

That's my mistake. There was no need to redefine g_direct_equal() in our code because it's implemented in glib since 2005.

Move #include <config.h> to .h is not a good idea. config.h must be included first in .c files, but indirectly includes via other headers became it not first.

Ah, ok, thank you, I put it back. I thought it was unused, but I was wrong.

But am I allowed also to use config.h in h-files?

I've never seen <config.h> included in h-files, only in c-files.

Are there any rules for that?

Probably yes, but Google can't help me.

I need to know the structure of the struct and I want the functions to be in h-files, so that they can be inlined everywhere, where they are used.

Your h-files are included in c-files after config.h so no problems.
But why you want make function inline?

comment:12 Changed 7 weeks ago by zaytsev

I've never seen <config.h> included in h-files, only in c-files.

Okay, I understand, so the rule is to use config.h in C-files so that it doesn't contaminate other files per mistake if used in an H-file, and use it first. I can check if there are other H-files where we include it by mistake.

But why you want make function inline?

These are nothing but accessor functions that reshuffle struct fields. Before my changes it was handled at every place where it was needed in the code. Now everywhere there is an extra function call, but the code is reused. I wanted to make it a function and not a macro, because macros are evil and non-type-safe. But it has some performance impact. For inline functions the impact is zero.

But if we don't care, then I can move these functions in a C-file, probably the impact will be some microseconds anyways which are not important...

comment:13 Changed 7 weeks ago by zaytsev

I have added a commit implementing this, took the two build fixes in master and rebased the branch squashing the fixups.

comment:14 Changed 5 weeks ago by andrew_b

I've created an alternative branch with some fixups: 4542_nanoseconds_2
Initial changeset:e89d65e17053e10912a2b1ed3afc9be06ec8c746

comment:15 Changed 5 weeks ago by andrew_b

  • Component changed from mc-core to mc-vfs
  • Type changed from enhancement to defect

comment:16 follow-up: ↓ 17 Changed 5 weeks ago by zaytsev

I'm a bit concerned about you removing config.h from vfs.h - it means vfs.h must be only included in C-files which include config.h themselves to work correctly. But I guess this is C life :-(

Thank you for the rest of the changes, I think I tried %ju but got some problems on Mac and AIX, and decided to use %llu. I will test again on Mac, AIX and I also wanted to check Solaris. Then report back.

comment:17 in reply to: ↑ 16 Changed 5 weeks ago by andrew_b

Replying to zaytsev:

I'm a bit concerned about you removing config.h from vfs.h - it means vfs.h must be only included in C-files which include config.h themselves to work correctly. But I guess this is C life :-(

Yes. We do compile c-files, not h-files. c-file must begin with include of config.h, then include system-wide h-files (from /usr/include), then project h-files.

Thank you for the rest of the changes, I think I tried %ju but got some problems on Mac and AIX, and decided to use %llu. I will test again on Mac, AIX and I also wanted to check Solaris. Then report back.

I get a warning for %llu:

src/vfs/shell/shell.c: In function 'shell_utime':
src/vfs/shell/shell.c:1490:69: warning: format '%llu' expects argument of type 'long long unsigned int', but argument 7 has type 'long unsigned int' [-Wformat=]
 1490 |                               "SHELL_FILENAME=%s SHELL_FILEATIME=%llu SHELL_FILEMTIME=%llu "
      |                                                                  ~~~^
      |                                                                     |
      |                                                                     long long unsigned int
      |                                                                  %lu
 1491 |                               "SHELL_TOUCHATIME=%s SHELL_TOUCHMTIME=%s SHELL_TOUCHATIME_W_NSEC=\"%s\" "
 1492 |                               "SHELL_TOUCHMTIME_W_NSEC=\"%s\";\n", rpath, (uintmax_t) atime.tv_sec,
      |                                                                           ~~~~~~~~~~~~~~~~~~~~~~~~
      |                                                                           |
      |                                                                           long unsigned int
src/vfs/shell/shell.c:1490:90: warning: format '%llu' expects argument of type 'long long unsigned int', but argument 8 has type 'long unsigned int' [-Wformat=]
 1490 |                               "SHELL_FILENAME=%s SHELL_FILEATIME=%llu SHELL_FILEMTIME=%llu "
      |                                                                                       ~~~^
      |                                                                                          |
      |                                                                                          long long unsigned int
      |                                                                                       %lu
......
 1493 |                               (uintmax_t) mtime.tv_sec, utcatime, utcmtime, utcatime_w_nsec,
      |                               ~~~~~~~~~~~~~~~~~~~~~~~~                                    
      |                               |
      |                               long unsigned int

and system_data_types(7) says:

The length modifier for uintmax_t for the printf(3) and the scanf(3)
families of functions is j; resulting commonly in %ju or %jx for printing
uintmax_t values.

Conforming to: C99 and later; POSIX.1-2001 and later.

comment:18 Changed 7 days ago by andrew_b

@zaytsev: well? What is your plan here?

comment:19 Changed 7 days ago by zaytsev

Been busy with other stuff, but probably should just test on Solaris and be done with it. Will see how I can get back to it.

Note: See TracTickets for help on using tickets.