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

8263898: (fs) Files.newOutputStream on the "NUL" special device throws FileSystemException: "nul: Incorrect function" (win) #3183

Closed
wants to merge 8 commits into from

Conversation

@bplb
Copy link
Member

@bplb bplb commented Mar 24, 2021

This request proposes to change the native method WindowsNativeDispatcher.SetEndOfFile() to succeed even if the Windows function SetEndOfFile() returns zero if GetFileSizeEx() called on the handle succeeds and reveals a file size of zero.


Progress

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

Issue

  • JDK-8263898: (fs) Files.newOutputStream on the "NUL" special device throws FileSystemException: "nul: Incorrect function" (win)

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3183

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

Using diff file

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

…s FileSystemException: "nul: Incorrect function" (win)
@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Mar 24, 2021

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

Loading

@openjdk openjdk bot added the rfr label Mar 24, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Mar 24, 2021

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

Loading

@openjdk openjdk bot added the nio label Mar 24, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Mar 24, 2021

Loading

} else if (size.QuadPart != 0) {
throwWindowsException(env, error);
}
}
}
Copy link
Contributor

@AlanBateman AlanBateman Mar 25, 2021

Choose a reason for hiding this comment

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

The native methods are meant to map to one win32 call where possible. In this case, I think the workaround needs to be in WindowsChannelFile.open.

Loading

Copy link
Member Author

@bplb bplb Mar 25, 2021

Choose a reason for hiding this comment

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

I know. I tried at the Java level first and one cannot verify the size of the NUL device from Java.

Loading

Copy link
Contributor

@AlanBateman AlanBateman Mar 25, 2021

Choose a reason for hiding this comment

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

it's okay to add a new method to WindowsNativeDispatcher if we need it.

Loading

Copy link
Member Author

@bplb bplb Mar 25, 2021

Choose a reason for hiding this comment

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

Added GetFileSizeEx().

Loading

* @summary Verify a byte can be written to the null device.
* @run main NullDevice
*/
public class NullDevice {
Copy link
Member

@jaikiran jaikiran Mar 25, 2021

Choose a reason for hiding this comment

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

Hello Brian,

Given what's being tested here, perhaps this test should only be run against Windows OS? So maybe something like @requires os.family=="windows"?

Loading

Copy link
Member Author

@bplb bplb Mar 25, 2021

Choose a reason for hiding this comment

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

You are correct, I intended to but forgot to add that.

Loading

Copy link
Member Author

@bplb bplb Mar 25, 2021

Choose a reason for hiding this comment

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

Added suggested @requires.

Loading

@openjdk openjdk bot removed the rfr label Mar 25, 2021
@openjdk openjdk bot added the rfr label Mar 25, 2021
Copy link
Member

@jaikiran jaikiran left a comment

The test case (changes) looks fine to me. Thank you Brian.

Loading

System.out.printf("Wrote a byte to %s%n", f);
}
}
}
Copy link
Contributor

@AlanBateman AlanBateman Mar 26, 2021

Choose a reason for hiding this comment

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

The updated changes to the Windows file system provider looks good. Have you checked that WindowsNativeDispatcher.GetFileSizeEx returns the correct size for a non-zero length file? (asking about that because it won't be exercised by the any of the tests and we might want to use it in order places).

The test name is "NullDevice" but it actually tests "nul" :-) so maybe it should be renamed. Files.newOutputStream is just one way to open the NUL device for writing. Would be good to also test opening it for wring without the TRUNCATE_EXISTING operation. I think we should do reading while we are there.
You can use Path.of("nul") to avoid first creating a java.io.File and using toPath.

Loading

Copy link
Member Author

@bplb bplb Mar 26, 2021

Choose a reason for hiding this comment

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

I don't have the length check in public code. I'll see how it is possible. I'll translate "null" into French and look at the reading and writing cases. I should have thought to use Path.of() especially given that I wrote it (JDK-8194746).

Loading

@bplb
Copy link
Member Author

@bplb bplb commented Mar 26, 2021

The test case (changes) looks fine to me. Thank you Brian.

Thanks

Loading

Copy link
Contributor

@AlanBateman AlanBateman left a comment

The change looks okay.

The test looks okay although there other cases that probably should be added at some point, in particular, FileChannel.open for reading, and AsynchronousFileChannel read & write.

Loading

