Hi Sherman,
just one comment, the copyright dates in ZipFileSystem.java,
ZipFileSystemProvider.java and ZipFSTester.java are not updated.
-Felix
________________________________
发件人: core-libs-dev <core-libs-dev-boun...@openjdk.java.net> 代表 Xueming Shen
<xueming.s...@oracle.com>
发送时间: 2018年9月17日 17:51:05
收件人: Langer, Christoph
抄送: nio-dev; core-libs-dev@openjdk.java.net
主题: Re: RFR JDK-8034802: (zipfs) newFileSystem throws UOE when the zip file is
located in a custom file system
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
>>
