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 >
