8343417: (fs) BasicFileAttributeView.setTimes uses microsecond precision with NOFOLLOW_LINKS #21886

bplb · 2024-11-04T19:30:11Z

Add support for setting the last access and last modification times of symbolic links with nanosecond precision on Linux where the system call utimensat(2) is available.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8343417: (fs) BasicFileAttributeView.setTimes uses microsecond precision with NOFOLLOW_LINKS (Bug - P4)

Reviewers

Alan Bateman (@AlanBateman - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/21886/head:pull/21886
$ git checkout pull/21886

Update a local copy of the PR:
$ git checkout pull/21886
$ git pull https://git.openjdk.org/jdk.git pull/21886/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 21886

View PR using the GUI difftool:
$ git pr show -t 21886

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/21886.diff

Using Webrev

Link to Webrev Comment

…ion with NOFOLLOW_LINKS

bridgekeeper · 2024-11-04T19:30:59Z

👋 Welcome back bpb! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2024-11-04T19:31:31Z

@bplb This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8343417: (fs) BasicFileAttributeView.setTimes uses microsecond precision with NOFOLLOW_LINKS

Reviewed-by: alanb

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 89 new commits pushed to the master branch:

d3c042f: 8343770: Build fails due to use of sun.misc.Unsafe in LoopOverRandom
1d117f6: 8343394: Make MemorySessionImpl.state a stable field
d2b681d: 8343730: JMX cleanups
ac82a8f: 8343610: InOutPathTest jpackage test produces invalid app image on macOS
f0b251d: 8343531: Improve print_location for invalid heap pointers
4244682: 8339190: Parameter arrays that are capped during annotation processing report incorrect length
7620b12: 8323803: ConstantOopReadValue::print_on should print 'null' instead of 'nullptr'
592a48b: 8321997: Increase upper limit of LoopOptsCount flag
c3df050: 8343726: [BACKOUT] NMT should not use ThreadCritical
0e1c1b7: 8343452: Incorrect WINDOWS build variable is used in macroAssembler_x86.cpp
... and 79 more: https://git.openjdk.org/jdk/compare/ea110c35f5429f1e1de57a301e2256f508a4c324...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk · 2024-11-04T19:32:04Z

@bplb The following label will be automatically applied to this pull request:

nio

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2024-11-04T19:35:01Z

Webrevs

AlanBateman · 2024-11-05T12:04:25Z

src/java.base/unix/classes/sun/nio/fs/UnixNativeDispatcher.java

+     *
+     * We hard code fd to FD_ATCWD and flags to AT_SYMLINK_NOFOLLOW.
+     */
+    static void utimensat(UnixPath path, long times0, long times1)


The *at methods that are supposed to map 1-1 to the similarly named sys calls so I'm a bit uncomfortable having this one work differently and hardcode dirfd to FD_ATCWD. I wonder if we could generate a value for FD_ATCWD in UnixConstants.java.template so that the use site would more closely match the sys call.

I wonder if we could generate a value for FD_ATCWD in UnixConstants.java.template so that the use site would more closely match the sys call.

Sure, no problem.

So changed in 26feef6.

…responding system call

AlanBateman · 2024-11-05T18:39:09Z

src/java.base/unix/classes/sun/nio/fs/UnixFileAttributeViews.java

                }
