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

[Xueming Shen]

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