*/
public class NulDevice {
public static void main(final String[] args) throws IOException {
System.getProperties().list(System.out);
Copy link
Contributor

@AlanBateman AlanBateman Mar 27, 2021

Choose a reason for hiding this comment

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

Is the printing of the system properties a left-over?

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Mar 27, 2021

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

8263898: (fs) Files.newOutputStream on the "NUL" special device throws FileSystemException: "nul: Incorrect function" (win)

Reviewed-by: jpai, 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 183 new commits pushed to the master branch:

  • 21e7402: 8263707: C1 RangeCheckEliminator support constant array and NewMultiArray
  • 2ad6f2d: 8263896: Make not_suspended parameter from ObjectMonitor::exit() have default value
  • b652198: 8264429: Test runtime/cds/appcds/VerifyWithDefaultArchive.java assumes OpenJDK build
  • 2c9365d: 8264271: Avoid creating non_oop_word oops
  • daeca3f: 8262958: (fs) UnixUserDefinedFileAttributeView cleanup
  • af02883: 8264191: Javadoc search is broken in Internet Explorer
  • 6e74c3a: 8264193: Remove TRAPS parameters for modules and defaultmethods
  • ee5e00b: 8264279: Shenandoah: Missing handshake after JDK-8263427
  • ac604a1: 8264374: Shenandoah: Remove leftover parallel reference processing argument
  • f3726a8: 8264020: Optimize double negation elimination
  • ... and 173 more: https://git.openjdk.java.net/jdk/compare/6aa28b3bdb78eb041a963d659d61e1622bc43ef8...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.

Loading

@openjdk openjdk bot added the ready label Mar 27, 2021
try (final OutputStream os = Files.newOutputStream(path)) {
os.write(0x02);
System.out.printf("Wrote a byte to %s%n", path);
try (InputStream is = Files.newInputStream(path);) {

This comment has been hidden.

Loading

@mlbridge
Copy link

@mlbridge mlbridge bot commented Mar 29, 2021

Mailing list message from Brian Burkhalter on nio-dev:

On Mar 27, 2021, at 8:57 AM, Alan Bateman <alanb at openjdk.java.net<mailto:alanb at openjdk.java.net>> wrote:

8263898: Fix accidentally reverted copyright year

The change looks okay.

The test looks okay although there other cases that probably should be added at some point, in particular, FileChannel.open for reading, and AsynchronousFileChannel read & write.

Added.

test/jdk/java/nio/file/Files/NulDevice.java line 41:

39: public class NulDevice {
40: public static void main(final String[] args) throws IOException {
41: System.getProperties().list(System.out);

Is the printing of the system properties a left-over?

Yes, it is vestigial; removed.

On Mar 28, 2021, at 8:24 AM, Alan Bateman <alanb at openjdk.java.net<mailto:alanb at openjdk.java.net>> wrote:

test/jdk/java/nio/file/Files/NulDevice.java line 47:

45: os.write(0x02);
46: System.out.printf("Wrote a byte to %s%n", path);
47: try (InputStream is = Files.newInputStream(path);) {

One other test that you could add here is to test that available() returns 0. I've no doubt there are many file operations that will behavior in surprising ways when using the special "NUL" device if the common operations work in unsurprising ways then it would help.

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

Loading

}

try (final FileChannel ch = FileChannel.open(path, CREATE,
TRUNCATE_EXISTING, WRITE)) {
Copy link
Contributor

@AlanBateman AlanBateman Mar 30, 2021

Choose a reason for hiding this comment

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

Did you mean to put a line break here? Same at L87. Asking because it would be easier to read if the open options were on the same line. If line length is the concern then you could drop the spurious "final" modifier.

Loading

Copy link
Member Author

@bplb bplb Mar 30, 2021

Choose a reason for hiding this comment

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

Fixed.

Loading

@bplb
Copy link
Member Author

@bplb bplb commented Mar 30, 2021

/integrate

Loading

@openjdk openjdk bot closed this Mar 30, 2021
@openjdk openjdk bot added the integrated label Mar 30, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Mar 30, 2021

@bplb Since your change was applied there have been 185 commits pushed to the master branch:

  • 2bd80f9: 8264326: Modernize javax.script.ScriptEngineManager and related classes' implementation
  • b08d638: 8262503: Support records in Dynalink
  • 21e7402: 8263707: C1 RangeCheckEliminator support constant array and NewMultiArray
  • 2ad6f2d: 8263896: Make not_suspended parameter from ObjectMonitor::exit() have default value
  • b652198: 8264429: Test runtime/cds/appcds/VerifyWithDefaultArchive.java assumes OpenJDK build
  • 2c9365d: 8264271: Avoid creating non_oop_word oops
  • daeca3f: 8262958: (fs) UnixUserDefinedFileAttributeView cleanup
  • af02883: 8264191: Javadoc search is broken in Internet Explorer
  • 6e74c3a: 8264193: Remove TRAPS parameters for modules and defaultmethods
  • ee5e00b: 8264279: Shenandoah: Missing handshake after JDK-8263427
  • ... and 175 more: https://git.openjdk.java.net/jdk/compare/6aa28b3bdb78eb041a963d659d61e1622bc43ef8...master

Your commit was automatically rebased without conflicts.

Pushed as commit 353807c.

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

Loading

@bplb bplb deleted the WindowsNullDevice-8263898 branch Mar 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants