JDK-8262957 (fs) Fail fast in UnixFileStore.isExtendedAttributesEnabled #2816

overheadhunter · 2021-03-03T22:15:38Z

Added new capability flag UnixNativeDispatcher.SUPPORTS_XATTR, which gets set #ifdef _SYS_XATTR_H_.

Note that _SYS_XATTR_H_ is defined in xattr.h in both macOS/Darwin, Linux and GNU-based BSD systems using glibc. It might not be defined for other operating systems that still support xattr. So if OpenJDK eventually adds support for further platforms, this might need to be adjusted as well.

If xattr capabilities are missing, UnixFileStore.isExtendedAttributesEnabled will return false immediately, avoiding any I/O.

As a side effect of this change, I redefined some other (private) capabilities. Strictly speaking, this change is not required but keeps the code consistent, as previously discussed in the mailing list.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8262957: (fs) Fail fast in UnixFileStore.isExtendedAttributesEnabled

Reviewers

Alan Bateman (@AlanBateman - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/2816/head:pull/2816
$ git checkout pull/2816

Update a local copy of the PR:
$ git checkout pull/2816
$ git pull https://git.openjdk.java.net/jdk pull/2816/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 2816

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/2816.diff

…ispatcher doesn't support xattr

bridgekeeper · 2021-03-03T22:19:43Z

👋 Welcome back overheadhunter! 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 · 2021-03-03T22:20:53Z

@overheadhunter 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 · 2021-03-03T22:23:53Z

Webrevs

00: Full (6bd45ec)

bplb · 2021-03-04T02:10:57Z

I think this look all right. I'll withhold approving until @AlanBateman has commented.

AlanBateman · 2021-03-04T15:15:00Z

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

+            // avoid I/O if native code doesn't support xattr
+            return false;
+        }
+


Add SUPPORTS_XATTR is a good idea.
Style-wise, it might be a bit neater to use if (UnixNativeDispatcher.xattrSupported()) { ...} so there will only one "return false" code path.
Looking at this code now make me wonder about the close(fd) for the case that the open fails. It should be checking if fd or there should be a nested try-finally so that close is only invoked when the open succeeds.

Style-wise, it might be a bit neater to use if (UnixNativeDispatcher.xattrSupported()) { ...} so there will only one "return false" code path.

I prefer to avoid nested blocks for improved readability, but thats mostly a matter of taste. If you like, I can reverse the "if".

Looking at this code now make me wonder about the close(fd) for the case that the open fails. It should be checking if fd or there should be a nested try-finally so that close is only invoked when the open succeeds.

True. But I guess that would be off-topic in this PR?

It's somewhat subjective. What you have is fine, I just would have done is differently to avoid too many return paths.

I think we should clean up the close(-1) case while we are in the area, separate PR of course.

All right, will you create another bug report for the close(-1) issue?

I just wanted to create another issue, but then I noticed that close(-1) is allowed as per contract:

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

Lines 90 to 97 in 45e1bab

/**

* close(int filedes). If fd is -1 this is a no-op.

*/

static void close(int fd) {

if (fd != -1) {

close0(fd);

}

}

We can still change this, of course, but I wouldn't consider this a necessary change for this PR and would therefore suggest to deal with this another time.

Okay, I had forgotten that that UnixNativeDispatcher.close(-1) is a no-op.

I guess we're good to merge, then?

overheadhunter · 2021-04-05T05:09:06Z

@bplb @AlanBateman Bump 😉

openjdk · 2021-04-07T16:52:39Z

@overheadhunter 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:

8262957: (fs) Fail fast in UnixFileStore.isExtendedAttributesEnabled

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 501 new commits pushed to the master branch:

d3fdd73: 8264173: [s390] Improve Hardware Feature Detection And Reporting
9d65039: 8263984: Invalidate printServices when there are no printers
adb860e: 8255800: Raster creation methods need some specification clean up
eab8455: 8261137: Optimization of Box nodes in uncommon_trap
92fad1b: 8264680: Use the blessed modifier order in java.desktop
17202c8: 8264748: Do not include arguments.hpp from compilerDefinitions.hpp
c3abdc9: 8264797: Do not include klassVtable.hpp from instanceKlass.hpp
eb5c097: 8262389: Use permitted_enctypes if default_tkt_enctypes or default_tgs_enctypes is not present
bfb034a: 8264524: jdk/internal/platform/docker/TestDockerMemoryMetrics.java fails due to swapping not working
a756d8d: 8264759: x86_32 Minimal VM build failure after JDK-8262355
... and 491 more: https://git.openjdk.java.net/jdk/compare/b397472e4f030e546fd6c565c336dd2fb0a6f60e...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@AlanBateman) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

overheadhunter · 2021-04-07T17:00:39Z

/integrate

openjdk · 2021-04-07T17:02:01Z

@overheadhunter
Your change (at version 6bd45ec) is now ready to be sponsored by a Committer.

shipilev · 2021-04-12T06:18:57Z

Alan, are you good with integrating this?

AlanBateman · 2021-04-12T06:42:17Z

Alan, are you good with integrating this?

