Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/java.base/unix/classes/sun/nio/fs/UnixFileStore.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,11 @@ public Object getAttribute(String attribute) throws IOException {
* @return <code>true</code> if enabled, <code>false</code> if disabled or unable to determine
*/
protected boolean isExtendedAttributesEnabled(UnixPath path) {
if (!UnixNativeDispatcher.xattrSupported()) {
// avoid I/O if native code doesn't support xattr
return false;
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@overheadhunter overheadhunter Mar 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@overheadhunter overheadhunter Mar 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we're good to merge, then?

int fd = -1;
try {
fd = path.openForAttributeAccess(false);
Expand Down
12 changes: 10 additions & 2 deletions src/java.base/unix/classes/sun/nio/fs/UnixNativeDispatcher.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -614,8 +614,9 @@ static native int flistxattr(int filedes, long listAddress, int size)
*/
private static final int SUPPORTS_OPENAT = 1 << 1; // syscalls
private static final int SUPPORTS_FUTIMES = 1 << 2;
private static final int SUPPORTS_FUTIMENS = 1 << 4;
private static final int SUPPORTS_LUTIMES = 1 << 8;
private static final int SUPPORTS_FUTIMENS = 1 << 3;
private static final int SUPPORTS_LUTIMES = 1 << 4;
private static final int SUPPORTS_XATTR = 1 << 5;
private static final int SUPPORTS_BIRTHTIME = 1 << 16; // other features
private static final int capabilities;

Expand Down Expand Up @@ -654,6 +655,13 @@ static boolean birthtimeSupported() {
return (capabilities & SUPPORTS_BIRTHTIME) != 0;
}

/**
* Supports extended attributes
*/
static boolean xattrSupported() {
return (capabilities & SUPPORTS_XATTR) != 0;
}

private static native int init();
static {
jdk.internal.loader.BootLoader.loadLibrary("nio");
Expand Down
6 changes: 6 additions & 0 deletions src/java.base/unix/native/libnio/fs/UnixNativeDispatcher.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -315,6 +315,12 @@ Java_sun_nio_fs_UnixNativeDispatcher_init(JNIEnv* env, jclass this)
capabilities |= sun_nio_fs_UnixNativeDispatcher_SUPPORTS_BIRTHTIME;
#endif

/* supports extended attributes */

#ifdef _SYS_XATTR_H_
capabilities |= sun_nio_fs_UnixNativeDispatcher_SUPPORTS_XATTR;
#endif

return capabilities;
}

Expand Down