-                if (!useLutimes) {
+                if (!useUtimensat && !useLutimes) {


Changing the utimesat method to match the sys call is good. Now I wonder if we go further and use fd from openForAttribtueAccess(false) with utimesat. That would remove the window between the lstat and utimesat(AT_FDCWD...) where the file may be replaced.

Indeed UnixFileAttributes.get calls lstat. Calling symlink.openForAttributeAccess(false) however yields

java.nio.file.FileSystemException: symlink: No such file or directory or unable to access attributes of symbolic link

According to symlink(7):

The last access and last modification timestamps of a symbolic link can be changed using utimensat(2) or lutimes(3).

It looks like futimens will not work with a symlink's fd so that utimensat is necessary here to obtain nsec precision. I don't see a way to close the window between lstat and utimensat, which anyway was already there for the lutimes case.

I don't think that the logic in 08f5b7d is quite correct yet. It might be possible if followLinks is false and the file is not a symlink, that lutimes would be used even though futimens is supported, thus decreasing the timestamp resolution.

AlanBateman · 2024-11-06T09:17:54Z

I think what you have is correct now but it's hard to follow as it's one of 4 sys calls. What would you think about use a local enum class and a switch and get rid of the 4 booleans. Alternatively, invert the setup so that if the follow case is first. The follow case should map to fd+futimens or utimes, no need for futimes. For the !follow case then it should map to lutimesnsat or lutimes. I realise this is adding to the work on this change but right now it's hard for anyone to touch this method due to the many cases.

bplb · 2024-11-06T15:36:51Z

I think what you have is correct now but it's hard to follow as it's one of 4 sys calls. [...] I realise this is adding to the work on this change but right now it's hard for anyone to touch this method due to the many cases.

It definitely needs reworking but I do not know the details as yet.

AlanBateman

Discussed with Brian about some refactoring setTimes to make it more maintainable, and maybe drop the use of methods that only support micro seconds precision The AIX port may hold us back, not sure. So approving for now, there will be cleanup to follow.

bplb · 2024-11-07T17:18:20Z

/integrate

openjdk · 2024-11-07T17:19:30Z

Going to push as commit 56c588b.
Since your change was applied there have been 89 commits pushed to the master branch:

d3c042f: 8343770: Build fails due to use of sun.misc.Unsafe in LoopOverRandom
1d117f6: 8343394: Make MemorySessionImpl.state a stable field
d2b681d: 8343730: JMX cleanups
ac82a8f: 8343610: InOutPathTest jpackage test produces invalid app image on macOS
f0b251d: 8343531: Improve print_location for invalid heap pointers
4244682: 8339190: Parameter arrays that are capped during annotation processing report incorrect length
7620b12: 8323803: ConstantOopReadValue::print_on should print 'null' instead of 'nullptr'
592a48b: 8321997: Increase upper limit of LoopOptsCount flag
c3df050: 8343726: [BACKOUT] NMT should not use ThreadCritical
0e1c1b7: 8343452: Incorrect WINDOWS build variable is used in macroAssembler_x86.cpp
... and 79 more: https://git.openjdk.org/jdk/compare/ea110c35f5429f1e1de57a301e2256f508a4c324...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-11-07T17:19:39Z

@bplb Pushed as commit 56c588b.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

8343417: (fs) BasicFileAttributeView.setTimes uses microsecond precis…

7628043

…ion with NOFOLLOW_LINKS

openjdk bot added the rfr Pull request is ready for review label Nov 4, 2024

openjdk bot added the nio nio-dev@openjdk.org label Nov 4, 2024

AlanBateman reviewed Nov 5, 2024

View reviewed changes

8343417: Make Java method utimensat() signature match that of the cor…

26feef6

…responding system call

AlanBateman reviewed Nov 5, 2024

View reviewed changes

8343417: Use utimensat or lutimes when not following links

08f5b7d

AlanBateman approved these changes Nov 7, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Nov 7, 2024

openjdk bot added the integrated Pull request has been integrated label Nov 7, 2024

openjdk bot closed this Nov 7, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Nov 7, 2024

bplb deleted the utimensat-8343417 branch November 12, 2024 21:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8343417: (fs) BasicFileAttributeView.setTimes uses microsecond precision with NOFOLLOW_LINKS #21886

8343417: (fs) BasicFileAttributeView.setTimes uses microsecond precision with NOFOLLOW_LINKS #21886

bplb commented Nov 4, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Nov 4, 2024

openjdk bot commented Nov 4, 2024 •

edited

Loading

openjdk bot commented Nov 4, 2024

mlbridge bot commented Nov 4, 2024 •

edited

Loading

AlanBateman Nov 5, 2024

bplb Nov 5, 2024

bplb Nov 5, 2024

AlanBateman Nov 5, 2024 •

edited

Loading

bplb Nov 5, 2024

bplb Nov 6, 2024

AlanBateman commented Nov 6, 2024

bplb commented Nov 6, 2024

AlanBateman left a comment

bplb commented Nov 7, 2024

openjdk bot commented Nov 7, 2024

openjdk bot commented Nov 7, 2024

8343417: (fs) BasicFileAttributeView.setTimes uses microsecond precision with NOFOLLOW_LINKS #21886

8343417: (fs) BasicFileAttributeView.setTimes uses microsecond precision with NOFOLLOW_LINKS #21886

Conversation

bplb commented Nov 4, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Nov 4, 2024

openjdk bot commented Nov 4, 2024 • edited Loading

openjdk bot commented Nov 4, 2024

mlbridge bot commented Nov 4, 2024 • edited Loading

Webrevs

AlanBateman Nov 5, 2024

Choose a reason for hiding this comment

bplb Nov 5, 2024

Choose a reason for hiding this comment

bplb Nov 5, 2024

Choose a reason for hiding this comment

AlanBateman Nov 5, 2024 • edited Loading

Choose a reason for hiding this comment

bplb Nov 5, 2024

Choose a reason for hiding this comment

bplb Nov 6, 2024

Choose a reason for hiding this comment

AlanBateman commented Nov 6, 2024

bplb commented Nov 6, 2024

AlanBateman left a comment

Choose a reason for hiding this comment

bplb commented Nov 7, 2024

openjdk bot commented Nov 7, 2024

openjdk bot commented Nov 7, 2024

bplb commented Nov 4, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Nov 4, 2024 •

edited

Loading

mlbridge bot commented Nov 4, 2024 •

edited

Loading

AlanBateman Nov 5, 2024 •

edited

Loading