Thanks!
webrev has been updated accordingly.
http://cr.openjdk.java.net/~sherman/8034802/webrev/
-Sherman
On 09/17/2018 07:30 AM, Langer, Christoph wrote:
Hi Sherman,
as I'm currently looking into zip stuff as well, it's a good exercise to review
your changeset. Overall it looks good. I found a few nits mostly in the area of
spelling and whitespace 😊
src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java:
88 ZipFileSystem(ZipFileSystemProvider provider,
I think, the constructor should initialize these items as last statements:
92 this.provider = provider;
93 this.zfpath = zfpath;
As per section 7-3 of the security guide which should probably apply here:
https://www.oracle.com/technetwork/java/seccodeguide-139067.html#7
132 // returns ture if there is a name=true/"true" setting in env
-> spelling: // returns true if there is a name=true/"true" setting in env
282 // and sync dose not close the ch
-> spelling: // and sync did not close the ch
587 // (1) writing the contents of a new entry, if the entry doesn't exits,
or
-> spelling: // (1) writing the contents of a new entry, if the entry doesn't
exist, or
1471 }
1472 else { // untouced CEN or COPY
-> put brace and else on the same line to match style above and spelling of
"untouched": } else { // untouched CEN or COPY
remove these 2 lines that have been commented out:
1881 // Entry(byte[] name, boolean isdir) {
1889 // this.method = METHOD_DEFLTED;
2402 fm.format(" name : %s%n", new String(name));
2403 fm.format(" creationTime : %tc%n",
creationTime().toMillis());
-> I would align name with the start of creationTime (and the other lines
below)
test/jdk/jdk/nio/zipfs/ZipFSTester.java:
134 try ( OutputStream os = Files.newOutputStream(src)) {
-> the blank between ( and OutputStream could be removed
436 private static void checkRead(Path path, byte[] expected) throws
IOException
-> should take brace from line 437 on the same line, line length is not too
long yet
438 //streams
-> insert a blank between // and streams
447 try (SeekableByteChannel sbc = Files.newByteChannel(path);
insert one more space before try to have correct indentation
src/jdk.zipfs/share/classes/jdk/nio/zipfs/ByteArrayChannel.java
43 * The currently position of this channel.
-> The current position of this channel.
97 throw new IllegalArgumentException("negative position " +
pos);
-> shouldn't it rather be "illegal position " as it must not necessarily be a
negative position?
Assuming that the updated test case runs correctly, you can consider the
changes reviewed from my end.
I'll post a change for adding executable bit support soon, based on your
changes.
Best regards
Christoph
-----Original Message-----
From: nio-dev<nio-dev-boun...@openjdk.java.net> On Behalf Of Xueming
Shen
Sent: Sonntag, 16. September 2018 09:06
To: core-libs-dev@openjdk.java.net; nio-dev<nio-...@openjdk.java.net>
Subject: RFR JDK-8034802: (zipfs) newFileSystem throws UOE when the zip
file is located in a custom file system
Hi
Please help review the change for JDK-8034802.
issue: https://bugs.openjdk.java.net/browse/JDK-8034802
webrev: http://cr.openjdk.java.net/~sherman/8034802/webrev
One of the reasons that the implementation works this way is that I
didn't have a
complete SeekableByteChannel support (mainly the position(...)) from
newByteChannel()
back then (so you really can't get a ZipFileSystem from a jar/zip file
inside a zfs :-))
So this changeset also includes a ByteArrayChannel which implements SBC
and has
better position()/position(int) support, and now we can have a zipfs
from a zip file
inside a zipfs.
Thanks,
Sherman
