RFR JDK-8034802: (zipfs) newFileSystem throws UOE when the zip file is located in a custom file system

[Langer, Christoph]

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-bounces at openjdk.java.net> On Behalf Of Xueming
> Shen
> Sent: Sonntag, 16. September 2018 09:06
> To: core-libs-dev at openjdk.java.net; nio-dev <nio-dev at 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
>