RFR: 8251329: (zipfs) Files.walkFileTree walks infinitely if zip has dir named "." inside
Hi, As discussed in the https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-July/079621.html thread, this is the revised patch to address the use of '.' and '..' within Zip FS Zip FS needs to use "." and ".." as links to the current and parent directories and cannot reliably support entries that have "." and ".." as name elements. This patch updates Zip Fs to reject ZIP files that have entries in the CEN that can't be used for files in a file system. Mach5 tiers 1 through 3 have been run without any errors encountered . Best, Lance - Commit messages: - Address missing linefeed after package name - Address overzelous intellij import update - Patch to address JDK-8251329 Changes: https://git.openjdk.java.net/jdk/pull/4900/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4900=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8251329 Stats: 174 lines in 2 files changed: 174 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/4900.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4900/head:pull/4900 PR: https://git.openjdk.java.net/jdk/pull/4900
Re: RFR: 8199594: Add doc describing how (?x) ignores spaces in character classes [v3]
On Thu, 22 Jul 2021 01:46:09 GMT, Ian Graves wrote: >> 8199594: Add doc describing how (?x) ignores spaces in character classes > > Ian Graves has updated the pull request incrementally with one additional > commit since the last revision: > > Rewording repetitive phrase Marked as reviewed by lancea (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4618
Re: RFR: 8190753: (zipfs): Accessing a large entry (> 2^31 bytes) leads to a negative initial size for ByteArrayOutputStream [v11]
On Fri, 23 Jul 2021 15:23:07 GMT, Jaikiran Pai wrote: > Thank you for the review Alan. > > @LanceAndersen, I've run the tier1 tests locally with the latest PR and they > have passed without any regressions. Given that we changed the implementation > to wrap ByteArrayOutputStream instead of extending it, would you want to > rerun some of your other tests that you had previously run, before I > integrate this? Yes, I will run additional tests and report back after they complete - PR: https://git.openjdk.java.net/jdk/pull/4607
Re: RFR: 8199594: Add doc describing how (?x) ignores spaces in character classes [v2]
On Wed, 21 Jul 2021 21:25:18 GMT, Ian Graves wrote: >> 8199594: Add doc describing how (?x) ignores spaces in character classes > > Ian Graves has updated the pull request incrementally with one additional > commit since the last revision: > > delete errant line src/java.base/share/classes/java/util/regex/Pattern.java line 758: > 756: *group just as in Perl. > 757: * > 758: * In Perl, free-spacing mode (which is called > comments Looks good overall, one suggestion is to perhaps re-phrase the opening of the paragraph as the preceding paragraph also starts with "In-Perl". - PR: https://git.openjdk.java.net/jdk/pull/4618
Re: RFR: 8190753: (zipfs): Accessing a large entry (> 2^31 bytes) leads to a negative initial size for ByteArrayOutputStream [v8]
On Mon, 5 Jul 2021 07:42:26 GMT, Jaikiran Pai wrote: >> Can I please get a review for this proposed fix for the issue reported in >> https://bugs.openjdk.java.net/browse/JDK-8190753? >> >> The commit here checks for the size of the zip entry before trying to create >> a `ByteArrayOutputStream` for that entry's content. A new jtreg test has >> been included in this commit to reproduce the issue and verify the fix. >> >> P.S: It's still a bit arguable whether it's a good idea to create the >> `ByteArrayOutputStream` with an initial size potentially as large as the >> `MAX_ARRAY_SIZE` or whether it's better to just use some smaller default >> value. However, I think that can be addressed separately while dealing with >> https://bugs.openjdk.java.net/browse/JDK-8011146 > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > reorganize the tests now that the temp file creation threshold isn't > exposed as a user configurable value I think the updates made to Zip FS look better. Alan is on vacation so I would prefer to wait until he gets back and give him a chance to provide any last thoughts on the change to Zip FS. The manual test looks OK and is a good addition - Marked as reviewed by lancea (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/4607
Re: RFR: 8190753: (zipfs): Accessing a large entry (> 2^31 bytes) leads to a negative initial size for ByteArrayOutputStream [v3]
Hi Jaikiran, On Jul 9, 2021, at 8:43 AM, Jaikiran Pai mailto:jai.forums2...@gmail.com>> wrote: On 05/07/21 10:52 am, Jaikiran Pai wrote: 4. I've never previously created a manual test case. The `LargeCompressedEntrySizeTest` in this PR is expected to be a manual test case (given how long it might take to run on various different systems). The only difference between this test case and other jtreg automated tests is the absence of a `@test` on this one. Is this how manual tests are written or is there some other way? We avoid manual tests as there is no guarantee that they will be run. So maybe we'll need to explore the scenario a bit further to see if there is some way to come up with an automated test. The jtreg foo for manual tests is `@run main/manual LargeCompressedEntrySizeTest`. You'll see a few examples in the test suite but I don't know if they are ever run. I have updated the PR to use jtreg's construct of @run testng/manual to mark this as a manual test. I will post the timing of this test case later today after I run the latest version locally and see how long it's taking. On my local setup, the LargeCompressedEntrySizeTest (latest version of this PR) consistently takes between 205 to 215 seconds to complete (so between 3 to 4 minutes). Is that something that will allow it to be added as part of automated tests or is it too long for automated tests? Thank you for the info. This test needs to remain a manual test. I have a full test run going and will finish my review later today or tomorrow Best Lance -Jaikiran [cid:E1C4E2F0-ECD0-4C9D-ADB4-B16CA7BCB7FC@home] Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com<mailto:lance.ander...@oracle.com>
Re: [jdk17] RFR: 6766844: ByteArrayInputStream#read with a byte array of length 0 not consistent with InputStream when at EOF [v2]
On Fri, 2 Jul 2021 16:58:18 GMT, Brian Burkhalter wrote: >> Modify the specification of >> `java.io.ByteArrayInputStream#read(byte[],int,int)` to indicate that `-1` is >> returned instead of `0` when the stream is at its end and the third >> parameter, `len`, is zero. > > Brian Burkhalter has updated the pull request incrementally with one > additional commit since the last revision: > > 6766844: Correct error messages in test Marked as reviewed by lancea (Reviewer). - PR: https://git.openjdk.java.net/jdk17/pull/189
Re: RFR: 8251329: (zipfs) Files.walkFileTree walks infinitely if zip has dir named "." inside [v4]
On Jul 2, 2021, at 12:13 PM, Lance Andersen
mailto:lance.ander...@oracle.com>> wrote:
On Jul 2, 2021, at 8:08 AM, Jaikiran Pai
mailto:jai.forums2...@gmail.com>> wrote:
Hello Lance,
On 02/07/21 4:42 pm, Lance Andersen wrote:
Hi Jaikiran,
Consider:
try (var os = Files.newOutputStream(ZIPFILE);
ZipOutputStream zos = new ZipOutputStream(os)) {
zos.putNextEntry(new ZipEntry("../Hello.txt"));
zos.write("Hello World".getBytes(StandardCharsets.UTF_8));
}
With your proposed fix, you will only return "/" when you walk the the Zip file
and we should also return "/Hello.txt"
Thank you for noticing this issue in my change and bringing this up. I have a
question around this use case. Please consider a small variation to your
example as below:
try (var os = Files.newOutputStream(ZIPFILE);
ZipOutputStream zos = new ZipOutputStream(os)) {
zos.putNextEntry(new ZipEntry("../Hello.txt"));
zos.write("Hello World".getBytes(StandardCharsets.UTF_8));
zos.closeEntry();
zos.putNextEntry(new ZipEntry("/Hello.txt"));
zos.write("Bye bye".getBytes(StandardCharsets.UTF_8));
zos.closeEntry();
}
Notice that I now have a zip/jar which has 2 differently named entries
"../Hello.txt" and "/Hello.txt". This creates the archive without any issues
and those 2 entries are noted in its listing. Now assuming someone walks over
this jar using the ZipFileSystem, starting at root ("/"), what should be the
expected output? The path(s) returned will end up being "/" and "/Hello.txt"
but which resource is expected to be served in this case? The one which has
"Hello World" in it or the one which has "Bye bye"? This example can be
extended a bit more by introducing a "./Hello.txt", in the jar, with yet
another different content in that entry. Is there some specification for this
that I can check and adapt the test case accordingly?
Consider Hello.zip created via:
try (var os = Files.newOutputStream(Path.of("Hello.zip"));
ZipOutputStream zos = new ZipOutputStream(os)) {
zos.putNextEntry(new ZipEntry("../Hello.txt"));
zos.write("Hello".getBytes(StandardCharsets.UTF_8));
zos.putNextEntry(new ZipEntry("Hello.txt"));
zos.write("Another Hello".getBytes(StandardCharsets.UTF_8));
}
Winzip does not handle this case well.
InfoZip will :
-
unzip ../Hello.zip
Archive: ../Hello.zip
warning: skipped "../" path component(s) in ../Hello.txt
inflating: Hello.txt
replace Hello.txt? [y]es, [n]o, [A]ll, [N]one, [r]ename: r
new name: foo.txt
inflating: foo.txt
% ls
Hello.txt foo.txt
% cat Hello.txt
Hello
% cat foo.txt
Another Hello
%
This scenario is not well covered in the Zip spec, so we can do what we think
is best.
Personally, I am OK with resolving the path. The odds of having multiple
relative paths to the same file would be a mistake in creating the Zip.
That being said, if we want to follow Alan’s suggestion and throw an Exception,
I am OK with that as well.
Either way, we currently cannot access the file via Zip FS due to the call to
ZipPath::getResolvedPath() for all access and the path is only normalized when
the Inodes are created.
Alan, do you have a specific preference?
One more datapoint
Consider:
try (FileSystem zipfs =
FileSystems.newFileSystem(Path.of("ZipFSHello.zip"),
Map.of("create", "true"))) {
Files.writeString(zipfs.getPath("HelloZipfs.txt"), "Hello");
Files.writeString(zipfs.getPath("../HelloZipfs.txt"), "A ZipFS Hello");
}
The above will result in a Zip with a single entry of “HelloZipfs.txt” and a
value of “A ZipFS Hello”
As mentioned in my previous comment, this is due to the use of
ZipPath::getResolvedPath() which brings me back to the suggestion of doing the
same when we create the Inodes given this is an a virtual FS.
HTH
Best
Lance
-Jaikiran
Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com<mailto:lance.ander...@oracle.com>
[cid:E1C4E2F0-ECD0-4C9D-ADB4-B16CA7BCB7FC@home]
Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com<mailto:lance.ander...@oracle.com>
Re: RFR: 8251329: (zipfs) Files.walkFileTree walks infinitely if zip has dir named "." inside [v4]
On Jul 2, 2021, at 8:08 AM, Jaikiran Pai
mailto:jai.forums2...@gmail.com>> wrote:
Hello Lance,
On 02/07/21 4:42 pm, Lance Andersen wrote:
Hi Jaikiran,
Consider:
try (var os = Files.newOutputStream(ZIPFILE);
ZipOutputStream zos = new ZipOutputStream(os)) {
zos.putNextEntry(new ZipEntry("../Hello.txt"));
zos.write("Hello World".getBytes(StandardCharsets.UTF_8));
}
With your proposed fix, you will only return "/" when you walk the the Zip file
and we should also return "/Hello.txt"
Thank you for noticing this issue in my change and bringing this up. I have a
question around this use case. Please consider a small variation to your
example as below:
try (var os = Files.newOutputStream(ZIPFILE);
ZipOutputStream zos = new ZipOutputStream(os)) {
zos.putNextEntry(new ZipEntry("../Hello.txt"));
zos.write("Hello World".getBytes(StandardCharsets.UTF_8));
zos.closeEntry();
zos.putNextEntry(new ZipEntry("/Hello.txt"));
zos.write("Bye bye".getBytes(StandardCharsets.UTF_8));
zos.closeEntry();
}
Notice that I now have a zip/jar which has 2 differently named entries
"../Hello.txt" and "/Hello.txt". This creates the archive without any issues
and those 2 entries are noted in its listing. Now assuming someone walks over
this jar using the ZipFileSystem, starting at root ("/"), what should be the
expected output? The path(s) returned will end up being "/" and "/Hello.txt"
but which resource is expected to be served in this case? The one which has
"Hello World" in it or the one which has "Bye bye"? This example can be
extended a bit more by introducing a "./Hello.txt", in the jar, with yet
another different content in that entry. Is there some specification for this
that I can check and adapt the test case accordingly?
Consider Hello.zip created via:
try (var os = Files.newOutputStream(Path.of("Hello.zip"));
ZipOutputStream zos = new ZipOutputStream(os)) {
zos.putNextEntry(new ZipEntry("../Hello.txt"));
zos.write("Hello".getBytes(StandardCharsets.UTF_8));
zos.putNextEntry(new ZipEntry("Hello.txt"));
zos.write("Another Hello".getBytes(StandardCharsets.UTF_8));
}
Winzip does not handle this case well.
InfoZip will :
-
unzip ../Hello.zip
Archive: ../Hello.zip
warning: skipped "../" path component(s) in ../Hello.txt
inflating: Hello.txt
replace Hello.txt? [y]es, [n]o, [A]ll, [N]one, [r]ename: r
new name: foo.txt
inflating: foo.txt
% ls
Hello.txt foo.txt
% cat Hello.txt
Hello
% cat foo.txt
Another Hello
%
This scenario is not well covered in the Zip spec, so we can do what we think
is best.
Personally, I am OK with resolving the path. The odds of having multiple
relative paths to the same file would be a mistake in creating the Zip.
That being said, if we want to follow Alan’s suggestion and throw an Exception,
I am OK with that as well.
Either way, we currently cannot access the file via Zip FS due to the call to
ZipPath::getResolvedPath() for all access and the path is only normalized when
the Inodes are created.
Alan, do you have a specific preference?
-Jaikiran
[cid:E1C4E2F0-ECD0-4C9D-ADB4-B16CA7BCB7FC@home]
Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com<mailto:lance.ander...@oracle.com>
Re: RFR: 8251329: (zipfs) Files.walkFileTree walks infinitely if zip has dir named "." inside [v4]
On Fri, 2 Jul 2021 11:06:40 GMT, Jaikiran Pai wrote:
>> Can I please get a review of this proposed fix for
>> https://bugs.openjdk.java.net/browse/JDK-8251329?
>>
>> As noted in that issue, if a zip filesystem created on top of a jar
>> containing a "./" entry is "walked" using the `Files.walkFileTree`, it leads
>> to a infinite never ending iteration (which ultimately fails with Java heap
>> space OOM).
>>
>> Alan notes in that issue that:
>>
>>> This is more likely an issue with the zipfs DirectoryStream implementation.
>>> A DirectoryStream is specified to not include elements that for the special
>>> links to the current or parent directory. It should be rare.
>>
>> This indeed turned out to be an issue in the
>> `jdk.nio.zipfs.ZipDirectoryStream#iterator()` implementation where it calls
>> the `jdk.nio.zipfs.ZipFileSystem#iteratorOf(...)` implementation. The
>> implementation, unlike what the javadoc of `java.nio.file.DirectoryStream`
>> states, was including the "." and ".." paths in its returned iterator:
>>
>>> The elements returned by the iterator are in no specific order. Some file
>> systems maintain special links to the directory itself and the directory's
>> parent directory. Entries representing these links are not returned by the
>> iterator.
>>
>>
>> The proposed fix in this patch checks the paths for "." and "..", similar to
>> what the `sun.nio.fs.UnixDirectoryStream` does in its `isSelfOrParent` and
>> skips those paths from being added into the returned iterator. The
>> `jdk.nio.zipfs.ZipFileSystem#iteratorOf(...)` (where this change has been
>> done) is currently only used by
>> `jdk.nio.zipfs.ZipDirectoryStream#iterator()` and has package-private
>> visibility, so this change shouldn't impact any other usage/expectations.
>>
>> A new jtreg test has been added to reproduce this issue and verify the fix.
>> Local testing of the `test/jdk/jdk/nio/` (including this new test) went fine
>> without any issues after this change. I will be triggering a `tier1` test
>> locally in a while.
>
> 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 upstream master branch to bring in fixes that might help
> fix the unrelated tier1 failures in Github Actions job runs
> - implement review suggestions:
> - reduce the toString() calls by creating a helper
> - fs.getPath("/") instead of fs.getRootDirectories().iterator().next()
> - implement review suggestion - move isSelfOrParent to ZipPath class
> - 8251329: (zipfs) Files.walkFileTree walks infinitely if zip has dir named
> "." inside
Hi Jaikiran,
Consider:
try (var os = Files.newOutputStream(ZIPFILE);
ZipOutputStream zos = new ZipOutputStream(os)) {
zos.putNextEntry(new ZipEntry("../Hello.txt"));
zos.write("Hello World".getBytes(StandardCharsets.UTF_8));
}
With your proposed fix, you will only return "/" when you walk the the Zip file
and we should also return "/Hello.txt"
If. you resolve the path when the Inode entry is created:
` name(new ZipPath(null, normalize(name), true).getResolvedPath());`
That should address the issue and also allow you to access the entry.
-
PR: https://git.openjdk.java.net/jdk/pull/4604
Re: [jdk17] RFR: 8269543: The warning for System::setSecurityManager should only appear once for each caller [v2]
On Wed, 30 Jun 2021 15:45:25 GMT, Weijun Wang wrote: >> Add a cache to record which sources have called `System::setSecurityManager` >> and only print out warning lines once for each. > > Weijun Wang has updated the pull request incrementally with one additional > commit since the last revision: > > update cache key from String to Class Marked as reviewed by lancea (Reviewer). - PR: https://git.openjdk.java.net/jdk17/pull/166
Re: RFR: 8190753: (zipfs): Accessing a large entry (> 2^31 bytes) leads to a negative initial size for ByteArrayOutputStream [v2]
Hi Jaikiran On Jun 30, 2021, at 12:15 PM, Jaikiran Pai mailto:jai.forums2...@gmail.com>> wrote: Hello Lance, On 29/06/21 11:31 pm, Lance Andersen wrote: I ran your current test 150 times and the max runtime was 25 seconds, most cases were in the 18-20 second range on our slower test boxes. Thank you for running those tests. Do you think those timings are good enough to let that test stay as a regular automated jtreg test, in tier1? I'm guessing this falls in tier1? I haven't yet looked in detail the tier definitions of the build. These tests run as part of tier2. The time for the test run is reasonable . As part of looking at what happens with a file whose deflated size is > 2gb, I would add a specific test which is a manual test to validate that there is no issue when we cross the 2gb threshold. I added a (manual) test to see what happens in this case. I have committed the test as part of this PR just for the sake of reference. The test is named LargeCompressedEntrySizeTest. The test uses ZipFS to create a (new) zip file and attempts to write out a zip entry whose deflated/compressed size is potentially greater than 2gb. When I run this test case, I consistenly run into the following exception: test LargeCompressedEntrySizeTest.testLargeCompressedSizeWithZipFS(): failure java.lang.OutOfMemoryError: Required array length 2147483639 + 419 is too large at java.base/jdk.internal.util.ArraysSupport.hugeLength(ArraysSupport.java:649) at java.base/jdk.internal.util.ArraysSupport.newLength(ArraysSupport.java:642) at java.base/java.io.ByteArrayOutputStream.ensureCapacity(ByteArrayOutputStream.java:100) at java.base/java.io.ByteArrayOutputStream.write(ByteArrayOutputStream.java:130) at java.base/java.util.zip.DeflaterOutputStream.deflate(DeflaterOutputStream.java:252) at java.base/java.util.zip.DeflaterOutputStream.write(DeflaterOutputStream.java:210) at jdk.zipfs/jdk.nio.zipfs.ZipFileSystem$DeflatingEntryOutputStream.write(ZipFileSystem.java:2016) at java.base/java.io.FilterOutputStream.write(FilterOutputStream.java:108) at LargeCompressedEntrySizeTest.testLargeCompressedSizeWithZipFS(LargeCompressedEntrySizeTest.java:104) which to me is understandable. Is this what you and Alan wanted tested/checked? In its current form I don't see a way to write out a entry whose deflated size exceeds 2gb, unless the user/caller use the "useTempFile=true" option while creating the zip filesystem. FWIW - if I do set this "useTempFile=true" while creating that zip filesystem, in the LargeCompressedEntrySizeTest, that test passes fine and the underlying zip that is created shows a compressed/deflated size as follows: unzip -lv JTwork/scratch/8190753-test-compressed-size.zip Archive: JTwork/scratch/8190753-test-compressed-size.zip Length MethodSize CmprDateTime CRC-32 Name -- --- -- - 2147483649 Defl:N 2148138719 0% 06-30-2021 21:39 52cab9f8 LargeZipEntry.txt --- ------ 2147483649 2148138719 0%1 file I understand that Alan's suggestion holds good and we should have some logic in place which switches to using a temp file once we notice that the sizes we are dealing with can exceed some threshold, but I guess that is something we need to do separately outside of this PR? Yes the intent would be to add some logic, which might need to be under a property (for now) to specify the size for when to use a temp file vs BAOS. Having the value configurable via a property might give us some flexibility for experimentation. I don’t see why this PR could not be used for this (as it would provide a more complete solution) Best Lance -Jaikiran [cid:E1C4E2F0-ECD0-4C9D-ADB4-B16CA7BCB7FC@home] Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com<mailto:lance.ander...@oracle.com>
Re: RFR: 8190753: (zipfs): Accessing a large entry (> 2^31 bytes) leads to a negative initial size for ByteArrayOutputStream [v2]
On Mon, 28 Jun 2021 09:13:31 GMT, Jaikiran Pai wrote:
>> Can I please get a review for this proposed fix for the issue reported in
>> https://bugs.openjdk.java.net/browse/JDK-8190753?
>>
>> The commit here checks for the size of the zip entry before trying to create
>> a `ByteArrayOutputStream` for that entry's content. A new jtreg test has
>> been included in this commit to reproduce the issue and verify the fix.
>>
>> P.S: It's still a bit arguable whether it's a good idea to create the
>> `ByteArrayOutputStream` with an initial size potentially as large as the
>> `MAX_ARRAY_SIZE` or whether it's better to just use some smaller default
>> value. However, I think that can be addressed separately while dealing with
>> https://bugs.openjdk.java.net/browse/JDK-8011146
>
> Jaikiran Pai has updated the pull request incrementally with one additional
> commit since the last revision:
>
> add @requires to the new test to selectively decide where the test runs
Thank you for looking into this issue.
I ran your current test 150 times and the max runtime was 25 seconds, most
cases were in the 18-20 second range on our slower test boxes.
As part of looking at what happens with a file whose deflated size is > 2gb, I
would add a specific test which is a manual test to validate that there is no
issue when we cross the 2gb threshold.
src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java line 1954:
> 1952: } else {
> 1953: os = new ByteArrayOutputStream((e.size > 0 && e.size <=
> MAX_ARRAY_SIZE)? (int)e.size : 8192);
> 1954: }
The proposed change will address the specific issue shown in the bug. As Alan
points out, there could be an issue if the deflated size is > 2gb. It would be
good to look into that as part of your proposed fix.
-
PR: https://git.openjdk.java.net/jdk/pull/4607
Re: RFR: 8190753: (zipfs): Accessing a large entry (> 2^31 bytes) leads to a negative initial size for ByteArrayOutputStream
Hi Jaikiran, This is on my list to look at but did not get to today. Best Lance On Jun 27, 2021, at 11:52 PM, Jaikiran Pai mailto:j...@openjdk.java.net>> wrote: Can I please get a review for this proposed fix for the issue reported in https://bugs.openjdk.java.net/browse/JDK-8190753? The commit here checks for the size of the zip entry before trying to create a `ByteArrayOutputStream` for that entry's content. A new jtreg test has been included in this commit to reproduce the issue and verify the fix. P.S: It's still a bit arguable whether it's a good idea to create the `ByteArrayOutputStream` with an initial size potentially as large as the `MAX_ARRAY_SIZE` or whether it's better to just use some smaller default value. However, I think that can be addressed separately while dealing with https://bugs.openjdk.java.net/browse/JDK-8011146 - Commit messages: - 8190753: (zipfs): Accessing a large entry (> 2^31 bytes) leads to a negative initial size for ByteArrayOutputStream Changes: https://git.openjdk.java.net/jdk/pull/4607/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4607=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8190753 Stats: 139 lines in 2 files changed: 138 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/4607.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4607/head:pull/4607 PR: https://git.openjdk.java.net/jdk/pull/4607 [cid:E1C4E2F0-ECD0-4C9D-ADB4-B16CA7BCB7FC@home] Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com<mailto:lance.ander...@oracle.com>
Re: [jdk17] RFR: 8269513: Clarify the spec wrt `useOldISOCodes` system property [v4]
On Mon, 28 Jun 2021 20:33:29 GMT, Naoto Sato wrote: >> Please review this small doc change to the system property. Accompanying CSR >> has also been created. > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Missed one. Marked as reviewed by lancea (Reviewer). - PR: https://git.openjdk.java.net/jdk17/pull/163
Re: [jdk17] RFR: 8269513: Clarify the spec wrt `useOldISOCodes` system property [v2]
On Mon, 28 Jun 2021 18:37:34 GMT, Naoto Sato wrote:
>> Please review this small doc change to the system property. Accompanying CSR
>> has also been created.
>
> Naoto Sato has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Refined wording.
src/java.base/share/classes/java/util/Locale.java line 460:
> 458: * back to that of before Java SE 17. If the system property is set to
> 459: * {@code true}, those three current language codes are mapped to their
> 460: * backward compatible forms. It is only read at Java runtime startup,
> so a
I had thought about some of some minor word smithing in your prior commit, but
chose not to request a change.
In the above sentence, It could be clearer what "It" is. Perhaps something
along the lines of:
This property is only read at Java runtime startup and subsequents calls to
"
-
PR: https://git.openjdk.java.net/jdk17/pull/163
Re: RFR: 8269409: Post JEP 411 refactoring: core-libs with maximum covering > 10K
On Mon, 28 Jun 2021 18:03:56 GMT, Weijun Wang wrote: > More refactoring to limit the scope of `@SuppressWarnings` annotations. > > Sometimes I introduce new methods. Please feel free to suggest method names > you like to use. > > Note: this is copied from https://github.com/openjdk/jdk17/pull/152. The revisions you made as part of the push to JDK 18 look fine Max. - Marked as reviewed by lancea (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/4615
Re: [jdk17] RFR: 8269513: Clarify the spec wrt `useOldISOCodes` system property
On Mon, 28 Jun 2021 16:57:15 GMT, Naoto Sato wrote: > Please review this small doc change to the system property. Accompanying CSR > has also been created. Looks good Naoto - Marked as reviewed by lancea (Reviewer). PR: https://git.openjdk.java.net/jdk17/pull/163
Re: [jdk17] RFR: 8269409: Post JEP 411 refactoring: core-libs with maximum covering > 10K [v2]
On Fri, 25 Jun 2021 23:40:27 GMT, Weijun Wang wrote: >> More refactoring to limit the scope of `@SuppressWarnings` annotations. >> >> Sometimes I introduce new methods. Please feel free to suggest method names >> you like to use. > > Weijun Wang has updated the pull request incrementally with one additional > commit since the last revision: > > one more Marked as reviewed by lancea (Reviewer). - PR: https://git.openjdk.java.net/jdk17/pull/152
Re: [jdk17] RFR: 8269409: Post JEP 411 refactoring: core-libs with maximum covering > 10K
On Fri, 25 Jun 2021 20:04:37 GMT, Weijun Wang wrote: > More refactoring to limit the scope of `@SuppressWarnings` annotations. > > Sometimes I introduce new methods. Please feel free to suggest method names > you like to use. Changes look good Max - Marked as reviewed by lancea (Reviewer). PR: https://git.openjdk.java.net/jdk17/pull/152
Re: [jdk17] RFR: 8256919: BCEL: Utility.encode forget to close
On Thu, 24 Jun 2021 20:36:29 GMT, Joe Wang wrote: > Fix a regression caused by the previous BCEL update. The issue was fixed in > the current BCEL repo with a reversal of the previous code, adding back > "gos.close();". Note however, doing so will result in a warning: [try] > explicit call to close() on an auto-closeable resource. This fix calls > finish() instead. Looks OK to me - Marked as reviewed by lancea (Reviewer). PR: https://git.openjdk.java.net/jdk17/pull/141
Re: RFR: 4847239: (spec) File.createTempFile() should make it clear that it doesn't create the temporary directory [v4]
On Thu, 24 Jun 2021 18:43:00 GMT, Brian Burkhalter wrote: >> Augment the specification of >> `java.io.File.createTempFile(String,String,File)` to clarify its behavior >> with respect to the `File` parameter `directory`. > > Brian Burkhalter has refreshed the contents of this pull request, and > previous commits have been removed. The incremental views will show > differences compared to the previous content of the PR. Marked as reviewed by lancea (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4561
Re: RFR: 8269124: Update java.time to use switch expressions (part II) [v5]
On Thu, 24 Jun 2021 12:01:04 GMT, Patrick Concannon wrote: >> Hi, >> >> Could someone please review the second half of my update for the `java.time` >> package to make use of switch expressions? >> >> This PR was split into two parts due to the large number of files affected. >> >> Kind regards, >> >> Patrick > > Patrick Concannon has updated the pull request incrementally with one > additional commit since the last revision: > > 8269124: Added instanceof pattern variables Marked as reviewed by lancea (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4552
Re: RFR: 4847239: (spec) File.createTempFile() should make it clear that it doesn't create the temporary directory [v2]
On Wed, 23 Jun 2021 23:02:01 GMT, Brian Burkhalter wrote:
>> Augment the specification of
>> `java.io.File.createTempFile(String,String,File)` to clarify its behavior
>> with respect to the `File` parameter `directory`.
>
> Brian Burkhalter has updated the pull request incrementally with one
> additional commit since the last revision:
>
> 4847239: Add test
src/java.base/share/classes/java/io/File.java line 2116:
> 2114: * abstract pathname is valid and denotes an existing directory,
> then the
> 2115: * file will be created in that directory; otherwise, the file will
> not
> 2116: * be created and an {@code IOException} will be thrown. In no
> case will
I might suggest reworking the 1st sentence as it is a tad long and we could
probably reduce the 'then' usages.
Maybe instead of "In no case..." change to "Under no circumstances "
Should it also indicate an IOException will be thrown if the directory does not
exist?
-
PR: https://git.openjdk.java.net/jdk/pull/4561
Re: RFR: 8268457: XML Transformer outputs Unicode supplementary character incorrectly to HTML [v3]
On Wed, 23 Jun 2021 07:34:06 GMT, Masanori Yano wrote: >> Hi all, >> >> Could you please review the 8268457 bug fixes? >> >> The problem is that ToHTMLStream applies processing for non-surrogate pairs >> to the surrogate pair. >> This fix changes the processing for non-surrogate pairs to the else >> condition. > > Masanori Yano has updated the pull request incrementally with one additional > commit since the last revision: > > remove unnecessally comments and add eof line The updated changes look reasonable to me. - Marked as reviewed by lancea (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/4474
Re: RFR: 8269124: Update java.time to use switch expressions (part II) [v3]
On Tue, 22 Jun 2021 17:50:05 GMT, Patrick Concannon wrote: >> Hi, >> >> Could someone please review the second half of my update for the `java.time` >> package to make use of switch expressions? >> >> This PR was split into two parts due to the large number of files affected. >> >> Kind regards, >> >> Patrick > > Patrick Concannon has updated the pull request incrementally with one > additional commit since the last revision: > > 8269124: Added missing return Marked as reviewed by lancea (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4552
Re: RFR: 8266901: Clarify the method description of Duration.toDaysPart()
On Mon, 21 Jun 2021 18:22:26 GMT, Naoto Sato wrote: > Please review this doc clarification fix to `toDaysPart()` method. CSR will > also be filed accordingly. Marked as reviewed by lancea (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4542
Re: RFR: 8153490: Cannot setBytes() if incoming buffer's length is bigger than number of elements we want to insert. [v4]
On Mon, 24 May 2021 11:18:57 GMT, Mitsuru Kariya wrote: >> Fix `SerialBlob.setBytes(long pos, byte[] bytes, int offset, int length)` in >> the following cases: >> >> 1. `pos - 1 + bytes.length - offset > this.length() && pos - 1 + length <= >> this.length()` >>The original implementation throws `ArrayIndexOutOfBoundsException` but >> this case should end successfully. >>(test31) >> >> 2. `pos - 1 + length > this.length()` >>The original implementation throws `ArrayIndexOutOfBoundsException` but >> this case should end successfully. *1 >>(test32) >> >> 3. `pos == this.length() + 1` >>The original implementation throws `SerialException` but this case should >> end successfully. *2 >>(test33) >> >> 4. `length < 0` >>The original implementation throws `ArrayIndexOutOfBoundsException` but >> this case should throw `SerialException`. >>(test34) >> >> 5. `offset + length > Integer.MAX_VALUE` >>The original implementation throws `ArrayIndexOutOfBoundsException` (or >> `OutOfMemoryError` in most cases) but this case should throw >> `SerialException`. >>(test35) >> >> Additionally, fix `SerialClob.setString(long pos, String str, int offset, >> int length)` in the following cases: >> >> 1. `offset > str.length()` >>The original implementaion throws `StringIndexOutOfBoundsException` but >> this case should throw `SerialException`. >>(test39) >> >> 2. `pos - 1 + str.length() - offset > this.length() && pos - 1 + length <= >> this.length()` >>The original implementation throws `ArrayIndexOutOfBoundsException` but >> this case should end successfully. >>(test32) >> >> 3. `pos - 1 + length > this.length()` >>The original implementaion throws `SerialException` but this case should >> end successfully. *3 >>(test40) >> >> 4. `pos == this.length() + 1` >>The original implementaion throws `SerialException` but this case should >> end successfully. *4 >>(test41) >> >> 5. `length < 0` >>The original implementation throws `StringIndexOutOfBoundsException` but >> this case should throw `SerialException`. >>(test42) >> >> 6. `offset + length > Integer.MAX_VALUE` >>The original implementation throws `ArrayIndexOutOfBoundsException` (or >> `OutOfMemoryError` in most cases) but this case should throw >> `SerialException`. >>(test43) >> >> >> The javadoc has also been modified according to the above. >> >> *1 The documentation of `Blob.setBytes()` says, "If the end of the Blob >> value is reached while writing the array of bytes, then the length of the >> Blob value will be increased to accommodate the extra bytes." >> >> *2 The documentation of `Blob.setBytes()` says, "If the value specified for >> pos is greater than the length+1 of the BLOB value then the behavior is >> undefined." >>So, it should work correctly when pos == length+1 of the BLOB value. >> >> *3 The documentation of `Clob.setString()` says, "If the end of the Clob >> value is eached while writing the given string, then the length of the Clob >> value will be increased to accommodate the extra characters." >> >> *4 The documentation of `Clob.setString()` says, "If the value specified for >> pos is greater than the length+1 of the CLOB value then the behavior is >> undefined." >>So, it should work correctly when pos == length+1 of the CLOB value. > > Mitsuru Kariya has updated the pull request incrementally with one additional > commit since the last revision: > > Modify javadoc for consistency I hope to get to the CSR this week. Sorry for the delay - PR: https://git.openjdk.java.net/jdk/pull/4001
Re: [jdk17] RFR: 8265073: XML transformation and indentation when using xml:space [v2]
On Thu, 17 Jun 2021 22:03:46 GMT, Joe Wang wrote: >> The issue was that the attribute was processed before the variable was set >> (e.g. m_preserveSpaces.push). Reversing the order fixed it. > > Joe Wang has updated the pull request incrementally with one additional > commit since the last revision: > > Thanks Naoto. Updated accordingly. Looks fine. I still add the DataProvider name out of habit myself even though it is always the same as the method names for me (some habits are hard to break) - Marked as reviewed by lancea (Reviewer). PR: https://git.openjdk.java.net/jdk17/pull/89
Re: RFR: 8268469: Update java.time to use switch expressions
On Wed, 9 Jun 2021 15:41:59 GMT, Patrick Concannon wrote: > Hi, > > Could someone please review my code for updating the code in the `java.time` > packages to make use of the switch expressions? > > Kind regards, > Patrick Updates look good Patrick - Marked as reviewed by lancea (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/4433
Re: RFR: 8268124: Update java.lang to use switch expressions [v5]
On Wed, 9 Jun 2021 10:25:35 GMT, Patrick Concannon wrote: >> Hi, >> >> Could someone please review my code for updating the code in the `java.lang` >> packages to make use of the switch expressions? >> >> Kind regards, >> Patrick > > Patrick Concannon has updated the pull request incrementally with two > additional commits since the last revision: > > - 8268124: moved cases into separate lines in DirectMethodHandleDescImpl > part II > - 8268124: moved cases onto separate lines in DirectMethodHandleDescImpl Marked as reviewed by lancea (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4312
Re: RFR: 8266835: Add a --validate option to the jar tool [v3]
On Tue, 8 Jun 2021 18:32:36 GMT, Jorn Vernee wrote: >> This patch adds a `--validate` option to the jar tool which can be used to >> validate a jar file that might be malformed. For instance, if a jar is a >> multi-release jar, it is malformed if different versions expose different >> APIs. >> >> The implementation is straight forward since there already exists validation >> logic that is run when creating or updating a jar. This patch just exposes >> that logic directly under a new command line flag. >> >> I've enhanced the existing ApiValidatorTest to also create malformed jars >> using the zip file APIs (the jar tool does not output malformed jars) and >> run them through `jar --validate`. >> >> Note that while the jdk's jar tool does not output malformed jars, >> third-party archiving tools might, or the jar could have been manually >> edited. >> >> Some prior discussion here: >> https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-May/077420.html >> >> Testing: running jdk/tools/jar test suite locally, tier 1-3 (in progress), >> manual testing. > > Jorn Vernee has updated the pull request incrementally with one additional > commit since the last revision: > > Improve help message. The changes look good - Marked as reviewed by lancea (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/3971
Re: RFR: JDK-8266254: Update to use jtreg 6
On Wed, 2 Jun 2021 16:13:48 GMT, Jonathan Gibbons wrote: > Please review the change to update to using jtreg 6. > > The primary change is to the jib-profiles.js file, which specifies the > version of jtreg to use, for those systems that rely on this file. In > addition, the `requiredVersion` has been updated in the various `TEST.ROOT` > files. > > All the tests that could be updated ahead of time have been updated. There > are a few tests remaining that need to be done at this time, because of the > change in the module name for TestNG 7.3. It changed from a default of > `testng` to and explicit `org.testng`. Marked as reviewed by lancea (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4315
Re: RFR: 8266559: XPathEvaluationResult.XPathResultType.NODESET maps to incorrect type
On Sat, 29 May 2021 00:12:09 GMT, Joe Wang wrote:
> Makes a correction to XPathEvaluationResult.XPathResultType.NODESET mapping.
> Clarifies the supported types for the evaluateExpression methods.
>
> Other changes were javadoc tag usages, e.g. s/the code tag/{@code
Marked as reviewed by lancea (Reviewer).
-
PR: https://git.openjdk.java.net/jdk/pull/4258
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v6]
On Mon, 31 May 2021 15:02:57 GMT, Weijun Wang wrote: >> Please review this implementation of [JEP >> 411](https://openjdk.java.net/jeps/411). >> >> The code change is divided into 3 commits. Please review them one by one. >> >> 1. >> https://github.com/openjdk/jdk/commit/576161d15423f58281e384174d28c9f9be7941a1 >> The essential change for this JEP, including the `@Deprecate` annotations >> and spec change. It also update the default value of the >> `java.security.manager` system property to "disallow", and necessary test >> change following this update. >> 2. >> https://github.com/openjdk/jdk/commit/26a54a835e9f84aa528740a7c5c35d07355a8a66 >> Manual changes to several files so that the next commit can be generated >> programatically. >> 3. >> https://github.com/openjdk/jdk/commit/eb6c566ff9207974a03a53335e0e697cffcf0950 >> Automatic changes to other source files to avoid javac warnings on >> deprecation for removal >> >> The 1st and 2nd commits should be reviewed carefully. The 3rd one is >> generated programmatically, see the comment below for more details. If you >> are only interested in a portion of the 3rd commit and would like to review >> it as a separate file, please comment here and I'll generate an individual >> webrev. >> >> Due to the size of this PR, no attempt is made to update copyright years for >> any file to minimize unnecessary merge conflict. >> >> Furthermore, since the default value of `java.security.manager` system >> property is now "disallow", most of the tests calling >> `System.setSecurityManager()` need to launched with >> `-Djava.security.manager=allow`. This is covered in a different PR at >> https://github.com/openjdk/jdk/pull/4071. >> >> Update: the deprecation annotations and javadoc tags, build, compiler, >> core-libs, hotspot, i18n, jmx, net, nio, security, and serviceability are >> reviewed. Rest are 2d, awt, beans, sound, and swing. > > Weijun Wang has updated the pull request incrementally with one additional > commit since the last revision: > > default behavior reverted to allow Marked as reviewed by lancea (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8267670: Update java.io, java.math, and java.text to use switch expressions [v8]
On Thu, 27 May 2021 15:33:36 GMT, Patrick Concannon wrote: >> Hi, >> >> Could someone please review my code for updating the code in the `java.io`, >> `java.math`, and `java.text` packages to make use of the switch expressions? >> >> Kind regards, >> Patrick > > Patrick Concannon 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 ten additional > commits since the last revision: > > - Merge remote-tracking branch 'origin/master' into JDK-8267670 > - Merge remote-tracking branch 'origin/master' into JDK-8267670 > - 8267670: Added missing brace > - Merge remote-tracking branch 'origin/master' into JDK-8267670 > - 8267670: Removed trailing whitespace > - 8267670: Remove redundant code from switch > - 8267670: Updated code to use yield > - Merge remote-tracking branch 'origin/master' into JDK-8267670 > - 8267670: Update java.io, java.math, and java.text to use switch expressions Looks good Patrick - Marked as reviewed by lancea (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/4182
Re: RFR: 8265248: Implementation Specific Properties: change prefix, plus add existing properties [v7]
On Mon, 24 May 2021 06:23:59 GMT, Joe Wang wrote: >> Update module summary, add a few existing properties and features into the >> tables. > > Joe Wang has updated the pull request incrementally with one additional > commit since the last revision: > > Consolidated impl specific properties; deprecated legacy properties; made > new ones take preference Marked as reviewed by lancea (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/3644
Re: RFR: 8153490: Cannot setBytes() if incoming buffer's length is bigger than number of elements we want to insert. [v3]
On Mon, 17 May 2021 14:39:05 GMT, Mitsuru Kariya wrote: >> Fix `SerialBlob.setBytes(long pos, byte[] bytes, int offset, int length)` in >> the following cases: >> >> 1. `pos - 1 + bytes.length - offset > this.length() && pos - 1 + length <= >> this.length()` >>The original implementation throws `ArrayIndexOutOfBoundsException` but >> this case should end successfully. >>(test31) >> >> 2. `pos - 1 + length > this.length()` >>The original implementation throws `ArrayIndexOutOfBoundsException` but >> this case should end successfully. *1 >>(test32) >> >> 3. `pos == this.length() + 1` >>The original implementation throws `SerialException` but this case should >> end successfully. *2 >>(test33) >> >> 4. `length < 0` >>The original implementation throws `ArrayIndexOutOfBoundsException` but >> this case should throw `SerialException`. >>(test34) >> >> 5. `offset + length > Integer.MAX_VALUE` >>The original implementation throws `ArrayIndexOutOfBoundsException` (or >> `OutOfMemoryError` in most cases) but this case should throw >> `SerialException`. >>(test35) >> >> Additionally, fix `SerialClob.setString(long pos, String str, int offset, >> int length)` in the following cases: >> >> 1. `offset > str.length()` >>The original implementaion throws `StringIndexOutOfBoundsException` but >> this case should throw `SerialException`. >>(test39) >> >> 2. `pos - 1 + str.length() - offset > this.length() && pos - 1 + length <= >> this.length()` >>The original implementation throws `ArrayIndexOutOfBoundsException` but >> this case should end successfully. >>(test32) >> >> 3. `pos - 1 + length > this.length()` >>The original implementaion throws `SerialException` but this case should >> end successfully. *3 >>(test40) >> >> 4. `pos == this.length() + 1` >>The original implementaion throws `SerialException` but this case should >> end successfully. *4 >>(test41) >> >> 5. `length < 0` >>The original implementation throws `StringIndexOutOfBoundsException` but >> this case should throw `SerialException`. >>(test42) >> >> 6. `offset + length > Integer.MAX_VALUE` >>The original implementation throws `ArrayIndexOutOfBoundsException` (or >> `OutOfMemoryError` in most cases) but this case should throw >> `SerialException`. >>(test43) >> >> >> The javadoc has also been modified according to the above. >> >> *1 The documentation of `Blob.setBytes()` says, "If the end of the Blob >> value is reached while writing the array of bytes, then the length of the >> Blob value will be increased to accommodate the extra bytes." >> >> *2 The documentation of `Blob.setBytes()` says, "If the value specified for >> pos is greater than the length+1 of the BLOB value then the behavior is >> undefined." >>So, it should work correctly when pos == length+1 of the BLOB value. >> >> *3 The documentation of `Clob.setString()` says, "If the end of the Clob >> value is eached while writing the given string, then the length of the Clob >> value will be increased to accommodate the extra characters." >> >> *4 The documentation of `Clob.setString()` says, "If the value specified for >> pos is greater than the length+1 of the CLOB value then the behavior is >> undefined." >>So, it should work correctly when pos == length+1 of the CLOB value. > > Mitsuru Kariya has updated the pull request incrementally with one additional > commit since the last revision: > > Fix for length + offset > Integer.MAX_VALUE case I have run the JCK tests in addition to to the JTREG Tess to validate there are no additional failures due to these changes - PR: https://git.openjdk.java.net/jdk/pull/4001
Re: RFR: 8153490: Cannot setBytes() if incoming buffer's length is bigger than number of elements we want to insert. [v3]
On Mon, 17 May 2021 14:39:05 GMT, Mitsuru Kariya
wrote:
>> Fix `SerialBlob.setBytes(long pos, byte[] bytes, int offset, int length)` in
>> the following cases:
>>
>> 1. `pos - 1 + bytes.length - offset > this.length() && pos - 1 + length <=
>> this.length()`
>>The original implementation throws `ArrayIndexOutOfBoundsException` but
>> this case should end successfully.
>>(test31)
>>
>> 2. `pos - 1 + length > this.length()`
>>The original implementation throws `ArrayIndexOutOfBoundsException` but
>> this case should end successfully. *1
>>(test32)
>>
>> 3. `pos == this.length() + 1`
>>The original implementation throws `SerialException` but this case should
>> end successfully. *2
>>(test33)
>>
>> 4. `length < 0`
>>The original implementation throws `ArrayIndexOutOfBoundsException` but
>> this case should throw `SerialException`.
>>(test34)
>>
>> 5. `offset + length > Integer.MAX_VALUE`
>>The original implementation throws `ArrayIndexOutOfBoundsException` (or
>> `OutOfMemoryError` in most cases) but this case should throw
>> `SerialException`.
>>(test35)
>>
>> Additionally, fix `SerialClob.setString(long pos, String str, int offset,
>> int length)` in the following cases:
>>
>> 1. `offset > str.length()`
>>The original implementaion throws `StringIndexOutOfBoundsException` but
>> this case should throw `SerialException`.
>>(test39)
>>
>> 2. `pos - 1 + str.length() - offset > this.length() && pos - 1 + length <=
>> this.length()`
>>The original implementation throws `ArrayIndexOutOfBoundsException` but
>> this case should end successfully.
>>(test32)
>>
>> 3. `pos - 1 + length > this.length()`
>>The original implementaion throws `SerialException` but this case should
>> end successfully. *3
>>(test40)
>>
>> 4. `pos == this.length() + 1`
>>The original implementaion throws `SerialException` but this case should
>> end successfully. *4
>>(test41)
>>
>> 5. `length < 0`
>>The original implementation throws `StringIndexOutOfBoundsException` but
>> this case should throw `SerialException`.
>>(test42)
>>
>> 6. `offset + length > Integer.MAX_VALUE`
>>The original implementation throws `ArrayIndexOutOfBoundsException` (or
>> `OutOfMemoryError` in most cases) but this case should throw
>> `SerialException`.
>>(test43)
>>
>>
>> The javadoc has also been modified according to the above.
>>
>> *1 The documentation of `Blob.setBytes()` says, "If the end of the Blob
>> value is reached while writing the array of bytes, then the length of the
>> Blob value will be increased to accommodate the extra bytes."
>>
>> *2 The documentation of `Blob.setBytes()` says, "If the value specified for
>> pos is greater than the length+1 of the BLOB value then the behavior is
>> undefined."
>>So, it should work correctly when pos == length+1 of the BLOB value.
>>
>> *3 The documentation of `Clob.setString()` says, "If the end of the Clob
>> value is eached while writing the given string, then the length of the Clob
>> value will be increased to accommodate the extra characters."
>>
>> *4 The documentation of `Clob.setString()` says, "If the value specified for
>> pos is greater than the length+1 of the CLOB value then the behavior is
>> undefined."
>>So, it should work correctly when pos == length+1 of the CLOB value.
>
> Mitsuru Kariya has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fix for length + offset > Integer.MAX_VALUE case
Overall the changes look reasonable. As mentioned in the comments, a CSR will
be required due to some of the wordsmithing cleanup
src/java.sql.rowset/share/classes/javax/sql/rowset/serial/SerialBlob.java line
306:
> 304: *
> 305: * @param pos the position in the SQL BLOB value at
> which
> 306: * to start writing. The first position is 1;
When updating the javadoc to use @code, please update all instances for
consistency
src/java.sql.rowset/share/classes/javax/sql/rowset/serial/SerialBlob.java line
308:
> 306: * to start writing. The first position is 1;
> 307: * must not be less than 1 nor greater than
> 308: * the length+1 of this {@code SerialBlob} object.
Changes such as this require a CSR. I think I have convinced myself that it is
OK to move forward with the CSR.
src/java.sql.rowset/share/classes/javax/sql/rowset/serial/SerialBlob.java line
313:
> 311: * @return the number of bytes written
> 312: * @throws SerialException if there is an error accessing the
> 313: * {@code BLOB} value; or if an invalid position is set;
Even though this addresses a typo, this will require a CSR
-
Changes requested by lancea (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/4001
Re: RFR: 8267110: Update java.util to use instanceof pattern variable
On Tue, 18 May 2021 10:37:21 GMT, Patrick Concannon wrote: > Hi, > > Could someone please review my code for updating the code in the `java.util` > package to make use of the `instanceof` pattern variable? > > Kind regards, > Patrick Changes look good. - Marked as reviewed by lancea (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/4088
Integrated: 8267180: Typo in copyright header for HashesTest
On Fri, 14 May 2021 17:44:20 GMT, Lance Andersen wrote: > Please review this fix for a typo in the copyright header in HashTest which > the jdk-tier1 does not catch > > Best > Lance This pull request has now been integrated. Changeset: 5eda812f Author: Lance Andersen URL: https://git.openjdk.java.net/jdk/commit/5eda812f53bfe65d11f6241b0831c588c1400b08 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod 8267180: Typo in copyright header for HashesTest Reviewed-by: dcubed, naoto, joehw - PR: https://git.openjdk.java.net/jdk/pull/4034
Integrated: 8267180: Typo in copyright header for HashesTest
Please review this fix for a typo in the copyright header in HashTest which the jdk-tier1 does not catch Best Lance - Commit messages: - Typo in copyright header for HashesTest Changes: https://git.openjdk.java.net/jdk/pull/4034/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4034=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8267180 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/4034.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4034/head:pull/4034 PR: https://git.openjdk.java.net/jdk/pull/4034
Integrated: 8266461: tools/jmod/hashes/HashesTest.java fails: static @Test methods
On Thu, 13 May 2021 10:49:21 GMT, Lance Andersen wrote: > HI all, > > Please review the fix to HashesTest.java to support running on TestNG 7.4. > > The fix adds a no-arg constructor which TestNG requires. > > The change allows the test to run with the current jtreg as well as the > upcoming jtreg > > > Best > Lance This pull request has now been integrated. Changeset: e90388bc Author:Lance Andersen URL: https://git.openjdk.java.net/jdk/commit/e90388bc1e7bba92675fa799d9da77aa4d6e1a05 Stats: 151 lines in 1 file changed: 8 ins; 25 del; 118 mod 8266461: tools/jmod/hashes/HashesTest.java fails: static @Test methods Reviewed-by: alanb, mchung - PR: https://git.openjdk.java.net/jdk/pull/4009
Re: RFR: 8266461: tools/jmod/hashes/HashesTest.java fails: static @Test methods [v5]
> HI all, > > Please review the fix to HashesTest.java to support running on TestNG 7.4. > > The fix adds a no-arg constructor which TestNG requires. > > The change allows the test to run with the current jtreg as well as the > upcoming jtreg > > > Best > Lance Lance Andersen has updated the pull request incrementally with one additional commit since the last revision: Additional formatting cleanup - Changes: - all: https://git.openjdk.java.net/jdk/pull/4009/files - new: https://git.openjdk.java.net/jdk/pull/4009/files/2e8f6c97..fe8b5a3c Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4009=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4009=03-04 Stats: 6 lines in 1 file changed: 3 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/4009.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4009/head:pull/4009 PR: https://git.openjdk.java.net/jdk/pull/4009
Re: RFR: 8266461: tools/jmod/hashes/HashesTest.java fails: static @Test methods [v4]
On Fri, 14 May 2021 08:02:29 GMT, Alan Bateman wrote:
>> Lance Andersen has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Adjust imports and spacing
>
> test/jdk/tools/jmod/hashes/HashesTest.java line 389:
>
>> 387: ModuleReference mref =
>> finder.find(name).orElseThrow(RuntimeException::new);
>> 388: try {
>> 389: try (ModuleReader reader = mref.open(); InputStream in =
>> reader.open("module-info.class").get()) {
>
> Thats for doing the right thing and using BeforeMethod. The changes looks
> good.
> There is some random re-formatting in hashes and deleteDirectory, I don't
> know if this was intended or not. I think we should at least resolve L389
> before integrating.
Thank you Alan. The re-formatting was done automagically by Intellij. I just
pushed an update to address your suggestions.
-
PR: https://git.openjdk.java.net/jdk/pull/4009
Re: RFR: 8266461: tools/jmod/hashes/HashesTest.java fails: static @Test methods [v4]
> HI all, > > Please review the fix to HashesTest.java to support running on TestNG 7.4. > > The fix adds a no-arg constructor which TestNG requires. > > The change allows the test to run with the current jtreg as well as the > upcoming jtreg > > > Best > Lance Lance Andersen has updated the pull request incrementally with one additional commit since the last revision: Adjust imports and spacing - Changes: - all: https://git.openjdk.java.net/jdk/pull/4009/files - new: https://git.openjdk.java.net/jdk/pull/4009/files/a36e9f51..2e8f6c97 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4009=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4009=02-03 Stats: 33 lines in 1 file changed: 16 ins; 8 del; 9 mod Patch: https://git.openjdk.java.net/jdk/pull/4009.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4009/head:pull/4009 PR: https://git.openjdk.java.net/jdk/pull/4009
Re: RFR: 8266461: tools/jmod/hashes/HashesTest.java fails: static @Test methods [v3]
On Thu, 13 May 2021 18:53:25 GMT, Mandy Chung wrote:
>> Lance Andersen has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Update copyright typo
>
> test/jdk/tools/jmod/hashes/HashesTest.java line 90:
>
>> 88: }
>> 89: this.mods = dest.resolve("mods");
>> 90: Path srcDir = dest.resolve("src");
>
> you can just get rid of this local variable and do this in line 92:
>
> this.builder = new ModuleInfoMaker(dest.resolve("src"));
Yes I can do that and realized that after I pushed the update
> test/jdk/tools/jmod/hashes/HashesTest.java line 387:
>
>> 385: if (hashes != null) {
>> 386: hashes.names().stream().sorted().forEach(n ->
>> 387: System.out.format(" %s %s%n", n,
>> toHex(hashes.hashFor(n)))
>
> Nit: the extra whitespaces in line 384 and 387 may be added by IDE. Can you
> revert it.
Intellij did that. I can tweak
-
PR: https://git.openjdk.java.net/jdk/pull/4009
Re: RFR: 8266461: tools/jmod/hashes/HashesTest.java fails: static @Test methods [v2]
On Thu, 13 May 2021 18:26:52 GMT, Lance Andersen wrote: >> HI all, >> >> Please review the fix to HashesTest.java to support running on TestNG 7.4. >> >> The fix adds a no-arg constructor which TestNG requires. >> >> The change allows the test to run with the current jtreg as well as the >> upcoming jtreg >> >> >> Best >> Lance > > Lance Andersen has updated the pull request incrementally with one additional > commit since the last revision: > > Updates based on feedback and additional cleanup I can look to revert the imports, that was an optimization via Intellij - PR: https://git.openjdk.java.net/jdk/pull/4009
Re: RFR: 8266461: tools/jmod/hashes/HashesTest.java fails: static @Test methods [v3]
> HI all, > > Please review the fix to HashesTest.java to support running on TestNG 7.4. > > The fix adds a no-arg constructor which TestNG requires. > > The change allows the test to run with the current jtreg as well as the > upcoming jtreg > > > Best > Lance Lance Andersen has updated the pull request incrementally with one additional commit since the last revision: Update copyright typo - Changes: - all: https://git.openjdk.java.net/jdk/pull/4009/files - new: https://git.openjdk.java.net/jdk/pull/4009/files/201b9a2b..a36e9f51 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4009=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4009=01-02 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/4009.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4009/head:pull/4009 PR: https://git.openjdk.java.net/jdk/pull/4009
Re: RFR: 8266461: tools/jmod/hashes/HashesTest.java fails: static @Test methods
On Thu, 13 May 2021 10:49:21 GMT, Lance Andersen wrote: > HI all, > > Please review the fix to HashesTest.java to support running on TestNG 7.4. > > The fix adds a no-arg constructor which TestNG requires. > > The change allows the test to run with the current jtreg as well as the > upcoming jtreg > > > Best > Lance The latest commit takes into account the feedback received > I also think it's good to fix this properly. Each test wants to run in an > unique directory. Another solution is to make the fields non-final and add a > new `@BeforeMethod` method to generate the unique pathname for each test. I > think this seems a clean way (I sent you a patch to checkout). The latest commit leverages @BeforeMethod vs an inner class and also includes some additional minor cleanup - PR: https://git.openjdk.java.net/jdk/pull/4009
Re: RFR: 8266461: tools/jmod/hashes/HashesTest.java fails: static @Test methods [v2]
> HI all, > > Please review the fix to HashesTest.java to support running on TestNG 7.4. > > The fix adds a no-arg constructor which TestNG requires. > > The change allows the test to run with the current jtreg as well as the > upcoming jtreg > > > Best > Lance Lance Andersen has updated the pull request incrementally with one additional commit since the last revision: Updates based on feedback and additional cleanup - Changes: - all: https://git.openjdk.java.net/jdk/pull/4009/files - new: https://git.openjdk.java.net/jdk/pull/4009/files/12c7c4b1..201b9a2b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4009=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4009=00-01 Stats: 182 lines in 1 file changed: 7 ins; 41 del; 134 mod Patch: https://git.openjdk.java.net/jdk/pull/4009.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4009/head:pull/4009 PR: https://git.openjdk.java.net/jdk/pull/4009
Re: RFR: 8266461: tools/jmod/hashes/HashesTest.java fails: static @Test methods
On Thu, 13 May 2021 11:24:34 GMT, Chris Hegarty wrote: > The non-static state in this test class is initialized for each of the static > testXXX scenarios. An alternative could be to move said state (four fields) > into a static inner class, and have each of the testXXX scenarios create an > instance of that class with the test-specific path. That would also allow the > addition of the no-args public constructor to HashesTest, and the testXXX > methods to be made non-static. I had originally thought about introducing an inner class but decided that given TestNG needs access to the default constructor, I chose that route vs. doing more of an update. I can revisit this though - PR: https://git.openjdk.java.net/jdk/pull/4009
Re: RFR: 8266461: tools/jmod/hashes/HashesTest.java fails: static @Test methods
On Thu, 13 May 2021 11:11:23 GMT, Alan Bateman wrote: >> HI all, >> >> Please review the fix to HashesTest.java to support running on TestNG 7.4. >> >> The fix adds a no-arg constructor which TestNG requires. >> >> The change allows the test to run with the current jtreg as well as the >> upcoming jtreg >> >> >> Best >> Lance > > test/jdk/tools/jmod/hashes/HashesTest.java line 94: > >> 92: lib= null; >> 93: builder=null; >> 94: } > > This looks like a workaround. Can you instead see if the fields can be > changed to non-final and place the constructor with a method that has the > `@BeforeClass` annotation? I can look to update the test. As I mentioned in my reply to Chris, I had thought about introducing an inner class bug decided to go the route of the default/no-arg constructor as it required fewer changes. - PR: https://git.openjdk.java.net/jdk/pull/4009
RFR: 8266461: tools/jmod/hashes/HashesTest.java fails: static @Test methods
HI all, Please review the fix to HashesTest.java to support running on TestNG 7.4. The fix adds a no-arg constructor which TestNG requires. The change allows the test to run with the current jtreg as well as the upcoming jtreg Best Lance - Commit messages: - Remove extrawhitespace - Update HashesTest for TestNG 7.4 Changes: https://git.openjdk.java.net/jdk/pull/4009/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4009=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8266461 Stats: 7 lines in 1 file changed: 6 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/4009.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4009/head:pull/4009 PR: https://git.openjdk.java.net/jdk/pull/4009
Re: RFR: 8153490: Cannot setBytes() if incoming buffer's length is bigger than number of elements we want to insert.
I won’t have time to look at this today, might not be until over the weekend. On May 12, 2021, at 2:07 PM, Mitsuru Kariya mailto:github.com+2217224+kariya-mits...@openjdk.java.net>> wrote: Fix `SerialBlob.setBytes(long pos, byte[] bytes, int offset, int length)` in the following cases: 1. `pos - 1 + bytes.length - offset > this.length() && pos - 1 + length <= this.length()` The original implementation throws `ArrayIndexOutOfBoundsException` but this case should end successfully. (test31) 2. `pos - 1 + length > this.length()` The original implementation throws `ArrayIndexOutOfBoundsException` but this case should end successfully. *1 (test32) 3. `pos == this.length() + 1` The original implementation throws `SerialException` but this case should end successfully. *2 (test33) Additionally, fix `SerialClob.setString(long pos, String str, int offset, int length)` in the following cases: 1. `offset > str.length()` The original implementaion throws `StringIndexOutOfBoundsException` but this case should throw `SerialException`. (test39) 2. `pos - 1 + str.length() - offset > this.length() && pos - 1 + length <= this.length()` The original implementation throws `ArrayIndexOutOfBoundsException` but this case should end successfully. (test32) 3. `pos - 1 + length > this.length()` The original implementaion throws `SerialException` but this case should end successfully. *3 (test40) 4. `pos == this.length() + 1` The original implementaion throws `SerialException` but this case should end successfully. *4 (test41) The javadoc has also been modified according to the above. The items below should would change the spec change, require a CSR and should be looked at separately *1 The documentation of `Blob.setBytes()` says, "If the end of the Blob value is reached while writing the array of bytes, then the length of the Blob value will be increased to accommodate the extra bytes." *2 The documentation of `Blob.setBytes()` says, "If the value specified for pos is greater than the length+1 of the BLOB value then the behavior is undefined." So, it should work correctly when pos == length+1 of the BLOB value. *3 The documentation of `Clob.setString()` says, "If the end of the Clob value is eached while writing the given string, then the length of the Clob value will be increased to accommodate the extra characters." *4 The documentation of `Clob.setString()` says, "If the value specified for pos is greater than the length+1 of the CLOB value then the behavior is undefined." So, it should work correctly when pos == length+1 of the CLOB value. - Commit messages: - 8153490:Cannot setBytes() if incoming buffer's length is bigger than number of elements we want to insert. Changes: https://git.openjdk.java.net/jdk/pull/4001/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4001=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8153490 Stats: 179 lines in 4 files changed: 122 ins; 17 del; 40 mod Patch: https://git.openjdk.java.net/jdk/pull/4001.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4001/head:pull/4001 PR: https://git.openjdk.java.net/jdk/pull/4001 [cid:E1C4E2F0-ECD0-4C9D-ADB4-B16CA7BCB7FC@home] Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com<mailto:lance.ander...@oracle.com>
Re: RFR: 8265248: Implementation Specific Properties: change prefix, plus add existing properties [v5]
On Wed, 12 May 2021 19:13:43 GMT, Joe Wang wrote: >> Update module summary, add a few existing properties and features into the >> tables. > > Joe Wang has updated the pull request incrementally with one additional > commit since the last revision: > > Thanks Roger. Changed to fully qualified names. Also made them align left > instead of center Marked as reviewed by lancea (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/3644
Re: RFR: 8265248: Implementation Specific Properties: change prefix, plus add existing properties [v4]
On Wed, 12 May 2021 00:42:57 GMT, Joe Wang wrote: >> Update module summary, add a few existing properties and features into the >> tables. > > Joe Wang has updated the pull request incrementally with one additional > commit since the last revision: > > Add legacy property names table Looks fine Joe. Thank you for the additional updates for clarity - Marked as reviewed by lancea (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/3644
Re: Would anyone please reopen JDK-4991002?
Hi Mitsuru,
Thank you for your interest in contributing to the OpenJDK project
If you are not an Author, you can file bugs via
https://bugreport.java.com/bugreport/
To contribute fixes, you will need to sign an Oracle Committer Agreement(OCA).
Please see https://openjdk.java.net/contribute/ for more info
Please enter a new issue which you can use to address your SerialBlob/Clob
fixes. You will also need to add the tests to verify the fix to the relevant
tests in open/test/jdk/javax/sql/testng/test
RowSets, in particular the Serialxxx classes, are seldom used (if at all) as
there are better alternatives for an API.
Once you have signed your OCA and have generated your proposed patch, feel free
to create your PR.
Best
Lance
On May 11, 2021, at 9:11 AM, Mitsuru Kariya
mailto:mitsuru.kar...@oss.nttdata.com>> wrote:
Hi,
While reading the source for SerialBlob, I found that SerialBlob.position()
does not work correctly.
So, I searched JBS and found the description in JDK-4991002 point 1, but it was
closed by Cannot Reproduce.
It can be reproduced easily like below.
SerialBlob sb = new SerialBlob(new byte[]{1, 2, 3, 4, 5});
System.out.println(sb.position(new byte[]{1, 3, 5}, 1));
It should output "-1"(not found) but the current implementation outputs
"3"(found at position 3).
So, would anyone please reopen JDK-4991002 for me?
(I cannot reopen it because I don't have an Author role.)
Furthermore, SerialClob has same problem and JDK-4991036 describes it.
So, may I fix it together?
Or do I need to separate the PR from JDK-4991002?
JDK-4991002:The class javax.sql.rowset.serial.SerialBlob is too buggy
https://bugs.openjdk.java.net/browse/JDK-4991002
JDK-4991036:The class javax.sql.rowset.serial.SerialClob is too buggy
https://bugs.openjdk.java.net/browse/JDK-4991036
Please let me know if there is a better place to do so.
Thanks
[cid:E1C4E2F0-ECD0-4C9D-ADB4-B16CA7BCB7FC@home]
Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com<mailto:lance.ander...@oracle.com>
Re: RFR: 8255035: Update BCEL to Version 6.5.0 [v2]
On Tue, 11 May 2021 16:32:25 GMT, Joe Wang wrote: >> Update BCEL to the latest version (6.5.0). The majority of the changes were >> code refactoring (name format changes). >> >> XML tests passed on both Linux and Windows. > > Joe Wang has updated the pull request incrementally with one additional > commit since the last revision: > > update MD file Marked as reviewed by lancea (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/3893
Re: RFR: 8265448: (zipfs): Reduce read contention in ZipFileSystem [v5]
On Fri, 7 May 2021 03:10:32 GMT, Jason Zaugg wrote: >> If the given Path represents a file, use the overload of read defined >> in FileChannel that accepts an explicit position and avoid serializing >> reads. >> >> Note: The underlying NIO implementation is not required to implement >> FileChannel.read(ByteBuffer, long) concurrently; Windows still appears >> to lock, as it returns true for NativeDispatcher.needsPositionLock. >> >> >> On MacOS X, the enclosed benchmark improves from: >> >> >> BenchmarkMode Cnt Score Error Units >> ZipFileSystemBenchmark.read avgt 10 75.311 ? 3.301 ms/op >> >> >> To: >> >> >> BenchmarkMode Cnt Score Error Units >> ZipFileSystemBenchmark.read avgt 10 12.520 ? 0.875 ms/op > > Jason Zaugg has updated the pull request incrementally with one additional > commit since the last revision: > > [zipfs] Add missing check-failed exception to position/read test > > This appears to have been omitted when this test was added. > To avoid false error reports, the condition must deal with the > edge case of zero-length entries, for which read will return -1. Mach5 jdk-tier1, jdk-tier, jdk-tier3 completed successfully - Marked as reviewed by lancea (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/3853
Re: RFR: 8265448: (zipfs): Reduce read contention in ZipFileSystem [v5]
On Fri, 7 May 2021 03:10:32 GMT, Jason Zaugg wrote: >> If the given Path represents a file, use the overload of read defined >> in FileChannel that accepts an explicit position and avoid serializing >> reads. >> >> Note: The underlying NIO implementation is not required to implement >> FileChannel.read(ByteBuffer, long) concurrently; Windows still appears >> to lock, as it returns true for NativeDispatcher.needsPositionLock. >> >> >> On MacOS X, the enclosed benchmark improves from: >> >> >> BenchmarkMode Cnt Score Error Units >> ZipFileSystemBenchmark.read avgt 10 75.311 ? 3.301 ms/op >> >> >> To: >> >> >> BenchmarkMode Cnt Score Error Units >> ZipFileSystemBenchmark.read avgt 10 12.520 ? 0.875 ms/op > > Jason Zaugg has updated the pull request incrementally with one additional > commit since the last revision: > > [zipfs] Add missing check-failed exception to position/read test > > This appears to have been omitted when this test was added. > To avoid false error reports, the condition must deal with the > edge case of zero-length entries, for which read will return -1. Hi Jason, I have made a pass through your proposed changes and they look OK. I am in the process of running our various Mach5 tiers against your patch to see if any unforeseen issues arise Best Lance - PR: https://git.openjdk.java.net/jdk/pull/3853
Re: RFR: 8255035: Update BCEL to Version 6.5.0
On Thu, 6 May 2021 00:17:40 GMT, Joe Wang wrote: > Update BCEL to the latest version (6.5.0). The majority of the changes were > code refactoring (name format changes). > > XML tests passed on both Linux and Windows. Marked as reviewed by lancea (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/3893
Re: RFR: 8266646: Add more diagnostic to java/lang/System/LoggerFinder/modules [v3]
On Thu, 6 May 2021 16:57:12 GMT, Daniel Fuchs wrote: >> Hi, please find here a trivial test change that adds some diagnostic (time >> stamps) to the LoggerFinder/modules subprocess test logs. > > Daniel Fuchs has updated the pull request incrementally with one additional > commit since the last revision: > > removed unused import statement Marked as reviewed by lancea (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/3904
Integrated: 8266460: java.io tests fail on null stream with upgraded jtreg/TestNG
On Tue, 4 May 2021 20:45:43 GMT, Lance Andersen wrote: > Hi all, > > This fix addresses a change in TestNG behavior for the Before/AfterGroups > annotations that was introduced via > https://github.com/cbeust/testng/pull/2167. The tests have been verified > against the latest TestNG release and continue to run with the current TestNG > release used by jtreg This pull request has now been integrated. Changeset: e8405970 Author:Lance Andersen URL: https://git.openjdk.java.net/jdk/commit/e8405970b9998ff8f77bcf196f1456713a98c47f Stats: 122 lines in 4 files changed: 12 ins; 24 del; 86 mod 8266460: java.io tests fail on null stream with upgraded jtreg/TestNG Reviewed-by: bpb - PR: https://git.openjdk.java.net/jdk/pull/3866
Integrated: 8266579: Update test/jdk/java/lang/ProcessHandle/PermissionTest.java & test/jdk/java/sql/testng/util/TestPolicy.java
On Wed, 5 May 2021 19:10:21 GMT, Lance Andersen wrote:
> Hi all,
>
> Please review this patch which updates
> test/jdk/java/lang/ProcessHandle/PermissionTest.java &
> test/jdk/java/sql/testng/util/TestPolicy.java to include :
>
> `new PropertyPermission("testng.memory.friendly", "read");
> `
> Which will be needed by TestNG 7.4
>
> Best,
> Lance
This pull request has now been integrated.
Changeset: fcedfc8a
Author:Lance Andersen
URL:
https://git.openjdk.java.net/jdk/commit/fcedfc8a3b4299372f195cae036129dcd7b740ea
Stats: 2 lines in 2 files changed: 2 ins; 0 del; 0 mod
8266579: Update test/jdk/java/lang/ProcessHandle/PermissionTest.java &
test/jdk/java/sql/testng/util/TestPolicy.java
Reviewed-by: joehw, naoto, bpb
-
PR: https://git.openjdk.java.net/jdk/pull/3888
RFR: 8266579: Update test/jdk/java/lang/ProcessHandle/PermissionTest.java & test/jdk/java/sql/testng/util/TestPolicy.java
Hi all,
Please review this patch which updates
test/jdk/java/lang/ProcessHandle/PermissionTest.java &
test/jdk/java/sql/testng/util/TestPolicy.java to include :
`new PropertyPermission("testng.memory.friendly", "read");
`
Which will be needed by TestNG 7.4
Best,
Lance
-
Commit messages:
- Add additional permission required by TestNG 7.4
Changes: https://git.openjdk.java.net/jdk/pull/3888/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3888=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8266579
Stats: 2 lines in 2 files changed: 2 ins; 0 del; 0 mod
Patch: https://git.openjdk.java.net/jdk/pull/3888.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/3888/head:pull/3888
PR: https://git.openjdk.java.net/jdk/pull/3888
Re: RFR: 8265448: (zipfs): Reduce read contention in ZipFileSystem [v2]
On Tue, 4 May 2021 14:10:52 GMT, Alan Bateman wrote:
>> Jason Zaugg has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Use pattern matching instanceof
>
> src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java line 2223:
>
>> 2221: synchronized (zfch) {
>> : n = zfch.position(pos).read(bb);
>> 2223: }
>
> @LanceAndersen Are you planning to look at this? Do you mind checking the
> async close case to make sure that the synchronization isn't masking anything?
>
> Also just to point out that pattern matching for instanceof ca be used here.
Yes, I plan to look at this. It would also be good to have a couple of
additional reviews as well :-)
-
PR: https://git.openjdk.java.net/jdk/pull/3853
Re: RFR: 8266460: java.io tests fail on null stream with upgraded jtreg/TestNG
On Tue, 4 May 2021 22:44:18 GMT, Brian Burkhalter wrote: >> That's IntelliJ magic. I can update if you prefer and can let IntelliJ >> optimize all of the imports > > Ah. I was just thinking of consistency with other tests. Up to you. I think your milage will vary depending on the author and the IDE being used. My primary concern was to address the issue for the failing test and Intellij arranged the imports as they are above. I guess I am less concerned about the imports as I think in some cases it comes to personal preference. So I would prefer to leave as is if you are OK :-) - PR: https://git.openjdk.java.net/jdk/pull/3866
Re: RFR: 8266460: java.io tests fail on null stream with upgraded jtreg/TestNG
On Tue, 4 May 2021 22:36:29 GMT, Brian Burkhalter wrote: >> Hi all, >> >> This fix addresses a change in TestNG behavior for the Before/AfterGroups >> annotations that was introduced via >> https://github.com/cbeust/testng/pull/2167. The tests have been verified >> against the latest TestNG release and continue to run with the current >> TestNG release used by jtreg > > test/jdk/java/io/InputStream/NullInputStream.java line 33: > >> 31: import java.io.InputStream; >> 32: >> 33: import static org.testng.Assert.*; > > Would it not be more standard to put the new imports just after this import? > Same comment applies in the other files. That's IntelliJ magic. I can update if you prefer and can let IntelliJ optimize all of the imports - PR: https://git.openjdk.java.net/jdk/pull/3866
RFR: 8266460: java.io tests fail on null stream with upgraded jtreg/TestNG
Hi all, This fix addresses a change in TestNG behavior for the Before/AfterGroups annotations that was introduced via https://github.com/cbeust/testng/pull/2167. The tests have been verified against the latest TestNG release and continue to run with the current TestNG release used by jtreg - Commit messages: - remove trailing space - Updates for TestNG 7.4 Changes: https://git.openjdk.java.net/jdk/pull/3866/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3866=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8266460 Stats: 122 lines in 4 files changed: 12 ins; 24 del; 86 mod Patch: https://git.openjdk.java.net/jdk/pull/3866.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3866/head:pull/3866 PR: https://git.openjdk.java.net/jdk/pull/3866
Re: RFR: 4890732: GZIPOutputStream doesn't support optional GZIP fields [v4]
Hi Lin, Sorry for not replying earlier, I thought I had. I believe we should still flush out the API proposal on the CoreLibs alias before continuing to move forward with updates to the PR (as was suggested by both Alan and I) For example, the updates to the PR does not include any proposed changes to GZIPInputStream and this should be something we should come to an agreement on as it can possibly impact the direction. I am not sure we need to add multiple constructors to GZIPOutputStream as part of the proposed change. It would also be useful to know where is the actual pain point, that is, is there a tool or API not having these fields settable for that is causing an issue? I ask so that we can make sure that we are taking that into consideration. Please note, that I am not trying to discourage your contribution or work to date, I just want to make sure we get agreement on the way forward as it not only impact the PR, but the CSR which will be needed as well. Best Lance On Apr 26, 2021, at 7:40 AM, Lin Zang mailto:lz...@openjdk.java.net>> wrote: On Thu, 8 Apr 2021 08:54:06 GMT, Alan Bateman mailto:al...@openjdk.org>> wrote: Dear All, May I ask your help to review this change? Thanks! BRs, Lin Dear All, May I ask your help to review this change? Thanks! @LanceAndersen Do you have cycles to help Lin? This proposal will require discussion, they may be case for the header to be a record for example. My personal view is that the PR should be set aside until there is at least at least some agreement on the API. Dear @AlanBateman @LanceAndersen, May I ask your help to review whether the usage of Record and Builder pattern is reasonable in the PR? Thanks BRs, Lin - PR: https://git.openjdk.java.net/jdk/pull/3072 [cid:E1C4E2F0-ECD0-4C9D-ADB4-B16CA7BCB7FC@home] Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com<mailto:lance.ander...@oracle.com>
Re: RFR: JDK-8265496: improve null check in DeflaterOutputStream/InflaterInputStream [v2]
On Mon, 26 Apr 2021 02:36:54 GMT, Hamlin Li wrote:
>> code like below will create Deflater before null check, although it's not a
>> real mem leak, but it's better to do null check before new Deflater.
>>
>> try {
>> DeflaterOutputStream dos = new DeflaterOutputStream(null);
>> } catch (NullPointerException e) {
>> passed = true;
>> }
>> Similar issues exist in several other classes.
>
> Hamlin Li has updated the pull request incrementally with one additional
> commit since the last revision:
>
> update copyrights.
Hi Hamlin,
The change looks fine. Please add the noreg-trivial change to the issue given
there is not a test update for this so that you do not get pinged by a bot
-
Marked as reviewed by lancea (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/3681
Re: RFR: 8265248: Implementation Specific Properties: change prefix, plus add existing properties
On Fri, 23 Apr 2021 00:41:17 GMT, Joe Wang wrote: > Update module summary, add a few existing properties and features into the > tables. Good morning Joe, The changes look good based on the discussions we have had about the change and the property documentation is more convenient to find - Marked as reviewed by lancea (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/3644
Re: RFR: 8265746: Update java.time to use instanceof pattern variable (part II) [v2]
On Fri, 23 Apr 2021 15:38:53 GMT, Patrick Concannon wrote: >> Hi, >> >> Could someone please review the second half of my update for the `java.time` >> package to make use of the `instanceof` pattern variable? >> >> This PR was split into two parts due to the large number of files affected. >> >> Kind regards, >> >> Patrick > > Patrick Concannon 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 three additional > commits since the last revision: > > - Removed redundant braces > - Merge remote-tracking branch 'origin/master' into JDK-8265746 > - 8265746: Update java.time to use instanceof pattern variable (part II) Marked as reviewed by lancea (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/3650
Re: RFR: 8265746: Update java.time to use instanceof pattern variable (part II)
On Fri, 23 Apr 2021 10:44:30 GMT, Patrick Concannon wrote: > Hi, > > Could someone please review the second half of my update for the `java.time` > package to make use of the `instanceof` pattern variable? > > This PR was split into two parts due to the large number of files affected. > > Kind regards, > > Patrick Looks good. Thank you for the updates - Marked as reviewed by lancea (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/3650
Integrated: 8265019 : Update tests for additional TestNG test permissions
On Tue, 13 Apr 2021 18:01:49 GMT, Lance Andersen wrote: > Hi all, > > Please review the following patch which adds additional permissions needed > for when JTREG upgrades to a newer version of TestNG. > > Best, > Lance This pull request has now been integrated. Changeset: ffb37718 Author:Lance Andersen URL: https://git.openjdk.java.net/jdk/commit/ffb37718 Stats: 22 lines in 2 files changed: 16 ins; 4 del; 2 mod 8265019: Update tests for additional TestNG test permissions Reviewed-by: naoto, bpb, alanb - PR: https://git.openjdk.java.net/jdk/pull/3471
Re: RFR: 8262883: doccheck: Broken links in java.base
On Wed, 14 Apr 2021 14:03:01 GMT, Michael McMahon wrote: > Hi, > > Could I get the following trivial doc changes reviewed please, caused by: > > - broken tags in MethodHandles referring to package.html instead of > package-summary.html > > - references to a package level #unixdomain anchor that no longer exists. > > - a tag missing a "../" in SocketChannel > > Thanks, > > Michael Marked as reviewed by lancea (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/3491
Re: RFR: 8265019 : Update tests for additional TestNG test permissions [v3]
> Hi all, > > Please review the following patch which adds additional permissions needed > for when JTREG upgrades to a newer version of TestNG. > > Best, > Lance Lance Andersen has updated the pull request incrementally with one additional commit since the last revision: Add back individual security imports - Changes: - all: https://git.openjdk.java.net/jdk/pull/3471/files - new: https://git.openjdk.java.net/jdk/pull/3471/files/20ef2bd0..31edda8e Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3471=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3471=01-02 Stats: 8 lines in 1 file changed: 7 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/3471.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3471/head:pull/3471 PR: https://git.openjdk.java.net/jdk/pull/3471
Re: RFR: 8265019 : Update tests for additional TestNG test permissions [v2]
On Apr 14, 2021, at 2:13 AM, Alan Bateman mailto:al...@openjdk.java.net>> wrote: On Tue, 13 Apr 2021 18:56:22 GMT, Lance Andersen mailto:lan...@openjdk.org>> wrote: Hi all, Please review the following patch which adds additional permissions needed for when JTREG upgrades to a newer version of TestNG. Best, Lance Lance Andersen has updated the pull request incrementally with one additional commit since the last revision: TestNG updates Permission re-organization Marked as reviewed by alanb (Reviewer). test/jdk/java/sql/testng/util/TestPolicy.java line 27: 25: import java.io.FilePermission; 26: import java.lang.reflect.ReflectPermission; 27: import java.security.*; My guess is that you didn't mean the change the import. Intellij did that automagically. I just reverted the change for consistency - PR: https://git.openjdk.java.net/jdk/pull/3471 [cid:E1C4E2F0-ECD0-4C9D-ADB4-B16CA7BCB7FC@home] Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com<mailto:lance.ander...@oracle.com>
Re: RFR: 8265019 : Update tests for additional TestNG test permissions [v2]
> Hi all, > > Please review the following patch which adds additional permissions needed > for when JTREG upgrades to a newer version of TestNG. > > Best, > Lance Lance Andersen has updated the pull request incrementally with one additional commit since the last revision: TestNG updates Permission re-organization - Changes: - all: https://git.openjdk.java.net/jdk/pull/3471/files - new: https://git.openjdk.java.net/jdk/pull/3471/files/0183f88b..20ef2bd0 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3471=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3471=00-01 Stats: 20 lines in 2 files changed: 10 ins; 10 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/3471.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3471/head:pull/3471 PR: https://git.openjdk.java.net/jdk/pull/3471
RFR: 8265019 : Update tests for additional TestNG test permissions
Hi all, Please review the following patch which adds additional permissions needed for when JTREG upgrades to a newer version of TestNG. Best, Lance - Commit messages: - TestNG updates Changes: https://git.openjdk.java.net/jdk/pull/3471/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3471=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8265019 Stats: 21 lines in 2 files changed: 11 ins; 6 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/3471.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3471/head:pull/3471 PR: https://git.openjdk.java.net/jdk/pull/3471
Re: RFR: 4890732: GZIPOutputStream doesn't support optional GZIP fields [v3]
Hi Lin On Apr 10, 2021, at 11:16 PM, Lin Zang mailto:lz...@openjdk.java.net>> wrote: Dear @AlanBateman and @LanceAndersen, Thanks a lot for your review and comments! We should look to see if it makes sense to use some of the more recent java features such as Record. If we are adding a specific class to hold the header fields, perhaps using the builder pattern should be considered vs constructors. I agree, we should finalize the API before moving forward. I am not quite fimilar with Record, I will do some investigation, trying to use it in this PR and discuss with you later. Please look at: The Jep https://openjdk.java.net/jeps/395 A simple tutorial https://oracle.github.io/learning-library/oci-library/oci-hol/oci-java-app/workshops/freetier/?customTrackingParam=:ow:lp:pt:::RC_WWMK200728P9:OCIJava_HOL=7e168170255b485a917a5e6602097868=100601=2=:ow:lp:cpo:::RC_WWMK200728P00012:LPD400088742+:ow:lp:cpo::=lab-6-records#STEP1:AsimpleRecord Chris and Julia’s video is also very good: https://www.youtube.com/watch?v=pxiHwrEMEtI Between the use of Record and/or the Builder pattern, assembling the header values can be done quite nicely without constructor overload. If we are writing out these additional fields, what should we do with them when reading a gzip file back? IMO, generally there should be check of header crc, and some other checks like the header format, magic number etc. May be we could implement it like the gnu gzip tool, which ingore contents of most header fields but verified the general header info. Sorry, that what not my point Your current proposal provides no wayto access these fields similar to for example ZipEntry. If we are going to set these additional fields then we should we should provided a means to access the fields Have you looked at other gzip api implementations to see what they provide in this area? Please look at api’s such as commons-compress. They provide access to these fields via the API in addition to being able to set the fields. This is what I was referring to above. I have looked at the gzip implementation at https://github.com/linzang/gzip/blob/distrotech-gzip/gzip.c#L1281, this method is use to generate header crc value for check. And there is some legal header check in this method. And `file name` is used to save original file name in gzip when it is truncated. refer to https://github.com/linzang/gzip/blob/distrotech-gzip/zip.c#L37 In JDK, there is a usage of `file comment` in the gzip heap dump file generated by jcmd `jmap -dump` command. at https://github.com/openjdk/jdk/blob/ff52f2989fd60ec8251eaf76f4c4b78f10d3e048/src/hotspot/share/services/heapDumperCompression.cpp#L127 , and this info is used in testing like an hint for uncompression, please refer https://github.com/openjdk/jdk/blob/ff52f2989fd60ec8251eaf76f4c4b78f10d3e048/test/lib/jdk/test/lib/hprof/parser/GzipRandomAccess.java#L280. And IMHO the behavior of adding information in `file comments` seems like a general way to transfer extra information between compressor and uncompressor. Is there anyone who relies on this additional header info? As I mentioned in an earlier comment, we should validate the changes against another implementation to ensure that we can read the data back correctly and that the additional header data that we write can be read by other tools. May be we could have similar behavior? which just do some general header info check, calculate CRC of the header and ignore the real contents of most header fields. Do you think it is reasonable? Adding additional support for these fields is fine, we just need to agree on the API changes to move forward prior to updating the PR Thanks! Lin - PR: https://git.openjdk.java.net/jdk/pull/3072 [cid:E1C4E2F0-ECD0-4C9D-ADB4-B16CA7BCB7FC@home] Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com<mailto:lance.ander...@oracle.com>
Re: RFR: 4890732: GZIPOutputStream doesn't support optional GZIP fields [v3]
Hi Lin, First thank you for your efforts on this feature. As Alan suggested on March 26, we should take a step back and flush out the overall changes to the API before moving forward with additional PR reviews. We should look to see if it makes sense to use some of the more recent java features such as Record. If we are adding a specific class to hold the header fields, perhaps using the builder pattern should be considered vs constructors. If we are writing out these additional fields, what should we do with them when reading a gzip file back? Have you looked at other gzip api implementations to see what they provide in this area? Is there anyone who relies on this additional header info? As I mentioned in an earlier comment, we should validate the changes against another implementation to ensure that we can read the data back correctly and that the additional header data that we write can be read by other tools. Once we reach a consensus on the overall API changes and/or additions, then we can look to move forward with the next PR review. Alan, Yes I am happy to help Lin on moving this forward. > On Apr 8, 2021, at 4:32 AM, Lin Zang wrote: > > On Fri, 2 Apr 2021 08:53:20 GMT, Lin Zang <mailto:lz...@openjdk.org>> wrote: > >>> Hi Lin, >>> >>> On Mar 24, 2021, at 2:51 AM, Lin Zang ***@***.**@***.***>> wrote: >>> >>> >>> >>> Hi Lance， >>> Thanks a lot for your review. I will update the PR ASAP. >>> May I ask your help to also review the CSR？ >>> >>> I believe we need to flush out some of the issues I raised that were not >>> test related as they will result in updates to the javadoc which will >>> require an update to the CSR. >>> >>> >>> >>> Thanks! >>> >>> BRs, >>> Lin >>> >>> — >>> You are receiving this because you commented. >>> Reply to this email directly, view it on >>> GitHub<https://github.com/openjdk/jdk/pull/3072#issuecomment-805549600>, or >>> unsubscribe<https://github.com/notifications/unsubscribe-auth/ABQPFO5R4INTAUFZ3QS6WL3TFGDXHANCNFSM4ZMLU6SA>. >>> >>> ***@***.*** >>> >>> >>> >>> Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 >>> Oracle Java Engineering >>> 1 Network Drive >>> Burlington, MA 01803 >>> ***@***.**@***.***> >> >> Dear @LanceAndersen, >> Here I added a class `GZIPHeaderData` which is defined to hold all gzip >> header members, and make it an argument of the GZIPOutputStream constructor. >> Would you like to help review and check whether this change looks >> reasonable? >> >> BTW, this pr still lack of the negative test cases, I plan to add it when >> the constructor API is fixed. >> >> Thanks! >> Lin > > Dear All, > May I ask your help to review this change? Thanks! > > BRs, > Lin > > - > > PR: https://git.openjdk.java.net/jdk/pull/3072 > <https://git.openjdk.java.net/jdk/pull/3072>
Re: RFR: 8173970: jar tool should have a way to extract to a directory [v3]
On Mon, 29 Mar 2021 14:04:10 GMT, Jaikiran Pai wrote:
>> Can I please get a review for this patch which proposes to implement the
>> enhancement request noted in
>> https://bugs.openjdk.java.net/browse/JDK-8173970?
>>
>> The commit in this PR introduces the `-o` and `--output-dir` option to the
>> `jar` command. The option takes a path to a destination directory as a value
>> and extracts the contents of the jar into that directory. This is an
>> optional option and the changes in the commit continue to maintain backward
>> compatibility where the jar is extracted into current directory, if no `-o`
>> or `--output-dir` option has been specified.
>>
>> As far as I know, there hasn't been any discussion on what the name of this
>> new option should be. I was initially thinking of using `-d` but that is
>> currently used by the `jar` command for a different purpose. So I decided to
>> use `-o` and `--output-dir`. This is of course open for change depending on
>> any suggestions in this PR.
>>
>> The commit in this PR also updates the `jar.properties` file which contains
>> the English version of the jar command's `--help` output. However, no
>> changes have been done to the internationalization files corresponding to
>> this one (for example: `jar_de.properties`), because I don't know what
>> process needs to be followed to have those files updated (if at all they
>> need to be updated).
>>
>> The commit also includes a jtreg testcase which verifies the usage of this
>> new option.
>
> Jaikiran Pai has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Alan's review feedback for -C help text
> - Keep -xfP backward compatible but don't allow -C/--dir with -xfP
Some additional comments basically suggesting that we test --extract in
addition to -x
test/jdk/tools/jar/JarExtractTest.java line 175:
> 173: final String dest = "foo-bar";
> 174: System.out.println("Extracting " + testJarPath + " to " + dest);
> 175: final int exitCode = JAR_TOOL.run(System.out, System.err, "-x",
> "-f", testJarPath.toString(),
Perhaps add a DataProvider so you can test --extract as well?
test/jdk/tools/jar/JarExtractTest.java line 216:
> 214: final Path jarPath = createJarWithPFlagSemantics();
> 215: // extract with -P flag without any explicit destination
> directory (expect the extraction to work fine)
> 216: final String[] args = new String[]{"-xvfP", jarPath.toString()};
Perhaps add a DataProvider so you can test --extract as well?
test/jdk/tools/jar/JarExtractTest.java line 239:
> 237: */
> 238: @Test
> 239: public void testExtractWithDirPFlagNotAllowed() throws Exception {
I would include --extract in your testing options
test/jdk/tools/jar/JarExtractTest.java line 240:
> 238: @Test
> 239: public void testExtractWithDirPFlagNotAllowed() throws Exception {
> 240: final String expectedErrMsg = "-P option cannot be used when
> extracting a jar to a specific location";
Probably need to add a comment that this must match the entry in jar.properties
test/jdk/tools/jar/JarExtractTest.java line 247:
> 245: cmdArgs.add(new String[]{"-x", "-f", testJarPath.toString(),
> "-P", "-C", "."});
> 246: cmdArgs.add(new String[]{"-x", "-f", testJarPath.toString(),
> "-P", "--dir", "."});
> 247: cmdArgs.add(new String[]{"-xvfP", testJarPath.toString(), "-C",
> tmpDir});
Perhaps add a DataProvider so you can test --extract as well?
test/jdk/tools/jar/JarExtractTest.java line 262:
> 260: */
> 261: @Test
> 262: public void testLegacyCompatibilityMode() throws Exception {
Perhaps add a DataProvider so you can test --extract as well?
test/jdk/tools/jar/JarExtractTest.java line 282:
> 280: cmdArgs.add(new String[]{"-x", "-f", testJarPath.toString(),
> "-C", tmpDir, "-C", tmpDir});
> 281: cmdArgs.add(new String[]{"-x", "-f", testJarPath.toString(),
> "--dir", tmpDir, "--dir", tmpDir});
> 282: cmdArgs.add(new String[]{"-x", "-f", testJarPath.toString(),
> "--dir", tmpDir, "-C", tmpDir});
Perhaps use a DataProvider so you can test --extract as well?
test/jdk/tools/jar/JarExtractTest.java line 300:
> 298: public void testExtractPartialContent() throws Exception {
> 299: final String tmpDir = Files.createTempDirectory(Path.of("."),
> "8173970-").toString();
> 300: final String[] cmdArgs = new String[]{"-x", "-f",
> testJarPath.toString(), "--dir", tmpDir,
Perhaps add a DataProvider so you can test --extract as well?
test/jdk/tools/jar/JarExtractTest.java line 332:
> 330: */
> 331: private void testExtract(final String dest) throws Exception {
> 332: final String[] args = new String[]{"-x", "-f",
> testJarPath.toString(), "-C", dest};
Perhaps add a DataProvider so you can test --extract as well?
-
PR: https://git.openjdk.java.net/jdk/pull/2752
Re: RFR: 8173970: jar tool should have a way to extract to a directory [v3]
On Mon, 29 Mar 2021 14:04:10 GMT, Jaikiran Pai wrote:
>> Can I please get a review for this patch which proposes to implement the
>> enhancement request noted in
>> https://bugs.openjdk.java.net/browse/JDK-8173970?
>>
>> The commit in this PR introduces the `-o` and `--output-dir` option to the
>> `jar` command. The option takes a path to a destination directory as a value
>> and extracts the contents of the jar into that directory. This is an
>> optional option and the changes in the commit continue to maintain backward
>> compatibility where the jar is extracted into current directory, if no `-o`
>> or `--output-dir` option has been specified.
>>
>> As far as I know, there hasn't been any discussion on what the name of this
>> new option should be. I was initially thinking of using `-d` but that is
>> currently used by the `jar` command for a different purpose. So I decided to
>> use `-o` and `--output-dir`. This is of course open for change depending on
>> any suggestions in this PR.
>>
>> The commit in this PR also updates the `jar.properties` file which contains
>> the English version of the jar command's `--help` output. However, no
>> changes have been done to the internationalization files corresponding to
>> this one (for example: `jar_de.properties`), because I don't know what
>> process needs to be followed to have those files updated (if at all they
>> need to be updated).
>>
>> The commit also includes a jtreg testcase which verifies the usage of this
>> new option.
>
> Jaikiran Pai has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Alan's review feedback for -C help text
> - Keep -xfP backward compatible but don't allow -C/--dir with -xfP
Hi Jaikiran
Overall this looks good. I have a few comments below and will look at the CSR
shortly.
src/jdk.jartool/share/classes/sun/tools/jar/Main.java line 1427:
> 1425: return rc;// leading '/' or 'dot-dot' only path
> 1426: }
> 1427: File f = new File(xdestDir, name.replace('/',
> File.separatorChar));
Could you please add a comment regarding xdestDir and also correct the typo on
line 1418 'requres' -> 'requires'
src/jdk.jartool/share/classes/sun/tools/jar/resources/jar.properties line 62:
> 60: Could not create a temporary file
> 61: error.extract.multiple.dest.dir=\
> 62: You may not specify more than one directory for extracting the jar
Perhaps something similar to:
You may not specify the '-C' or '--dir' option more than once with the '-x'
option
src/jdk.jartool/share/classes/sun/tools/jar/resources/jar.properties line 64:
> 62: You may not specify more than one directory for extracting the jar
> 63: error.extract.pflag.not.allowed=\
> 64: -P option cannot be used when extracting a jar to a specific
> location
Perhaps something similar to
You may not specify '-Px' with the '-C' or '--dir' options
src/jdk.jartool/share/classes/sun/tools/jar/resources/jar.properties line 167:
> 165: (in = {0}) (out= {1})
> 166: out.extract.dir=\
> 167: extracting to {0}
Perhaps 'Extracting to directory: {0}'
src/jdk.jartool/share/classes/sun/tools/jar/resources/jar.properties line 249:
> 247: \ -C DIR Change to the specified directory and
> include the\n\
> 248: \ following file. When used in extract mode,
> extracts\n\
> 249: \ the jar to the specified directory
Should this be updated on line 187 as well in the compatibility mode section?
test/jdk/tools/jar/JarExtractTest.java line 152:
> 150: return abs;
> 151: }
> 152:
Please add a comment to each test giving a high level overview of what it does
as it will help future maintainers
test/jdk/tools/jar/JarExtractTest.java line 307:
> 305: // make sure only the specific files were extracted
> 306: final Stream paths = Files.walk(Path.of(tmpDir));
> 307: Assert.assertEquals(paths.count(), 6, "Unexpected number of
> files/dirs in " + tmpDir);
Should '6' be in a local variable to make it clearer or at a minimum a comment
test/jdk/tools/jar/JarExtractTest.java line 367:
> 365: }
> 366:
> 367: private static Path createJarWithPFlagSemantics() throws IOException
> {
Perhaps add a comment as to what the method does
-
PR: https://git.openjdk.java.net/jdk/pull/2752
Re: Proposal for Decimal64 and Decimal128 value-based classes
Agree this could be beneficial to JDBC users. To officially support this in JDBC would require an MR but as Douglas indicates the work required to the JDBC spec would be minimal Best Lance On Mar 31, 2021, at 10:23 AM, Douglas Surber mailto:douglas.sur...@oracle.com>> wrote: +1 JDBC would support this immediately. All it would take is the addition of a couple of lines in some appendices to require that conforming implementations of getObject(int, Class), setObject(int, Object, SQLType), etc support Decimal64 and Decimal128. No change to the API required. Driver vendors could add this support the instant the types are available, no need to wait for a change in the JDBC spec. This would be a huge win for many Java apps. A large fraction of Java apps deal with money in some form. Binary floats are inappropriate for money and BigDecimal is too big and too slow. Rather than waiting on Valhala I would prefer that this project be fast tracked and added to OpenJDK ASAP. Thanks for doing this. Douglas On Mar 30, 2021, at 10:10 PM, core-libs-dev-requ...@openjdk.java.net<mailto:core-libs-dev-requ...@openjdk.java.net><mailto:core-libs-dev-requ...@openjdk.java.net> wrote: Date: Tue, 30 Mar 2021 22:12:32 -0400 From: Brian Goetz mailto:brian.go...@oracle.com><mailto:brian.go...@oracle.com>> To: Raffaello Giulietti mailto:raffaello.giulie...@gmail.com><mailto:raffaello.giulie...@gmail.com>>, Paul Sandoz mailto:paul.san...@oracle.com><mailto:paul.san...@oracle.com>> Cc: core-libs-dev mailto:core-libs-dev@openjdk.java.net><mailto:core-libs-dev@openjdk.java.net>> Subject: Re: Proposal for Decimal64 and Decimal128 value-based classes Message-ID: <64334a24-0e4c-57b8-b666-447ca3508...@oracle.com<mailto:64334a24-0e4c-57b8-b666-447ca3508...@oracle.com><mailto:64334a24-0e4c-57b8-b666-447ca3508...@oracle.com>> Content-Type: text/plain; charset=utf-8; format=flowed They'll find a natural home in JDBC, since SQL has a native decimal type. On 3/30/2021 7:05 PM, Raffaello Giulietti wrote: As far as I can tell, scientific computation will make use of binary floating point numbers for a long time. Decimal floating point numbers are still limited to biz and fin applications. [cid:E1C4E2F0-ECD0-4C9D-ADB4-B16CA7BCB7FC@home] Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com<mailto:lance.ander...@oracle.com>
Re: RFR: 4890732: GZIPOutputStream doesn't support, in fact thwarts, use of optional GZIP fields [v3]
On Mar 27, 2021, at 12:44 PM, Alan Bateman mailto:al...@openjdk.java.net>> wrote: Dear Lance， Sorry that I reply so late, I got stuck at some work recently. * We should have tests that include some , but not all of the additional fields as I believe they are all optional according to the RFC. I see Joe's comments in the CSR about the encode of the byte array of String-alike field such file name, I checked that the RFC has description it is "ISO8859-1". So do you think it is ok to change the argument type to String and add the encoding decription in the documented comments? And Joe also suggested to make all optional header field as a class (I propose to name it "gzipHeaderMetaRecord" ). And then the constructor could accept it and process related flag and fields. Moreover it seems this class cloud also be used in GzipInputStream，Do you think it is ok to also change it here? Thanks! BRs, Lin GZIPInputStream/GZIPOutputStream are JDK 1.1 era classes that aren't well specified and I think we'll have to do some improvements as part of extending the support. For example, the GZIPOutputStream constructors (existing and proposed) do not specify that they write the member header. That is a reasonable suggestion to indicate what is and is not done. As regards the new constructors then I think new parameters will need to be clearly specified and/or linked to their description in the RFC. The types of some of the parameters may need to be re-examined, maybe the file name and comment should be provided as Strings (that will help with the discussion about the encoding to 8859_1). Agree and that will also align it with ZipEntry(String), Zipentry::setComment I think the javadoc will need to make it clear on what flags/values are allowed and the behavior when other values are used. The Gzip spec is kind of vague in this area for example with the SI1 and SI2 for the Extra Field only one value set is specified but I can imagine additional values being somewhere. It might be that the class will need enums and other classes to provide a better API. I am not sure how much if at all these extra header fields are used. Certainly if the API now sets them, we should probably provide a way to access them as well. - PR: https://git.openjdk.java.net/jdk/pull/3072 [cid:E1C4E2F0-ECD0-4C9D-ADB4-B16CA7BCB7FC@home] Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com<mailto:lance.ander...@oracle.com>
Re: RFR: 8173970: jar tool should have a way to extract to a directory
th the extraction logic itself. Specifically, when the explicitly specified or the default current directory already has directories/files that belong to the jar being extracted, those files/dirs will continue to be overwritten. Compatibility mode: The code in this PR also supports the compatibility mode for this option. Specifically, a command like: jar -xvf somejar.jar -C /tmp/foo/ will work fine and the jar will be extracted into /tmp/foo directory. Message/error internationalization: I have only updated the jar.properties for the English version of the new output and error messages. I don't know what the process is for adding this to other languages, if at all that needs to be done in this PR. jar --help output: Currently the jar --help output only talks about creation and updation of the jar. There's no mention of using the tool for extracting the jar content: "jar creates an archive for classes and resources, and can manipulate or restore individual classes or resources from an archive." It does mention "manipulate" but doesn't specifically say extraction. The examples in the help command output don't have any examples for extraction. Should we add an example for extracting the jar file, in this help output? Testing: A new jtreg test has been introduced which tests various aspects of this feature. It runs most of those tests against both absolute and relative paths. A couple of tests in the new introduced test case, check for the output/error messages. The jar tool uses resource bundles to print out these messages. I need input on whether I should enforce a specific locale to run these tests so that I can compare the error/output messages for expected strings? See testExtractFailWithMultipleDir() or testHelpOutput() for what I mean. Man page: This one I need input on. I have tried to see how these man pages are generated and from what I can understand it looks like these man pages are autogenerated during the build process using pandoc. Is that right? The hints that I see in the Docs.gmk seems to suggest that there are some markdown source files from which these man pages get generated. However, I can't seem to locate any such markdown files for this or other tools, from which the man pages get generated. Any help on how I should go about editing/updating the man page for the jar tool? Example usage: Here are some example usages: jar -x -f somejar.jar -C /tmp/foo/bar/ This command extracts the somejar.jar file to the /tmp/foo/bar/ directory, creating it if necessary. jar -x -f somejar.jar --dir /tmp/foo/bar/ Same as above, except uses the long form --dir option jar -x -f somejar.jar -C /tmp/foo/bar/ f1.txt d1/f2.txt Assuming somejar.jar contains "f1.txt" (at root), "d1/f2.txt" and other files, then the above command extracts only "f1.txt" and "d1/f2.txt" into the /tmp/foo/bar/ directory. -Jaikiran On 14/03/21 6:21 pm, Alan Bateman wrote: On 12/03/2021 12:18, Lance Andersen wrote: : I don’t have a strong preference but lean slightly towards ‘-directory’ as it is more descriptive, similar to the other GNU-style commands jar currently supports . Tar supports ‘—cd’, ‘—directory’ in addition to ‘-C’ which is why I suggested supporting both GNU-style long options. Perhaps jpackage should also support —dir/directory in addition to ‘—dest' if we are looking at consistency between java tools. I do agree that it would be nice to be consistent across the java tools for options so if we go the ‘-directory’, we should follow your suggestion and make it the primary and remove ‘—dir’ from the usage output. My comment on consistency was limited to the long option to specify the directory when extracting, didn't mean to suggest doing anything with the other tools that specify an output/destination directory. In any case, I think we have enough to make progress on this issue now. -Alan [cid:E1C4E2F0-ECD0-4C9D-ADB4-B16CA7BCB7FC@home] Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com<mailto:lance.ander...@oracle.com>
Re: RFR: 8173970: jar tool should have a way to extract to a directory
On Mar 27, 2021, at 12:05 PM, Alan Bateman mailto:al...@openjdk.java.net>> wrote: On Fri, 26 Feb 2021 17:03:11 GMT, Jaikiran Pai mailto:j...@openjdk.org>> wrote: Can I please get a review for this patch which proposes to implement the enhancement request noted in https://bugs.openjdk.java.net/browse/JDK-8173970? The commit in this PR introduces the `-o` and `--output-dir` option to the `jar` command. The option takes a path to a destination directory as a value and extracts the contents of the jar into that directory. This is an optional option and the changes in the commit continue to maintain backward compatibility where the jar is extracted into current directory, if no `-o` or `--output-dir` option has been specified. As far as I know, there hasn't been any discussion on what the name of this new option should be. I was initially thinking of using `-d` but that is currently used by the `jar` command for a different purpose. So I decided to use `-o` and `--output-dir`. This is of course open for change depending on any suggestions in this PR. The commit in this PR also updates the `jar.properties` file which contains the English version of the jar command's `--help` output. However, no changes have been done to the internationalization files corresponding to this one (for example: `jar_de.properties`), because I don't know what process needs to be followed to have those files updated (if at all they need to be updated). The commit also includes a jtreg testcase which verifies the usage of this new option. I think the summary is that we've converged on -C/--dir. We might have to tweak the usage message for -C so that it starts with the existing "Change to the specified directory ..." rather than changing it to start with the extract case. Are you, or Lance, going to create the CSR for this? I have not had a chance to go through the latest update from Jaikiran or his revised PR yet. Yes, once we flush everything out, I will work with Jaikiran on the CSR and determine which of us will create the CSR. On the todo list for next week. Best Lance - PR: https://git.openjdk.java.net/jdk/pull/2752 [cid:E1C4E2F0-ECD0-4C9D-ADB4-B16CA7BCB7FC@home] Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com<mailto:lance.ander...@oracle.com>
Re: RFR: 8264031: Fix typo in ZipFileSystem.deleteFile ZipException
On Tue, 16 Feb 2021 13:48:20 GMT, Simon Legner wrote: > 8264031: Fix typo in ZipFileSystem.deleteFile ZipException Marked as reviewed by lancea (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2588
Re: RFR: 8263668: Update java.time to use instanceof pattern variable
On Wed, 24 Mar 2021 09:56:16 GMT, Patrick Concannon wrote: > Hi, > > Could someone please review my code for updating the code in the `java.time` > package to make use of the `instanceof` pattern variable? > > Kind regards, > Patrick Changes look good Patrick. - Marked as reviewed by lancea (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/3170
Re: RFR: 4890732: GZIPOutputStream doesn't support, in fact thwarts, use of optional GZIP fields [v3]
On Wed, 24 Mar 2021 06:51:16 GMT, Lin Zang wrote: >> Hi Lin, >> >> Again, thank you for taking on this 19+ year feature request. >> >> I have not done a deep dive on the CSR, but wanted to get a few comments >> back to you on a quick scan of the last PR update. >> >> Personally, I would like to see more testing for a change such as this given >> the age of the code: >> >> - Please do not modify the existing test, you can either create a new >> test(s) or add tests to the existing test class >> - We should capture Gzip files with these headers set from other tools and >> store the Gzip in an array within the test which can then be written to disk >> so the tests can validate interoperability. Please see some of the other >> Zip tests for an example >> - We should have tests that include some , but not all of the additional >> fields as I believe they are all optional according to the RFC. >> - Please include some negative tests >> >> >> I have also include some additional comments within the code > > Hi Lance， > Thanks a lot for your review. I will update the PR ASAP. > May I ask your help to also review the CSR？ Thanks! > > BRs, > Lin Hi Lin, On Mar 24, 2021, at 2:51 AM, Lin Zang ***@***.**@***.***>> wrote: Hi Lance， Thanks a lot for your review. I will update the PR ASAP. May I ask your help to also review the CSR？ I believe we need to flush out some of the issues I raised that were not test related as they will result in updates to the javadoc which will require an update to the CSR. Thanks! BRs, Lin — You are receiving this because you commented. Reply to this email directly, view it on GitHub<https://github.com/openjdk/jdk/pull/3072#issuecomment-805549600>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ABQPFO5R4INTAUFZ3QS6WL3TFGDXHANCNFSM4ZMLU6SA>. ***@***.*** Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 ***@***.**@***.***> - PR: https://git.openjdk.java.net/jdk/pull/3072
Re: RFR: 8264091: Use the blessed modifier order in java.logging
On Tue, 23 Mar 2021 21:41:32 GMT, Alex Blewitt wrote: > 8264091: Use the blessed modifier order in java.logging Marked as reviewed by lancea (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/3163
Re: RFR: 4890732: GZIPOutputStream doesn't support, in fact thwarts, use of optional GZIP fields [v3]
On Fri, 19 Mar 2021 08:19:58 GMT, Lin Zang wrote:
>> 4890732: GZIPOutputStream doesn't support, in fact thwarts, use of optional
>> GZIP fields
>
> Lin Zang has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - update copyright
> - reuse arguments constructor for non-argument one.
Hi Lin,
Again, thank you for taking on this 19+ year feature request.
I have not done a deep dive on the CSR, but wanted to get a few comments back
to you on a quick scan of the last PR update.
Personally, I would like to see more testing for a change such as this given
the age of the code:
- Please do not modify the existing test, you can either create a new test(s)
or add tests to the existing test class
- We should capture Gzip files with these headers set from other tools and
store the Gzip in an array within the test which can then be written to disk so
the tests can validate interoperability. Please see some of the other Zip
tests for an example
- We should have tests that include some , but not all of the additional fields
as I believe they are all optional according to the RFC.
- Please include some negative tests
I have also include some additional comments within the code
src/java.base/share/classes/java/util/zip/GZIPOutputStream.java line 193:
> 191: * @param generateHeaderCRC
> 192: *if {@code true} the header will include the CRC16 of the
> header.
> 193: * @param extraFieldBytes the byte array of extra filed,
filed -> field
src/java.base/share/classes/java/util/zip/GZIPOutputStream.java line 195:
> 193: * @param extraFieldBytes the byte array of extra filed,
> 194: *the generated header would calculate the
> byte[] size
> 195: *and fill it before the byte[] in header.
This is not clear what you are trying to articulate here
src/java.base/share/classes/java/util/zip/GZIPOutputStream.java line 269:
> 267: * @param generateHeaderCRC
> 268: *if {@code true} the header will include the CRC16 of the
> header.
> 269: * @param extraFieldBytes the byte array of extra filed,
filed -> field
src/java.base/share/classes/java/util/zip/GZIPOutputStream.java line 320:
> 318: * +---+---+=+
> 319: */
> 320: int xlen = extraFieldBytes.length;
On a quick look at the RFC, I noticed the following:
2.3.1.1. Extra field
If the FLG.FEXTRA bit is set, an "extra field" is present in
the header, with total length XLEN bytes. It consists of a
series of subfields, each of the form:
+---+---+---+---+==+
|SI1|SI2| LEN |... LEN bytes of subfield data ...|
+---+---+---+---+==+
SI1 and SI2 provide a subfield ID, typically two ASCII letters
with some mnemonic value. Jean-Loup Gailly
is maintaining a registry of subfield
IDs; please send him any subfield ID you wish to use. Subfield
IDs with SI2 = 0 are reserved for future use. The following
IDs are currently defined:
---
This does not seem to be accounted for or is it?
src/java.base/share/classes/java/util/zip/GZIPOutputStream.java line 323:
> 321: if (xlen > 0x) {
> 322: throw new ZipException("extra field size out of range");
> 323: }
Where is the ZipException documented that is being thrown?
src/java.base/share/classes/java/util/zip/GZIPOutputStream.java line 342:
> 340: *+=+
> 341: */
> 342: out.write(filename);
The RFC indicates:
If FNAME is set, an original file name is present,
terminated by a zero byte. The name must consist of ISO
8859-1 (LATIN-1) characters; on operating systems using
EBCDIC or any other character set for file names, the name
must be translated to the ISO LATIN-1 character set. This
is the original name of the file being compressed, with any
directory components removed, and, if the file being
compressed is on a file system with case insensitive names,
forced to lower case. There is no original file name if the
data was compressed from a source other than a named file;
for example, if the source was stdin on a Unix system, there
is no file name.
It looks like there is no validation being done so I am not sure what the
expectation is.
src/java.base/share/classes/java/util/zip/GZIPOutputStream.java line 357:
> 355: */
> 356: out.write(fileComment);
> 357: out.write(0);
The RFC states:
If FCOMMENT is set, a zero-terminated file comment is
Re: RFR: 8263885: Use the blessed modifier order in java.sql/rowset/transation.xa
On Fri, 19 Mar 2021 15:27:00 GMT, Alex Blewitt wrote: > Fixes for the `java.sql`, `java.sql.rowset` and `java.transaction.xa` > packages. > > @cl4es would you mind creating a subtask of JDK-8263854 so I can update the > PR title? Marked as reviewed by lancea (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/3090
Re: RFR: 4890732: GZIPOutputStream doesn't support, in fact thwarts, use of optional GZIP fields
Hi Lin, Thank you for your contribution. I have no looked at this in any detail. A few general comments: * This will require a CSR * Please update your PR to include test coverage demonstrating that the data can be written and then read back Best, Lance On Mar 18, 2021, at 6:57 AM, Lin Zang mailto:lz...@openjdk.java.net>> wrote: 4890732: GZIPOutputStream doesn't support, in fact thwarts, use of optional GZIP fields - Commit messages: - remove trailing spaces - 4890732: support optional GZIP fields in GZIPOutputStream Changes: https://git.openjdk.java.net/jdk/pull/3072/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3072=00 Issue: https://bugs.openjdk.java.net/browse/JDK-4890732 Stats: 184 lines in 1 file changed: 184 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/3072.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3072/head:pull/3072 PR: https://git.openjdk.java.net/jdk/pull/3072 [cid:E1C4E2F0-ECD0-4C9D-ADB4-B16CA7BCB7FC@home] Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com<mailto:lance.ander...@oracle.com>
Re: RFR: 8261673: Move javadoc for the lookup mechanism to module-info [v2]
On Tue, 16 Mar 2021 21:36:40 GMT, Joe Wang wrote: >> Consolidate and move javadoc for the lookup mechanism to the module summary. > > Joe Wang has updated the pull request incrementally with one additional > commit since the last revision: > > Fix typos: s/XMLEventFactory/XMLInputFactory > s/XMLEventFactory/XMLOutputFactory Marked as reviewed by lancea (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/3020
Re: RFR: 8261673: Move javadoc for the lookup mechanism to module-info
On Tue, 16 Mar 2021 00:52:24 GMT, Joe Wang wrote: > Consolidate and move javadoc for the lookup mechanism to the module summary. Marked as reviewed by lancea (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/3020
Re: RFR: 8173970: jar tool should have a way to extract to a directory
On Mar 12, 2021, at 6:32 AM, Alan Bateman mailto:alan.bate...@oracle.com>> wrote: On 11/03/2021 15:25, Lance Andersen wrote: The only reason I included it is for completeness with tar which the jar options were originally derived from. I think it would be surprising to have two GNU-style long options so I think we should pick one. "--dir" seems okay to me and is consistent with the other the other JDK tools that extract to a directory (although those tools might not be well known). If you prefer "--directory" then I think we'll should add it to the other tools and hide "--dir" from the usage output. I don’t have a strong preference but lean slightly towards ‘-directory’ as it is more descriptive, similar to the other GNU-style commands jar currently supports . Tar supports ‘—cd’, ‘—directory’ in addition to ‘-C’ which is why I suggested supporting both GNU-style long options. Perhaps jpackage should also support —dir/directory in addition to ‘—dest' if we are looking at consistency between java tools. I do agree that it would be nice to be consistent across the java tools for options so if we go the ‘-directory’, we should follow your suggestion and make it the primary and remove ‘—dir’ from the usage output. -Alan [cid:E1C4E2F0-ECD0-4C9D-ADB4-B16CA7BCB7FC@home] Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com<mailto:lance.ander...@oracle.com>
Re: RFR: 8173970: jar tool should have a way to extract to a directory
On Mar 11, 2021, at 9:29 AM, Alan Bateman mailto:alan.bate...@oracle.com>> wrote: On 04/03/2021 12:01, Lance Andersen wrote: Hi Jaikiran From https://docs.oracle.com/en/java/javase/15/docs/specs/man/jar.html The jar man page includes the following : The syntax for the jar command resembles the syntax for the tar command. Because of the above, I feel that we should support: ——— -C —dir —directory The addition of ‘-dir’ adds support for an option used by some of the other java command line tools. Is --directory needed with this proposal? I agree with the suggest to create the directory so that it's consistent with other tools. The only reason I included it is for completeness with tar which the jar options were originally derived from. Could go either way though Best Lance -Alan. [cid:E1C4E2F0-ECD0-4C9D-ADB4-B16CA7BCB7FC@home] Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com<mailto:lance.ander...@oracle.com>
