Skip to content
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.

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

JDK-8260966 (fs) Consolidate Linux and macOS implementations of UserDefinedFileAttributeView #2604

Conversation

overheadhunter
Copy link
Contributor

@overheadhunter overheadhunter commented Feb 17, 2021

Deduplicated code in these classes:

  • LinuxUserDefinedAttributeView + BsdUserDefinedAttributeViewUnixUserDefinedAttributeView. Due to different supported length of attribute names, I added an abstract method UnixUserDefinedAttributeView.maxNameLength().
  • LinuxNativeDispatcher + BsdNativeDispatcherUnixNativeDispatcher. I basically just moved the Linux implementation up to Unix and added preprocessor directives to distinguish Linux/BSD/Other. Others will throw ENOTSUP UnixExceptions.
  • LinuxFileStore.isExtendedAttributesEnabled() + BsdFileStore.isExtendedAttributesEnabled()UnixFileStore.isExtendedAttributesEnabled()

For the latter I introduced UnixConstants.XATTR_NOT_FOUND, which is ENODATA on Linux and ENOATTR on BSD. Note that UnixConstants.ENODATA is still present, as @AlanBateman "would prefer if we left ENODATA so that it can be used in Linux specific code" (Quote from #2363 (comment))

This also fixes Files.copy(src, dst, StandardCopyOption.COPY_ATTRIBUTES) on macOS/BSD. Previosly an invocation was missing.

I plan to add further commits to clean up this code, once the deduplication is reviewed.


Progress

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

Issues

  • JDK-8260966: (fs) Consolidate Linux and macOS implementations of UserDefinedFileAttributeView
  • JDK-8260691: (fs) LinuxNativeDispatcher should link to xattr functions

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/2604/head:pull/2604
$ git checkout pull/2604

@bridgekeeper
Copy link

bridgekeeper bot commented Feb 17, 2021

👋 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.

@overheadhunter
Copy link
Contributor Author

overheadhunter commented Feb 17, 2021

/issue add JDK-8260691

@openjdk
Copy link

openjdk bot commented Feb 17, 2021

@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.

@openjdk openjdk bot added the nio nio-dev@openjdk.org label Feb 17, 2021
@openjdk
Copy link

openjdk bot commented Feb 17, 2021

@overheadhunter
Adding additional issue to issue list: 8260691: (fs) LinuxNativeDispatcher should link to xattr functions.

@overheadhunter overheadhunter marked this pull request as ready for review Feb 17, 2021
@openjdk openjdk bot added the rfr Pull request is ready for review label Feb 17, 2021
@mlbridge
Copy link

mlbridge bot commented Feb 17, 2021

Webrevs

@AlanBateman
Copy link
Contributor

AlanBateman commented Feb 17, 2021

Thanks for spitting up the steps in the refactoring as it makes it easier to review. One other thing that is still creating noise in the patch is that I think your IDE or editor has changed the formatting to 8 space indent in several places where the original code in LinuxUserDefinedFileAttributeView.java was 4 space indent. We've typically used 4 space indent in this code. If you revert those indentations then I think it will reduce the size of the patch and make it easy for us to see where the real changes are.

@mlbridge
Copy link

mlbridge bot commented Feb 17, 2021

Mailing list message from Sebastian Stenzel on nio-dev:

Can you show me an example, where you see an 8 space indentation?

@AlanBateman
Copy link
Contributor

AlanBateman commented Feb 17, 2021

Mailing list message from Sebastian Stenzel on nio-dev:

Can you show me an example, where you see an 8 space indentation?

UnixUserDefinedFileAttributeView L41, L58, L73, L125-126, L151-152, L207, ... so looks like changes from the original code in LinuxUserDefinedFileAttributeView, might be the IDE did it. It's not a big deal, just noise in the patch.

@overheadhunter
Copy link
Contributor Author

overheadhunter commented Feb 17, 2021

@bplb @AlanBateman Now that there is a webrev just for the deduplication, can I already amend further changes or wait for each set of changes to be reviewed individually? Sorry, I'm still new to the processes.

@bplb
Copy link
Member

bplb commented Feb 17, 2021

It might be better to split the changes across two or three PRs, as appropriate.

@AlanBateman
Copy link
Contributor

AlanBateman commented Feb 17, 2021

Thanks for the update patch, this is much easier to read now. Overall I think this looks good with only a few minor discussion points that I'll add to the PR. I agree with @bplb that it's best to integrate this once we've finished the review. Another issue/PR can be used for the other refactoring/modernization that you want to do.

@overheadhunter
Copy link
Contributor Author

overheadhunter commented Feb 19, 2021

I originally intended to also add one further commit that avoids unnecessary I/O in UnixFileStore.isExtendedAttributesEnabled() if UnixNativeDispatcher doesn't support xattr at all. This is not strictly deduplication but rather an enhancement, so I'm fine with deferring it to another PR.

Also, I deduplicated code inside of UnixUserDefinedAttributeView. While this is in the spirit of this issue, it is a quite large diff.

Just let me know, if you think there is anything more to be done in this PR.

@overheadhunter
Copy link
Contributor Author

overheadhunter commented Feb 27, 2021

Anything I need to do to request a review?

Copy link
Contributor

@AlanBateman AlanBateman left a comment

Overall I think this looks good, just a few minor nits with the method descriptions on protected methods (they don't actually need to be protected of course).

private static final String USER_NAMESPACE = "user.";

// returns the maximum supported length of xattr names (in bytes, including namespace)
protected abstract int maxNameLength();
Copy link
Contributor

@AlanBateman AlanBateman Mar 1, 2021

Choose a reason for hiding this comment

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

I think we should move this declaration to after the constructor to avoid having an abstract method declared in the middle of private API elements. I'd probably use /** .. */ style for the method description as it's not private.

Copy link
Contributor Author

@overheadhunter overheadhunter Mar 1, 2021

Choose a reason for hiding this comment

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

Will change the comment style.

Regarding method order: I agree. However, this class needs refactoring due to code duplication and complex branching anyway, as I have proposed in my original PR. I can sure move this single method, but then it will still be "field, field, method, method, field, field, constructor, method, ..."

Having this in mind, can you confirm this should be done in this PR or don't you think it'd fit into the follow-up cleanup better?

Copy link
Contributor

@AlanBateman AlanBateman Mar 1, 2021

Choose a reason for hiding this comment

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

If it's okay with you, then I'd prefer to do it in this PR. So let's get this PR out of the way and then discuss the refactoring in a follow-on PR.

@@ -172,6 +172,27 @@ public Object getAttribute(String attribute) throws IOException {
throw new UnsupportedOperationException("'" + attribute + "' not recognized");
}

// returns true if extended attributes enabled on file system where given
// file resides, returns false if disabled or unable to determine.
protected boolean isExtendedAttributesEnabled(UnixPath path) {
Copy link
Contributor

@AlanBateman AlanBateman Mar 1, 2021

Choose a reason for hiding this comment

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

This used to be private method in LinuxFileStore, now it's a protected method so might be cleaner to change the method description to use /** .. */ comment as it's used from sub-classes.

@overheadhunter
Copy link
Contributor Author

overheadhunter commented Mar 2, 2021

Applied discussed changes. @AlanBateman can you sponsor this?

Copy link
Contributor

@AlanBateman AlanBateman left a comment

Thanks for the update.
A minor nit is that the new javadoc for isExtendedAttributesEnabled means there is 100+ line in the source file, mildly annoying when reviewing diffs in side-by-side view.

@openjdk
Copy link

openjdk bot commented Mar 2, 2021

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

8260966: (fs) Consolidate Linux and macOS implementations of UserDefinedFileAttributeView
8260691: (fs) LinuxNativeDispatcher should link to xattr functions

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

  • 5f4bc0a: 8253100: Fix "no comment" warnings in java.base/java.net
  • d185a6c: 8261483: jdk/dynalink/TypeConverterFactoryMemoryLeakTest.java failed with "AssertionError: Should have GCd a method handle by now"
  • 85a5ae8: 8261606: Surprising behavior of step over in String switch
  • be67aaa: 8262726: AArch64: C1 StubAssembler::call_RT can corrupt stack
  • 0f6122b: 8262819: gc/shenandoah/compiler/TestLinkToNativeRBP.java fails with release VMs
  • dd33a8e: 8262461: handle wcstombsdmp return value correctly in unix awt_InputMethod.c
  • 3b350ad: 8261710: SA DSO objects have sizes that are too large
  • fdd1093: 8261552: s390: MacroAssembler::encode_klass_not_null() may produce wrong results for non-zero values of narrow klass base
  • f5ab7f6: 8262472: Buffer overflow in UNICODE::as_utf8 for zero length output buffer
  • 6635d7a: 8261670: Add javadoc for the XML processing limits
  • ... and 159 more: https://git.openjdk.java.net/jdk/compare/d19503353e5c347ce393544a3a30d5caec53d133...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).

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Mar 2, 2021
@overheadhunter
Copy link
Contributor Author

overheadhunter commented Mar 2, 2021

/integrate

@openjdk openjdk bot added the sponsor Pull request is ready to be sponsored label Mar 2, 2021
@openjdk
Copy link

openjdk bot commented Mar 2, 2021

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

@overheadhunter
Copy link
Contributor Author

overheadhunter commented Mar 2, 2021

A minor nit is that the new javadoc for isExtendedAttributesEnabled means there is 100+ line in the source file, mildly annoying when reviewing diffs in side-by-side view.

Just out of curiosity, since I only see the diff in GitHub or via my local git client: Are these webrev things somehow related to the review process and mess this up? In an earlier comment, brian suggested that individual commits in git are somehow "squashed" by webrev, causing all the review pain in the original PR in the first place.

I know this is off-topic, but I just want to understand what things I need to be aware of, if I contribute further patches. I have the feeling that Skara wasn't so much about transitioning to git and GitHub but rather added a bridge between those and some internal tooling.

@AlanBateman
Copy link
Contributor

AlanBateman commented Mar 2, 2021

Just out of curiosity, since I only see the diff in GitHub or via my local git client: Are these webrev things somehow related to the review process and mess this up? In an earlier comment, brian suggested that individual commits in git are somehow "squashed" by webrev, causing all the review pain in the original PR in the first place.

I know this is off-topic, but I just want to understand what things I need to be aware of, if I contribute further patches. I have the feeling that Skara wasn't so much about transitioning to git and GitHub but rather added a bridge between those and some internal tooling.

Some people look at the diffs in on github where it's possible to customize the view. Some people look use the generated webrev. There's a bot that generates the webrev and adds a link to the PR, look for the comment from "mlbridge" ("mailing list bridge").

@AlanBateman
Copy link
Contributor

AlanBateman commented Mar 2, 2021

/integrate

@openjdk
Copy link

openjdk bot commented Mar 2, 2021

@AlanBateman Only the author (@overheadhunter) is allowed to issue the integrate command. As this PR is ready to be sponsored, and you are an eligible sponsor, did you mean to issue the /sponsor command?

@AlanBateman
Copy link
Contributor

AlanBateman commented Mar 2, 2021

/sponsor

@openjdk openjdk bot closed this Mar 2, 2021
@openjdk 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 Mar 2, 2021
@openjdk
Copy link

openjdk bot commented Mar 2, 2021

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

  • 5f4bc0a: 8253100: Fix "no comment" warnings in java.base/java.net
  • d185a6c: 8261483: jdk/dynalink/TypeConverterFactoryMemoryLeakTest.java failed with "AssertionError: Should have GCd a method handle by now"
  • 85a5ae8: 8261606: Surprising behavior of step over in String switch
  • be67aaa: 8262726: AArch64: C1 StubAssembler::call_RT can corrupt stack
  • 0f6122b: 8262819: gc/shenandoah/compiler/TestLinkToNativeRBP.java fails with release VMs
  • dd33a8e: 8262461: handle wcstombsdmp return value correctly in unix awt_InputMethod.c
  • 3b350ad: 8261710: SA DSO objects have sizes that are too large
  • fdd1093: 8261552: s390: MacroAssembler::encode_klass_not_null() may produce wrong results for non-zero values of narrow klass base
  • f5ab7f6: 8262472: Buffer overflow in UNICODE::as_utf8 for zero length output buffer
  • 6635d7a: 8261670: Add javadoc for the XML processing limits
  • ... and 159 more: https://git.openjdk.java.net/jdk/compare/d19503353e5c347ce393544a3a30d5caec53d133...master

Your commit was automatically rebased without conflicts.

Pushed as commit 0de6abd.

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

@overheadhunter overheadhunter deleted the feature/deduplicate-UserDefinedFileAttributeView branch Mar 2, 2021
@tstuefe
Copy link
Member

tstuefe commented Mar 3, 2021

Unfortunately this broke AIX and I don't know if this would work on BSD (not sure if anyone still maintains those).

https://bugs.openjdk.java.net/browse/JDK-8262926

@mlbridge
Copy link

mlbridge bot commented Mar 3, 2021

Mailing list message from Sebastian Stenzel on nio-dev:

On 3. Mar 2021, at 07:53, Thomas Stuefe <stuefe at openjdk.java.net> wrote:

Unfortunately this broke AIX and I don't know if this would work on BSD (not sure if anyone still maintains those).

https://bugs.openjdk.java.net/browse/JDK-8262926 <https://bugs.openjdk.java.net/browse/JDK-8262926>

Oh sh... Well, since this is merely an access modifier, it is easy to fix.

While I could easily do this, I'd be flying blind here, since I can't test on AIX.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/nio-dev/attachments/20210303/02893ac8/attachment.htm>

@tstuefe
Copy link
Member

tstuefe commented Mar 3, 2021

If you do a PR, I can test it for you.

@tstuefe
Copy link
Member

tstuefe commented Mar 3, 2021

(you could leave it as draft PR to avoid spamming the mailing lists)

@mlbridge
Copy link

mlbridge bot commented Mar 3, 2021

Mailing list message from Alan Bateman on nio-dev:

On 03/03/2021 06:53, Thomas Stuefe wrote:

:
Unfortunately this broke AIX and I don't know if this would work on BSD (not sure if anyone still maintains those).

https://bugs.openjdk.java.net/browse/JDK-8262926

The BSD port is in maintained downstream rather than in OpenJDK and no
requirement that it builds on BSD before integrating changes here. It's
somewhat historical that there are Bsd* classes here, it dates from
initial macOS port and Apple contributions in 7u4.

We aren't required to do any building or testing on AIX prior to
integrating changes. Instead, we've always left it to SAP or IBM to keep
that port alive. So apologies, there will be periodic breakage and needs
someone with easy access to AIX systems to keep it working.

-Alan

@mlbridge
Copy link

mlbridge bot commented Mar 3, 2021

Mailing list message from Sebastian Stenzel on nio-dev:

On 3. Mar 2021, at 08:23, Thomas Stuefe <stuefe at openjdk.java.net> wrote:

On Wed, 3 Mar 2021 06:51:27 GMT, Thomas Stuefe <stuefe at openjdk.org <mailto:stuefe at openjdk.org>> wrote:

Just out of curiosity, since I only see the diff in GitHub or via my local git client: Are these webrev things somehow related to the review process and mess this up? In an earlier comment, brian suggested that individual commits in git are somehow "squashed" by webrev, causing all the review pain in the original PR in the first place.

I know this is off-topic, but I just want to understand what things I need to be aware of, if I contribute further patches. I have the feeling that Skara wasn't so much about transitioning to git and GitHub but rather added a bridge between those and some internal tooling.

Some people look at the diffs in on github where it's possible to customize the view. Some people look use the generated webrev. There's a bot that generates the webrev and adds a link to the PR, look for the comment from "mlbridge" ("mailing list bridge").

Unfortunately this broke AIX and I don't know if this would work on BSD (not sure if anyone still maintains those).

https://bugs.openjdk.java.net/browse/JDK-8262926 <https://bugs.openjdk.java.net/browse/JDK-8262926>

If you do a PR, I can test it for you.

Here you go: https://github.com//pull/2805 <https://github.com//pull/2805>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/nio-dev/attachments/20210303/6b7ee68a/attachment.htm>

@mlbridge
Copy link

mlbridge bot commented Mar 3, 2021

Mailing list message from Thomas St��fe on nio-dev:

Hi Alan,

On Wed, Mar 3, 2021 at 8:31 AM Alan Bateman <Alan.Bateman at oracle.com> wrote:

On 03/03/2021 06:53, Thomas Stuefe wrote:

:
Unfortunately this broke AIX and I don't know if this would work on BSD
(not sure if anyone still maintains those).

https://bugs.openjdk.java.net/browse/JDK-8262926

The BSD port is in maintained downstream rather than in OpenJDK and no
requirement that it builds on BSD before integrating changes here. It's
somewhat historical that there are Bsd* classes here, it dates from
initial macOS port and Apple contributions in 7u4.

We aren't required to do any building or testing on AIX prior to
integrating changes. Instead, we've always left it to SAP or IBM to keep
that port alive. So apologies, there will be periodic breakage and needs
someone with easy access to AIX systems to keep it working.

No problem, we are aware of that and usually fix without much noise. Also,
Oracle developers are usually very careful to at least "dry-fix" AIX or at
least ping us, for which we are thankful. On the whole this works quite
well.

In this case I was afraid, due to the PR title, that this fix would involve
major implementation work on our side. Since it looked like Sebastian
unified a lot of code for Linux and MacOs without being aware that there
are other supported Unices. From his initial response I take that this is
an easy fix though.

As for BSD, the hotspot os sources in particular contain code which seems
like it could have bitrotted for the non-mac-platforms. I always suspected
that these configurations are seldom built. But that is a different issue
of course.

Thanks, Thomas

-Alan

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/nio-dev/attachments/20210303/6b14ea23/attachment-0001.htm>

@tstuefe
Copy link
Member

tstuefe commented Mar 3, 2021

@ArnoZeller will take care of the AIX build issue. Thanks, Sebastian, for the quick response.

@mlbridge
Copy link

mlbridge bot commented Mar 3, 2021

Mailing list message from Sebastian Stenzel on nio-dev:

On 3. Mar 2021, at 10:51, Thomas Stuefe <stuefe at openjdk.java.net> wrote:

On Wed, 3 Mar 2021 07:26:51 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

If you do a PR, I can test it for you.

(you could leave it as draft PR to avoid spamming the mailing lists)

@ArnoZeller will take care of the AIX build issue. Thanks, Sebastian, for the quick response.

Maybe you didn't see my previous mail, but I created a PR already: https://github.com//pull/2805 <https://github.com//pull/2805>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/nio-dev/attachments/20210303/8627870b/attachment.htm>

@mlbridge
Copy link

mlbridge bot commented Mar 3, 2021

Mailing list message from Zeller, Arno on nio-dev:

Hi Sebastian,

your PR looks good and fixes the issue. I had a similar change open but not created a pull request till now. I tested it in our build environment and it works.

Best regards,
Arno
PS: I am not a (R)eviewer.

From: nio-dev <nio-dev-retn at openjdk.java.net> On Behalf Of Sebastian Stenzel
Sent: Mittwoch, 3. M?rz 2021 10:55
To: Thomas Stuefe <stuefe at openjdk.java.net>
Cc: nio-dev at openjdk.java.net
Subject: Re: RFR: JDK-8260966 (fs) Consolidate Linux and macOS implementations of UserDefinedFileAttributeView [v3]

On 3. Mar 2021, at 10:51, Thomas Stuefe <stuefe at openjdk.java.net<mailto:stuefe at openjdk.java.net>> wrote:

On Wed, 3 Mar 2021 07:26:51 GMT, Thomas Stuefe <stuefe at openjdk.org<mailto:stuefe at openjdk.org>> wrote:

If you do a PR, I can test it for you.

(you could leave it as draft PR to avoid spamming the mailing lists)

@ArnoZeller will take care of the AIX build issue. Thanks, Sebastian, for the quick response.

Maybe you didn't see my previous mail, but I created a PR already: https://github.com//pull/2805
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/nio-dev/attachments/20210303/80fdd28f/attachment-0001.htm>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated Pull request has been integrated nio nio-dev@openjdk.org
4 participants