Yes, one I get time to do more testing (the optimisation is for AIX only and I want to double check that we don't have side effects on the main stream platforms).

AlanBateman · 2021-04-14T09:54:46Z

/sponsor

openjdk · 2021-04-14T09:56:20Z

@AlanBateman @overheadhunter Since your change was applied there have been 592 commits pushed to the master branch:

e2106d5: 8265104: CpuLoad and SystemCpuLoad in OperatingSystem MXBean returns -1.0
ebbce91: 8264681: Use the blessed modifier order in java.security
ea5c55a: 8265103: Remove unnecessary inclusion of oopMap.hpp
26186ec: 8039261: [TEST_BUG]: There is not a minimal security level in Java Preferences and the TestApplet.html is blocked.
283d64f: 8262896: [macos_aarch64] Crash in jni_fast_GetLongField
55d5649: 8263157: [macos]: java.library.path is being set incorrectly
e80012e: 8264768: JFR: Allow events to be printed to the log
3b576ed: 8265100: (fs) WindowsFileStore.hashCode() should read cached hash code once
8df8512: 8265125: IGV: cannot edit forms with NetBeans GUI builder
9cd5400: 8265138: Simplify DerUtils::checkAlg
... and 582 more: https://git.openjdk.java.net/jdk/compare/b397472e4f030e546fd6c565c336dd2fb0a6f60e...master

Your commit was automatically rebased without conflicts.

Pushed as commit 27dd88b.

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

ajbaldocchi · 2021-09-30T04:02:24Z

Doesn't glibc use _SYS_XATTR_H rather than _SYS_XATTR_H_? Wouldn't it be better to use something like XATTR_CREATE instead of _SYS_XATTR_H_?

Fail fast in UnixFileStore.isExtendedAttributesEnabled if UnixNativeD…

6bd45ec

…ispatcher doesn't support xattr

openjdk bot added the rfr Pull request is ready for review label Mar 3, 2021

openjdk bot added the nio nio-dev@openjdk.org label Mar 3, 2021

overheadhunter changed the title ~~JDK-8262957 Fail fast in UnixFileStore.isExtendedAttributesEnabled~~ JDK-8262957 (fs) Fail fast in UnixFileStore.isExtendedAttributesEnabled Mar 3, 2021

AlanBateman reviewed Mar 4, 2021

View reviewed changes

AlanBateman approved these changes Apr 7, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Apr 7, 2021

openjdk bot added the sponsor Pull request is ready to be sponsored label Apr 7, 2021

openjdk bot closed this Apr 14, 2021

openjdk bot added integrated Pull request has been integrated and removed sponsor Pull request is ready to be sponsored ready Pull request is ready to be integrated rfr Pull request is ready for review labels Apr 14, 2021

overheadhunter deleted the JDK-8262957 branch April 14, 2021 12:01

Dmor01 mentioned this pull request Sep 30, 2021

JDK-8274562: fs determines unsupported UserDefinedFileAttributeView due to fastfail mechanism #5767

Closed

3 tasks

	/**
	* close(int filedes). If fd is -1 this is a no-op.
	*/
	static void close(int fd) {
	if (fd != -1) {
	close0(fd);
	}
	}

JDK-8262957 (fs) Fail fast in UnixFileStore.isExtendedAttributesEnabled #2816

JDK-8262957 (fs) Fail fast in UnixFileStore.isExtendedAttributesEnabled #2816

Uh oh!

Conversation

overheadhunter commented Mar 3, 2021 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Mar 3, 2021

Uh oh!

openjdk bot commented Mar 3, 2021

Uh oh!

mlbridge bot commented Mar 3, 2021

Webrevs

Uh oh!

bplb commented Mar 4, 2021

Uh oh!

AlanBateman Mar 4, 2021

Choose a reason for hiding this comment

Uh oh!

overheadhunter Mar 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlanBateman Mar 4, 2021

Choose a reason for hiding this comment

Uh oh!

overheadhunter Mar 5, 2021

Choose a reason for hiding this comment

Uh oh!

overheadhunter Mar 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlanBateman Mar 24, 2021

Choose a reason for hiding this comment

Uh oh!

overheadhunter Mar 26, 2021

Choose a reason for hiding this comment

Uh oh!

overheadhunter commented Apr 5, 2021

Uh oh!

openjdk bot commented Apr 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

overheadhunter commented Apr 7, 2021

Uh oh!

openjdk bot commented Apr 7, 2021

Uh oh!

shipilev commented Apr 12, 2021

Uh oh!

AlanBateman commented Apr 12, 2021

Uh oh!

AlanBateman commented Apr 14, 2021

Uh oh!

openjdk bot commented Apr 14, 2021

Uh oh!

ajbaldocchi commented Sep 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

overheadhunter commented Mar 3, 2021 •

edited by openjdk bot

Loading

overheadhunter Mar 4, 2021 •

edited

Loading

overheadhunter Mar 24, 2021 •

edited

Loading

openjdk bot commented Apr 7, 2021 •

edited

Loading

ajbaldocchi commented Sep 30, 2021 •

edited

Loading