On Tue, 29 Sep 2026 14:38:03 GMT, Alan Bateman <[email protected]> wrote:
>> Jaikiran Pai has updated the pull request with a new target base due to a
>> merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase. The pull request contains four additional
>> commits since the last revision:
>>
>> - merge latest from master branch
>> - remove incorrect comment about localization of error messages
>> - update test
>> - 8392966: Improve error message for executable JAR file
>
> src/java.base/share/native/libjli/emessages.h line 66:
>
>> 64: #define JAR_ERROR_CANNOT_OPEN "Error: Unable to open JAR file %s"
>> 65: #define JAR_ERROR_MANIFEST_MISSING "Error: No manifest in JAR file %s"
>> 66: #define JAR_ERROR_MANIFEST_PARSE "Error: Unable to parse the manifest
>> of JAR file %s"
>
> Although these names are easier to read than the old names, it means a
> different convention to all the other errors defined in emessages.h. It's not
> bad, just looks different.
I don't have a strong preference, so if you suggest, then for consistency I can
switch those macro names to the older ones.
> src/java.base/share/native/libjli/java.c line 1473:
>
>> 1471: JLI_ReportErrorMessage(JAR_ERROR_MANIFEST_MISSING,
>> jar_path);
>> 1472: } else {
>> 1473: JLI_ReportErrorMessage(JAR_ERROR_MANIFEST_PARSE, jar_path);
>
> This is "Unable to parse the manifest of JAR file" so I assume is the error
> when a corrupt JAR file is opened. The existing code use JAR_ERROR3 had
> "Invalid or corrupt .." and I'm wondering if the replacement message needs to
> retain some of this because -3 is both the corrupt JAR file and no manifest
> case now.
I've adjusted the message to consider this detail. I can adjust it further if
necessary. Just one clarification:
> because -3 is both the corrupt JAR file and no manifest case now.
The case of "no manifest" is -2. The case of corrupt JAR or failure to inflate
a manifest (that exists) due to some error is -3.
> src/java.base/share/native/libjli/parse_manifest.c line 589:
>
>> 587: if ((rc = find_file(fd, &entry, manifest_name)) != 0) {
>> 588: close(fd);
>> 589: return rc == -2 ? rc : -3; // -2 if manifest file not found, -3
>> for other errors
>
> This would be easier to read/understand if you change to:
>
> if (rc == -2) {
> return -2; // manfiest entry not found
> }
> return -3; // other error
Done, I've updated the PR to use this style.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/33064#discussion_r4135355463
PR Review Comment: https://git.openjdk.org/jdk/pull/33064#discussion_r4135337533
PR Review Comment: https://git.openjdk.org/jdk/pull/33064#discussion_r4135314874
