Re: RFR: 8325590: Regression in round-tripping UTF-16 strings after JDK-8311906
On Mon, 12 Feb 2024 21:29:02 GMT, Roger Riggs wrote: > Correct the result string coder of a string encoded using a CharsetDecoder > with multi-byte encoded input. > Added tests for UTF16 strings and a regression test. test/jdk/java/nio/file/Files/ReadWriteString.java line 322: > 320: } > 321: assertEquals(actual, original, "Round trip string mismatch with > multi-byte encoding"); > 322: } The update to newStringNoRepl1 looks fine. The added test is very different to the tests in this source file. We really need to expand the test to exercise a lot more charsets and input cases. It's okay to have a targeted test for now but needs to be renamed to be consistent with the other tests. Also the other tests use testFiles as the file paths rather than putting files in /tmp. - PR Review Comment: https://git.openjdk.org/jdk/pull/17817#discussion_r1487326652
Re: RFR: 8325679: Optimize ArrayList subList sort
On Mon, 12 Feb 2024 22:52:51 GMT, Attila Szegedi wrote: > Somewhat surprisingly, `ArrayList$Sublist.sort()` is not specialized and will > thus fall back to slower default method of `List.sort()` instead of sorting a > range of the array in-place in its backing root `ArrayList`. > > This doesn't change observable behavior, so haven't added tests, and `tier1` > tests still all pass except for > `test/jdk/java/util/Locale/LocaleProvidersFormat.java` which also currently > fails on master too on the machine I tested on. @szegedi Oh interesting catch. Looks like a reasonable optimization. Is this covered by a test, or should a new test be added? @JimLaskey The `List.sort` method was added in JDK 8, so not really day one. Prior to that one had to call `Collections.sort` which copied to a temp array, sorted, then copied back; this applied equally to full lists and sublists. Since JDK 8, `ArrayList.sort` on the full list did sorting in place but sorting a subList just inherited the default method (which uses the old copy-to-temp-and-sort technique). My guess is that nobody noticed this because sublists aren't used that much, and sorting of subarrays, while arguably necessary for completeness, probably aren't used all that much either. - PR Comment: https://git.openjdk.org/jdk/pull/17818#issuecomment-1940384762
Re: RFR: 8325679: Optimize ArrayList subList sort
On Mon, 12 Feb 2024 22:52:51 GMT, Attila Szegedi wrote: > Somewhat surprisingly, `ArrayList$Sublist.sort()` is not specialized and will > thus fall back to slower default method of `List.sort()` instead of sorting a > range of the array in-place in its backing root `ArrayList`. > > This doesn't change observable behavior, so haven't added tests, and `tier1` > tests still all pass except for > `test/jdk/java/util/Locale/LocaleProvidersFormat.java` which also currently > fails on master too on the machine I tested on. Been there since day one? - PR Comment: https://git.openjdk.org/jdk/pull/17818#issuecomment-1939788165
RFR: 8325679: Optimize ArrayList subList sort
Somewhat surprisingly, `ArrayList$Sublist.sort()` is not specialized and will thus fall back to slower default method of `List.sort()` instead of sorting a range of the array in-place in its backing root `ArrayList`. This doesn't change observable behavior, so haven't added tests, and `tier1` tests still all pass except for `test/jdk/java/util/Locale/LocaleProvidersFormat.java` which also currently fails on master too on the machine I tested on. - Commit messages: - Add sort specialization for ArrayList Changes: https://git.openjdk.org/jdk/pull/17818/files Webrev: https://webrevs.openjdk.org/?repo=jdk=17818=00 Issue: https://bugs.openjdk.org/browse/JDK-8325679 Stats: 14 lines in 1 file changed: 12 ins; 1 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/17818.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17818/head:pull/17818 PR: https://git.openjdk.org/jdk/pull/17818
Re: RFR: 8303866: Allow ZipInputStream.readEnd to parse small Zip64 ZIP files [v16]
On Mon, 5 Feb 2024 13:14:39 GMT, Eirik Bjørsnøs wrote: >> ZipInputStream.readEnd currently assumes a Zip64 data descriptor if the >> number of compressed or uncompressed bytes read from the inflater is larger >> than the Zip64 magic value. >> >> While the ZIP format mandates that the data descriptor `SHOULD be stored in >> ZIP64 format (as 8 byte values) when a file's size exceeds 0x`, it >> also states that `ZIP64 format MAY be used regardless of the size of a >> file`. For such small entries, the above assumption does not hold. >> >> This PR augments ZipInputStream.readEnd to also assume 8-byte sizes if the >> ZipEntry includes a Zip64 extra information field AND at least one of the >> 'compressed size' and 'uncompressed size' have the expected Zip64 "magic" >> value 0x. This brings ZipInputStream into alignment with the APPNOTE >> format spec: >> >> >> When extracting, if the zip64 extended information extra >> field is present for the file the compressed and >> uncompressed sizes will be 8 byte values. >> >> >> While small Zip64 files with 8-byte data descriptors are not commonly found >> in the wild, it is possible to create one using the Info-ZIP command line >> `-fd` flag: >> >> `echo hello | zip -fd > hello.zip` >> >> The PR also adds a test verifying that such a small Zip64 file can be parsed >> by ZipInputStream. > > Eirik Bjørsnøs has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 230 commits: > > - Update readZipInputStream to verify that the ZipInputStream finds a single > zip entry with the expected contents > - Merge branch 'master' into data-descriptor > - Merge branch 'master' into data-descriptor > - Update comment of expect64BitDataDescriptor to reflect relaxed validation > - Dial down validation of the Zip64 extra field > - 8321712: C2: "failed: Multiple uses of register" in > C2_MacroAssembler::vminmax_fp > >Co-authored-by: Volodymyr Paprotski >Reviewed-by: kvn, thartmann, epeter, jbhateja > - 8319128: sun/security/pkcs11 tests fail on OL 7.9 aarch64 > >Reviewed-by: mbaesken > - 8322971: KEM.getInstance() should check if a 3rd-party security provider > is signed > >Reviewed-by: mullan, valeriep > - 8320890: [AIX] Find a better way to mimic dl handle equality > >Reviewed-by: stuefe, mdoerr > - 8323276: StressDirListings.java fails on AIX > >Reviewed-by: jpai, dfuchs > - ... and 220 more: https://git.openjdk.org/jdk/compare/692c9f88...e8d3b904 The latest updates seem OK. Thank you Eirik - Marked as reviewed by lancea (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/12524#pullrequestreview-1876488622
RFR: 8325590: Regression in round-tripping UTF-16 strings after JDK-8311906
Correct the result string coder of a string encoded using a CharsetDecoder with multi-byte encoded input. Added tests for UTF16 strings and a regression test. - Commit messages: - 8325590: Regression in round-tripping UTF-16 strings after JDK-8311906 Changes: https://git.openjdk.org/jdk/pull/17817/files Webrev: https://webrevs.openjdk.org/?repo=jdk=17817=00 Issue: https://bugs.openjdk.org/browse/JDK-8325590 Stats: 24 lines in 2 files changed: 18 ins; 1 del; 5 mod Patch: https://git.openjdk.org/jdk/pull/17817.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17817/head:pull/17817 PR: https://git.openjdk.org/jdk/pull/17817
Re: RFR: 8325558: Add jcheck whitespace checking for properties files
On Fri, 9 Feb 2024 13:35:55 GMT, Magnus Ihse Bursie wrote: > This is an attempt to finally implement the idea brought forward in > JDK-8295729: Properties files is essentially source code. It should have the > same whitespace checks as all other source code, so we don't get spurious > trailing whitespace changes or leading tabs instead of spaces. > > With Skara jcheck, it is possible to increase the coverage of the whitespace > checks. > > However, this turned out to be problematic, since trailing whitespace is > significant in properties files. That issue has mostly been sorted out in a > series of PRs, and this patch will finish the job with the few remaining > files, and actually enable the check in jcheck. java.xml changes look fine to me. - Marked as reviewed by joehw (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17789#pullrequestreview-1876181019
Re: RFR: 8325558: Add jcheck whitespace checking for properties files
On Fri, 9 Feb 2024 13:46:06 GMT, Magnus Ihse Bursie wrote: >> This is an attempt to finally implement the idea brought forward in >> JDK-8295729: Properties files is essentially source code. It should have >> the same whitespace checks as all other source code, so we don't get >> spurious trailing whitespace changes or leading tabs instead of spaces. >> >> With Skara jcheck, it is possible to increase the coverage of the whitespace >> checks. >> >> However, this turned out to be problematic, since trailing whitespace is >> significant in properties files. That issue has mostly been sorted out in a >> series of PRs, and this patch will finish the job with the few remaining >> files, and actually enable the check in jcheck. > > src/java.xml/share/classes/com/sun/org/apache/xerces/internal/impl/msg/XPointerMessages.properties > line 24: > >> 22: # Messages for message reporting >> 23: BadMessageKey = The error message corresponding to the message key can >> not be found. >> 24: FormatFailed = An internal error occurred while formatting the following >> message:\n > > Same here with `:\n`... It's done with the code (that is, a key is appended with the code). In fact, the whole Xerces stack was implemented this way. I'd leave it as is. - PR Review Comment: https://git.openjdk.org/jdk/pull/17789#discussion_r1486731927
Re: RFR: 8325558: Add jcheck whitespace checking for properties files
On Fri, 9 Feb 2024 13:35:55 GMT, Magnus Ihse Bursie wrote: > This is an attempt to finally implement the idea brought forward in > JDK-8295729: Properties files is essentially source code. It should have the > same whitespace checks as all other source code, so we don't get spurious > trailing whitespace changes or leading tabs instead of spaces. > > With Skara jcheck, it is possible to increase the coverage of the whitespace > checks. > > However, this turned out to be problematic, since trailing whitespace is > significant in properties files. That issue has mostly been sorted out in a > series of PRs, and this patch will finish the job with the few remaining > files, and actually enable the check in jcheck. Approving the sun.net changes. - Marked as reviewed by dfuchs (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17789#pullrequestreview-1875818676
Re: RFR: 8325558: Add jcheck whitespace checking for properties files
On Fri, 9 Feb 2024 13:35:55 GMT, Magnus Ihse Bursie wrote: > This is an attempt to finally implement the idea brought forward in > JDK-8295729: Properties files is essentially source code. It should have the > same whitespace checks as all other source code, so we don't get spurious > trailing whitespace changes or leading tabs instead of spaces. > > With Skara jcheck, it is possible to increase the coverage of the whitespace > checks. > > However, this turned out to be problematic, since trailing whitespace is > significant in properties files. That issue has mostly been sorted out in a > series of PRs, and this patch will finish the job with the few remaining > files, and actually enable the check in jcheck. > @naotoj Thanks! Would you care to also submit a review? My bad. I thought I approved this PR. - Marked as reviewed by naoto (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17789#pullrequestreview-1875811619
Re: RFR: 8311864: Add ArraysSupport.hashCode(int[] a, fromIndex, length, initialValue) [v4]
On Tue, 2 Jan 2024 14:37:16 GMT, Pavel Rappo wrote: >> This PR adds an internal method to calculate hash code from the provided >> integer array, offset and length into that array, and the initial hash code >> value. >> >> Current options for calculating a hash code for int[] are inflexible. It's >> either ArraysSupport.vectorizedHashCode with an offset, length and initial >> value, or Arrays.hashCode with just an array. >> >> For an arbitrary int[], unconditional vectorization might be unwarranted or >> puzzling. Unfortunately, it's the only hash code method that operates on an >> array subrange or accepts the initial hash code value. >> >> A more convenient method could be added and then used, for example, here: >> >> * >> https://github.com/openjdk/jdk/blob/0ef03f122866f010ebf50683097e9b92e41cdaad/src/java.base/share/classes/java/util/concurrent/CopyOnWriteArrayList.java#L1076-L1083 >> >> * >> https://github.com/openjdk/jdk/blob/0ef03f122866f010ebf50683097e9b92e41cdaad/src/java.base/share/classes/java/util/ArrayList.java#L669-L680 >> >> * >> https://github.com/openjdk/jdk/blob/0ef03f122866f010ebf50683097e9b92e41cdaad/src/java.base/share/classes/sun/security/pkcs10/PKCS10.java#L356-L362 >> >> This PR adds such a method and provides tests for it. Additionally, this PR >> adds tests for `null` passed to `java.util.Arrays.hashCode` overloads, >> behaviour which was specified but untested. > > Pavel Rappo 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 branch 'master' into 8311864 > - Merge remote-tracking branch 'jdk/master' into 8311864 > - Merge branch 'master' into 8311864 > - Initial commit To Skara bots: keep this PR alive. - PR Comment: https://git.openjdk.org/jdk/pull/14831#issuecomment-1939184314
Re: RFR: 8325579: Inconsistent behavior in com.sun.jndi.ldap.Connection::createSocket
On Fri, 9 Feb 2024 21:29:28 GMT, Christoph Langer wrote: > During analysing a customer case I figured out that we have an inconsistency > between documentation and actual behavior in class > com.sun.jndi.ldap.Connection. The [method documentation of > com.sun.jndi.ldap.Connection::createSocket](https://github.com/openjdk/jdk/blob/3ebe6c192a5dd5cc46ae2d263713c9ff38cd46bb/src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java#L281) > states: "If a timeout is supplied but unconnected sockets are not supported > then the timeout is ignored and a connected socket is created." > > This, however does not happen. If a SocketFactory would not support > unconnected sockets, it would likely throw a SocketException in > [SocketFactory::createSocket()](https://github.com/openjdk/jdk/blob/6303c0e7136436a2d3cb6043b88edf788c0067cc/src/java.base/share/classes/javax/net/SocketFactory.java#L123). > And since [the > code](https://github.com/openjdk/jdk/blob/3ebe6c192a5dd5cc46ae2d263713c9ff38cd46bb/src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java#L336) > does not check for this behavior, a connection with timeout value through a > SocketFactory that does not support unconnected sockets would simply fail > with an IOException. > > So we should either make the code adhere to what is documented or adapt the > documentation to the actual behavior. > > I hereby try to fix the connect coding. Alternatively, we could also adapt > the description - I have no strong opinion. What do the experts suggest? Hi Christoph, I think the proposed change is good, and it solves the problem we've also seen before with custom socket factories specified in the `"java.naming.ldap.factory.socket"` JNDI environment property not implementing `javax.net.SocketFactory::createSocket()` method - custom implementations are not required to implement this method, hence `SocketException` can be thrown by the default implementation. The change proposed by you should help to address such scenarios. It would also be great to update the `com.sun.jndi.ldap.connect.timeout` env property documentation in the `java.naming` module-info with the code comment mentioned above. To fully clarify the `"unconnected sockets are not supported"` statement the `"java.naming.ldap.factory.socket"` environment property might need to have documentation added. I've launched JNDI/LDAP regression tests with your patch and no failures were observed. As a good addition to the proposed fix, it would be great to have a test for scenarios when a custom socket factory does/doesn't override the `createSocket` method. There are a few test examples that can be used as a bootstrap - for example, `test/jdk/com/sun/jndi/ldap/DeadSSLLdapTimeoutTest.java`. - PR Comment: https://git.openjdk.org/jdk/pull/17797#issuecomment-1939169699
RFR: 8325576: TEST_BUG: java/lang/ProcessHandle/InfoTest.java#test3 fails on systems with coreutils with --enable-single-binary
Ran the test on AmazonLinux 2 which has multiple binaries from coreutils package and no coreutils executable as well as AmazonLinux 2023 that uses `--enable-single-binary` - Commit messages: - TEST_BUG: java/lang/ProcessHandle/InfoTest.java#test3 fails on systems with coreutils with --enable-single-binary Changes: https://git.openjdk.org/jdk/pull/17798/files Webrev: https://webrevs.openjdk.org/?repo=jdk=17798=00 Issue: https://bugs.openjdk.org/browse/JDK-8325576 Stats: 30 lines in 2 files changed: 29 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/17798.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17798/head:pull/17798 PR: https://git.openjdk.org/jdk/pull/17798
Re: The common ForkJoinPool does not have any ForkJoinWorkerThread while tasks are submitted to the queue
Hi,
Could the problem have occurred because the ForkJoinPool got an OOME when it
tried to allocate a ForkJoinWorkerThread?
To check for that, if you're using the commonPool(), you might be able to add a
custom ForkJoinWorkerThreadFactory via passing in
-Djava.util.concurrent.ForkJoinPool.common.threadFactory= and implement newThread() such that you try-catch OOME and log it
from there.
Cheers,
√
Viktor Klang
Software Architect, Java Platform Group
Oracle
From: core-libs-dev on behalf of Xiao Yu
Sent: Saturday, 3 February 2024 19:54
To: Jaikiran Pai
Cc: core-libs-dev@openjdk.org
Subject: Re: The common ForkJoinPool does not have any ForkJoinWorkerThread
while tasks are submitted to the queue
Hi Jaikiran,
Thanks a lot for replying.
Our application is a client that communicates to the server for
request/response. The client creates a secure (TLS) connection to the server,
that is, on top of the SocketChannel, we implement a Wrapper class called
SSLDataChannel for reading and writing. The SSLDataChannel uses the
javax.net.ssl.SSLEngine. Before any read and write can happen, we need to do
SSL handshakes by calling methods in SSLEngine. One of the methods is
SSLEngine#getDelegatedTask(). The returned task needs to be executed before the
handshake can proceed. After the task is done, we need to continue processing
read and write events on the connection. The connection read and write events
are all handled by a class called NioEndpointHandler. One requirement for our
client is that it supports an asynchronous API and therefore the whole stack
must all implement non-blocking methods. The tasks from the SSLEngine could
take a long time and we do not want them to block our other connection events,
and this is when the ForkJoinPool is used. We run the SSL tasks in the
ForkJoinPool and after the task is done we arrange to run the
NioEndpointHandler callbacks to proceed with the read and write events. The
much simplified code looks somewhat like the following.
```
class NioEndpointHandler {
/** The ssl channel */
private final SSLDataChannel sslDataChannel;
/** The runnable to execute to handle read after ssl tasks is done. */
private final Runnable handleReadAfterSSLTask = () -> onRead();
/** The handler state. */
State state;
/** Executes the SSL tasks until no task to run, then run the callback. */
private void executeSSLTask(ExecutorService executor, Runnable callback) {
executor.submit(() -> {
Runnable task;
while ((task = sslDataChannel.getSSLEngine().getDelegatedTask()) !=
null) {
task.run();
}
try {
callback.run();
} catch (Throwable t) {
/* logging the exception. */
}
});
}
/** Handle a read event. */
private void onRead() {
if (sslDataChannel.needsHandshake()) {
/* do handshake */
/* One of the handshake step is to check if there is any SSL task
to run. */
if (sslDataChannel.needExecuteTask()) {
executeSSLTask(ForkJoinPool.commonPool(),
handleReadAfterSSLTask);
}
}
}
private void terminate() {
state = TERMINATED;
/* Other clean up tasks, however, tasks submitted to the ForkJoinPool
are not cancelled. */
}
}
```
> What are these handlers? Are they classes which implement Runnable or
> are they something else? What does termination of handler mean in this
> context? Do you use any java.util.concurrent.* APIs to "cancel" such
> terminated handlers?
The much simplified handler code please see above.
The tasks submitted to the ForkJoinPool queue are Runnables that are fields to
the NioEndpointHandler. What we have observed is that there are a lot of tasks
in the fork join pool that have a reference to the lambda inside
NioEndpointHandler#executeSSLTask which eventually have a reference to the
NioEndpointHandler. Those NioEndpointHandler are in the TERMINATED state. The
only reference to those NioEndpointHandler are these tasks or otherwise they
can be garbage collected after the termination cleans up all the other
references.
Termination of the handler means those connections are at the end of their life
cycle. We clean up things such as signal end of life cycle for all the
associated request/response pairs and closing the SSLDataChannel, etc.
No, we have not use the cancel method to cancel the submitted tasks. I agree
that this is an oversight and it would be cleaner to cancel them. However, my
current theory is that this is not the root cause. From my understanding of the
code, the cancel method only changes the state of the task. It does not remove
the task from the queue of the ForkJoinPool. Therefore, those tasks, even if
got cancelled, would still stay in the queue preventing the terminated
NioEndpointHandler from being garbage collected. Currently, I am
Re: RFR: JDK-8324930: java/lang/StringBuilder problem with concurrent jtreg runs
On Tue, 30 Jan 2024 09:08:28 GMT, Matthias Baesken wrote: > On some Windows machines we see sometimes OOM errors because of high resource > (memory/swap) consumption. This is especially seen when the jtreg runs have > higher concurrency. A solution is to put the java/lang/StringBuilder tests in > the exclusiveAccess.dirs group so that they are not executed concurrently, > which helps to mitigate the resource shortages. > Of course this has the downside that on very large machines the concurrent > execution is not done any more. Hello Matthias, looking at the crash log you pasted, it's clear that the test itself isn't a culprit here. Specifically, the failure appears to be when a JVM launch is being attempted for the `test/jdk/java/lang/StringBuilder/Insert.java` test (which looking at the code doesn't use too much memory once launched). What seems to be happening is that the system where this run appears to be launching too many tests concurrently. The exact command used to launch these tests on that setup would be helpful in understanding the configurations. The JDK build by default "computes" the `TEST_JOBS` value which controls this concurrency (the number of jtreg concurrent tests to run) and that's done here https://github.com/openjdk/jdk/blob/master/make/RunTests.gmk#L151 and as noted in testing.md, it is configurable (and has a per system default) https://github.com/openjdk/jdk/blob/master/doc/testing.md#jobs-1. This configuration ultimately translates to the `-concurrency` option of jtreg which is explained in section `3.8 How do I specify whether to run tests concurrently?` and `3.25 My system is unusable while I run tests. How do I fix that?` of the jtreg FAQ https://openjdk.org/jtreg/faq.html. Based on the available details so far, it appears that you might have to reduce the value for this concurrency option, through the right build/test option. - PR Comment: https://git.openjdk.org/jdk/pull/17625#issuecomment-1938437285
Re: RFR: 8303891: Speed up Zip64SizeTest using a small ZIP64 file [v8]
On Fri, 9 Feb 2024 12:31:27 GMT, Eirik Bjørsnøs wrote: >> Please review this PR which suggests we speed up the `Zip64SizeTest` using a >> small-sized ZIP64 ZIP file specifically created to reproduce the issue being >> tested. >> >> The disk space requirement of this test is known to cause problems in some >> builds, see [JDK-8259866](https://bugs.openjdk.org/browse/JDK-8259866) >> >> By using a sparse file, we reduce consumed disk space from 5GB to 266 bytes >> and also reduce the runtime from ~35 seconds to ~1 seconds on my Macbook Pro. >> >> The PR also fixes the `@summary` tag, which seems to have been copied from >> an unrelated test. > > Eirik Bjørsnøs has updated the pull request incrementally with two additional > commits since the last revision: > > - Use three dots consistently for representing an ellipsis > - Add zipdetails of the Zip64 extra field in updateCENHeaderToZip64 These test-only changes look good to me. - Marked as reviewed by jpai (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/12948#pullrequestreview-1874939320
Re: RFR: 8303891: Speed up Zip64SizeTest using a small ZIP64 file [v5]
On Fri, 9 Feb 2024 10:41:18 GMT, Eirik Bjørsnøs wrote:
>> test/jdk/java/util/zip/ZipFile/Zip64SizeTest.java line 112:
>>
>>> 110: ZipEntry e1 = new ZipEntry("first");
>>> 111: // Make room for an 8-byte ZIP64 extra field
>>> 112: e1.setExtra(createOpaqueExtra((short) Long.BYTES));
>>
>> Hello Eirik, I couldn't understand why we first add a opaque extra field
>> first and then update it to be a zip64 extra field. Why do we do this?
>
>> Hello Eirik, I couldn't understand why we first add a opaque extra field
>> first and then update it to be a zip64 extra field. Why do we do this?
>
> `ZipEntry.setExtra` processes the byte array argument, looking for Zip64
> extended fields which it can extract the size fields from. To prevent this
> parsing from happening, we temporarily use the `unknown` tag.
>
> In this particular case, `ZipExtra.setExtra` actually ends up skipping this
> processing (because `isLOC == true` and it has a guard for the block size
> being `>= 16`).
>
> However, I prefer the test to not depend too much on the details of
> `setExtra` Zip64 processing. This trick is used in other tests as well and
> may be copied over to a test where the conditions are not the same.
>
> I have refactored a bit and added some code comments to help explain the use
> of the 'unknown' tag.
>
> Do you think this makes sense?
Hello Eirik, thank you for that detail. Yes, what you note and the updated
comment, looks good to me.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/12948#discussion_r1485992196
Re: RFR: 8324539: Do not use LFS64 symbols in JDK libs [v10]
On Thu, 8 Feb 2024 14:47:26 GMT, Joachim Kern wrote: >> And also `#define statvfs statvfs64` is not necessary with the same >> explanation as for the `opendir` defines above -- sorry again. >> The very only difference between statvfs and statvfs64 is that the >> filesystem ID field in statvfs has a width of 8 Bytes, while in statvfs64 it >> has a width of 16 Bytes. > >> @JoKern65 So what about `#define fstatvfs fstatvfs64`? Is that still needed? >> If so, I could not be bothered to make another change. Otherwise, we can get >> rid of _all_ AIX 64-bit redefines, and then I'll (probably) do it. > > Same as `statvfs`. Only the file system ID field is smaller. > @JoKern65 @MBaesken I did not receive any reply about what to do with > `fstatvfs`, so I decided to keep the last verified change for AIX. If you > want to clean this up, then please do so, but at that time it will be an > AIX-only patch. @magicus I have to reach out to IBM asking if the different size of the 'filesystem ID' field in statvfs makes an important difference. If not, I will remove the defines in an AIX-only patch. Thanks a lot for your effort. - PR Comment: https://git.openjdk.org/jdk/pull/17538#issuecomment-1938300228
Re: RFR: 8325558: Add jcheck whitespace checking for properties files
On Fri, 9 Feb 2024 18:09:11 GMT, Naoto Sato wrote: >> This is an attempt to finally implement the idea brought forward in >> JDK-8295729: Properties files is essentially source code. It should have >> the same whitespace checks as all other source code, so we don't get >> spurious trailing whitespace changes or leading tabs instead of spaces. >> >> With Skara jcheck, it is possible to increase the coverage of the whitespace >> checks. >> >> However, this turned out to be problematic, since trailing whitespace is >> significant in properties files. That issue has mostly been sorted out in a >> series of PRs, and this patch will finish the job with the few remaining >> files, and actually enable the check in jcheck. > > Skimmed through the changes and all look good to me. Good to have `jcheck` > detect those unneeded trailing spaces. @naotoj Thanks! Would you care to also submit a review? - PR Comment: https://git.openjdk.org/jdk/pull/17789#issuecomment-1938204446
Re: RFR: 8324539: Do not use LFS64 symbols in JDK libs [v11]
> Similar to [JDK-8318696](https://bugs.openjdk.org/browse/JDK-8318696), we > should use -D_FILE_OFFSET_BITS=64, and not -D_LARGEFILE64_SOURCE in the JDK > native libraries. Magnus Ihse Bursie has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 15 commits: - Merge branch 'master' into jdk-FOB64 - Fix indentation - Once more, remove AIX dirent64 et al defines - Also fix fstatvfs on AIX - Redefine statvfs as statvfs64 on AIX - Add kludge to BufferedRenderPipe.c for AIX - Merge branch 'master' into jdk-FOB64 - Remove all system includes from debug_util.h - Merge branch 'master' into jdk-FOB64 - Move #include out of debug_util.h - ... and 5 more: https://git.openjdk.org/jdk/compare/efa071dd...caccbf9b - Changes: https://git.openjdk.org/jdk/pull/17538/files Webrev: https://webrevs.openjdk.org/?repo=jdk=17538=10 Stats: 284 lines in 29 files changed: 23 ins; 144 del; 117 mod Patch: https://git.openjdk.org/jdk/pull/17538.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17538/head:pull/17538 PR: https://git.openjdk.org/jdk/pull/17538
Integrated: 8324539: Do not use LFS64 symbols in JDK libs
On Tue, 23 Jan 2024 15:42:55 GMT, Magnus Ihse Bursie wrote: > Similar to [JDK-8318696](https://bugs.openjdk.org/browse/JDK-8318696), we > should use -D_FILE_OFFSET_BITS=64, and not -D_LARGEFILE64_SOURCE in the JDK > native libraries. This pull request has now been integrated. Changeset: e5cb78cc Author:Magnus Ihse Bursie URL: https://git.openjdk.org/jdk/commit/e5cb78cc88761cd27964e9fe77fc9c6f9073e888 Stats: 310 lines in 29 files changed: 23 ins; 144 del; 143 mod 8324539: Do not use LFS64 symbols in JDK libs Reviewed-by: jwaters, erikj, mbaesken, alanb - PR: https://git.openjdk.org/jdk/pull/17538
Re: RFR: 8324539: Do not use LFS64 symbols in JDK libs [v11]
On Mon, 12 Feb 2024 08:01:23 GMT, Magnus Ihse Bursie wrote: >> Similar to [JDK-8318696](https://bugs.openjdk.org/browse/JDK-8318696), we >> should use -D_FILE_OFFSET_BITS=64, and not -D_LARGEFILE64_SOURCE in the JDK >> native libraries. > > Magnus Ihse Bursie has updated the pull request with a new target base due to > a merge or a rebase. The pull request now contains 15 commits: > > - Merge branch 'master' into jdk-FOB64 > - Fix indentation > - Once more, remove AIX dirent64 et al defines > - Also fix fstatvfs on AIX > - Redefine statvfs as statvfs64 on AIX > - Add kludge to BufferedRenderPipe.c for AIX > - Merge branch 'master' into jdk-FOB64 > - Remove all system includes from debug_util.h > - Merge branch 'master' into jdk-FOB64 > - Move #include out of debug_util.h > - ... and 5 more: https://git.openjdk.org/jdk/compare/efa071dd...caccbf9b Thank you again for this! - PR Comment: https://git.openjdk.org/jdk/pull/17538#issuecomment-1938202537
Re: RFR: 8324539: Do not use LFS64 symbols in JDK libs [v10]
On Thu, 8 Feb 2024 14:47:26 GMT, Joachim Kern wrote: >> And also `#define statvfs statvfs64` is not necessary with the same >> explanation as for the `opendir` defines above -- sorry again. >> The very only difference between statvfs and statvfs64 is that the >> filesystem ID field in statvfs has a width of 8 Bytes, while in statvfs64 it >> has a width of 16 Bytes. > >> @JoKern65 So what about `#define fstatvfs fstatvfs64`? Is that still needed? >> If so, I could not be bothered to make another change. Otherwise, we can get >> rid of _all_ AIX 64-bit redefines, and then I'll (probably) do it. > > Same as `statvfs`. Only the file system ID field is smaller. @JoKern65 @MBaesken I did not receive any reply about what to do with `fstatvfs`, so I decided to keep the last verified change for AIX. If you want to clean this up, then please do so, but at that time it will be an AIX-only patch. - PR Comment: https://git.openjdk.org/jdk/pull/17538#issuecomment-1938201250
Re: RFR: 8324539: Do not use LFS64 symbols in JDK libs [v12]
> Similar to [JDK-8318696](https://bugs.openjdk.org/browse/JDK-8318696), we > should use -D_FILE_OFFSET_BITS=64, and not -D_LARGEFILE64_SOURCE in the JDK > native libraries. Magnus Ihse Bursie has updated the pull request incrementally with one additional commit since the last revision: Update copyright year - Changes: - all: https://git.openjdk.org/jdk/pull/17538/files - new: https://git.openjdk.org/jdk/pull/17538/files/caccbf9b..7f673ef6 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17538=11 - incr: https://webrevs.openjdk.org/?repo=jdk=17538=10-11 Stats: 26 lines in 26 files changed: 0 ins; 0 del; 26 mod Patch: https://git.openjdk.org/jdk/pull/17538.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17538/head:pull/17538 PR: https://git.openjdk.org/jdk/pull/17538
