On Mon, 19 May 2025 12:39:22 GMT, Jaikiran Pai <[email protected]> wrote:
>> David Beaumont has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Fixed test.
>
> src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java line 210:
>
>> 208: }
>> 209: }
>> 210: // sm and existence check
>
> Nit - this is pre-existing, but would be good to cleanup. The "sm" here
> refers to SecurityManager. In JDK mainline, the SecurityManager is no longer
> present. So it would be good to change this comment to just "existence check"
> and remove reference to "sm".
Done.
> src/jdk.zipfs/share/classes/module-info.java line 290:
>
>> 288: * already exist, and is incompatible with {@code
>> "create"=true}.
>> 289: * Specifying both will cause an {@code
>> IllegalArgumentException}
>> 290: * to be thrown when the Zip filesystem is created
>
> To be precise, perhaps this last sentence should be reworded to:
>
>> Specifying {@code create} as {@code true} and {@code accessMode} as {@code
>> readOnly} will cause an {@code IllegalArgumentException} to be thrown when
>> the ZIP filesystem is created.
Done.
> src/jdk.zipfs/share/classes/module-info.java line 304:
>
>> 302: * </li>
>> 303: * </ul>
>> 304: * The access mode has no effect on reported POSIX file
>> permissions (in cases
>
> For consistency, I think this should be:
>
>> The {@code accessMode} has no ....
Done.
> test/jdk/jdk/nio/zipfs/NewFileSystemTests.java line 208:
>
>> 206: // Multi-release JARs, when opened with a specified version are
>> inherently read-only.
>> 207: Path multiReleaseJar = createMultiReleaseJar();
>> 208: try (FileSystem fs = FileSystems.newFileSystem(multiReleaseJar,
>> Map.of("accessMode", "readWrite"))) {
>
> I wonder if the ZIP filesystem implementation should throw an exception in
> this case. In the case where the application code has explicitly stated
> `accessMode=readWrite`, if the underlying file `Path` isn't writable, then we
> currently throw an `IOException`. I think we should specify and implement the
> same for the case where `accessMode=readWrite` and the file is a
> multi-release JAR file.
It does throw if accessMode=readWrite and the result is not writable for any
reason.
this.readOnly = forceReadOnly || multiReleaseVersion.isPresent() ||
!Files.isWritable(zfpath);
if (readOnly && accessMode == ZipAccessMode.READ_WRITE) {
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2095718428
PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2095714913
PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2095715684
PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2095723172
