RFR: 8303814: getLastErrorString should avoid charset conversions [v2]
Roger Riggs
rriggs at openjdk.org
Mon Mar 13 15:10:31 UTC 2023
On Mon, 13 Mar 2023 09:48:19 GMT, Daniel Jeliński <djelinski at openjdk.org> wrote:
>> This patch modifies the `getLastErrorString` method to return a `jstring`. Thanks to that we can avoid unnecessary back and forth conversions between Unicode and other charsets on Windows.
>>
>> Other changes include:
>> - the Windows implementation of `getLastErrorString` no longer checks `errno`. I verified all uses of the method and confirmed that `errno` is not used anywhere.
>> - While at it, I found and fixed a few calls to `JNU_ThrowIOExceptionWithLastError` that were done in context where `LastError` was not set.
>> - jdk.hotspot.agent was modified to use `JNU_ThrowByNameWithLastError` and `JNU_ThrowByName` instead of `getLastErrorString`; the code is expected to have identical behavior.
>> - zip_util was modified to return static messages instead of generated ones. The generated messages were not observed anywhere, because they were replaced by a static message in ZIP_Open, which is the only method used by other native code.
>> - `getLastErrorString` is no longer exported by libjava.
>>
>> Tier1-3 tests continue to pass.
>>
>> No new automated regression test; testing this requires installing a language pack that cannot be displayed in the current code page.
>> Tested this manually by installing Chinese language pack on English Windows 11, selecting Chinese language, then checking if the message on exception thrown by `InetAddress.getByName("nonexistent.local");` starts with `"不知道这样的主机。"` (or `"\u4e0d\u77e5\u9053\u8fd9\u6837\u7684\u4e3b\u673a\u3002"`). Without the change, the exception message started with a row of question marks.
>
> Daniel Jeliński has updated the pull request incrementally with three additional commits since the last revision:
>
> - Address review comments
> - Mention that the returned text is static and thread safe
> - Define buffer size
LGTM for some pre-existing code style inconsistencies.
src/java.base/share/native/libzip/zip_util.c line 812:
> 810:
> 811: if (strlen(name) >= PATH_MAX) {
> 812: if (pmsg) {
Some pre-existing references to pmsg still use implicit comparison, though not material to this PR, can you update them to compare with NULL.
-------------
Marked as reviewed by rriggs (Reviewer).
PR: https://git.openjdk.org/jdk/pull/12922
More information about the nio-dev
mailing list