On Fri, 16 May 2025 14:23:38 GMT, Jaikiran Pai <[email protected]> wrote:
>> David Beaumont has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Changes based on review feedback.
>
> src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java line 114:
>
>> 112: private final ZipPath rootdir;
>> 113: // Starts in readOnly (safe mode), but might be reset at the end of
>> initialization.
>> 114: private boolean readOnly = true;
>
> If `readOnly` gets used by some code when a `ZipFileSystem` instance is being
> constructed (which is why I believe this can't be a `final` field), then I
> think we should not change this value to `true`. In other words, would this
> change now have a chance of introducing a `ReadOnlyFileSystemException` when
> constructing the `ZipFileSystem` whereas before it wouldn't?
""would this change now have a chance of introducing a
ReadOnlyFileSystemException when constructing the ZipFileSystem whereas before
it wouldn't?""
If there was ever a "ReadOnlyFileSystemException" when initializing the
ZipFileSystem, we have a very serious bug already. Since we already have the
opening of Zip working for cases where the underlying file is read-only in the
file system, it has to be true that no attempt is made to modify the file
contents.
While I accept that this new code now calls out to functions with this flag in
a different state to before, I believe this is an absolute improvement since:
1. It is not acceptable to say to users "we support a read-only mode" if during
initialization we might modify the file in any way (even including changing
last-modification times etc.).
2. All the evidence points to whichever operations are being done during init
as being fundamentally "read-only" (both due to it working on read-only zip
files, and there being no conceptual need to modify anything during setup).
I'll happily do a thorough audit of everything which could be affected by this
change if that will give you confidence, but I would not want to change this
code back to its default "read-write until stated otherwise" behaviour.
> src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java line 245:
>
>> 243: : "The underlying ZIP file is not writable";
>> 244: throw new IOException(
>> 245: "A writable ZIP file system could not be opened
>> for: " + zfpath + "\n" + reason);
>
> The JDK coding guidelines recommend excluding file paths from exception
> messages. So in this case the `zfpath` should be left out from the exception
> message. Additionally, I haven't seen us using newline characters in
> exception messages that we construct in the JDK code. So I think we should
> leave that out too.
>
> Given this, I think it might be simpler to just change this `if` block to
> something like:
>
>
>
> if (...) {
> String reason = multiReleaseVersion.isPresent()
> ? "the multi-release JAR file is not writable"
> : "the ZIP file is not writable";
>
> throw new IOException(reason);
> }
Thanks for letting me know. Now I think about the "no filename" things is very
sensible for core libraries.
Done.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2095517691
PR Review Comment: https://git.openjdk.org/jdk/pull/25178#discussion_r2095520598
