RFR(S): 8240333: jmod incorrectly updates .jar and .jmod files during hashing
Hi, can I please get a review for the following small fix: http://cr.openjdk.java.net/~simonis/webrevs/2020/8240333/ https://bugs.openjdk.java.net/browse/JDK-8240333 The "jmod" tool needs to update .jar files as well as .jmod files when adding hashes to the module-info file. This is handled in the method jdk.tools.jmod.JmodTask.updateModularJar() for modular jar files and in the methods jdk.tools.jmod.JmodTask.updateJmodFile() and jdk.tools.jmod.JmodOutputStream.writeEntry() for .jmod files. Unfortunately, both implementations are incorrect because they are writing the JarEntry/ZipEntry entries which they reading from the ZipInputStream verbatim to the ZipOutputStream. This approach is not correct, because Zip/Entry/JarEntry contains both, the compressed and uncompressed size of the corresponding entry. But the original Deflater which initially compressed that entry can be either different from the current one or it might have used a different compression level compared to the current Deflater which re-deflates the entry data. When the newly created entry is closed, the new compressed size will be checked against the original compressed size if that was recorded in the ZipEntry/JarEntry entry and an exception will be thrown, when they differ. For modular jar files it looks as follows: $ jmod hash --module-path . --hash-modules .* Error: java.util.zip.ZipException: invalid entry compressed size (expected 58 but got 57 bytes) java.io.UncheckedIOException: java.util.zip.ZipException: invalid entry compressed size (expected 58 but got 57 bytes) at jdk.jlink/jdk.tools.jmod.JmodTask$Hasher.lambda$updateModularJar$3(JmodTask.java:995) at java.base/java.util.zip.ZipFile$EntrySpliterator.tryAdvance(ZipFile.java:571) at java.base/java.util.Spliterator.forEachRemaining(Spliterator.java:326) at java.base/java.util.stream.ReferencePipeline$Head.forEach(ReferencePipeline.java:658) at jdk.jlink/jdk.tools.jmod.JmodTask$Hasher.updateModularJar(JmodTask.java:980) at jdk.jlink/jdk.tools.jmod.JmodTask$Hasher.updateModuleInfo(JmodTask.java:957) at jdk.jlink/jdk.tools.jmod.JmodTask.lambda$hashModules$3(JmodTask.java:300) at java.base/java.util.HashMap.forEach(HashMap.java:1425) at jdk.jlink/jdk.tools.jmod.JmodTask.hashModules(JmodTask.java:291) at jdk.jlink/jdk.tools.jmod.JmodTask.run(JmodTask.java:220) at jdk.jlink/jdk.tools.jmod.Main.main(Main.java:34) Suppressed: java.util.zip.ZipException: invalid entry compressed size (expected 58 but got 57 bytes) at java.base/java.util.zip.ZipOutputStream.closeEntry(ZipOutputStream.java:267) at java.base/java.util.zip.ZipOutputStream.finish(ZipOutputStream.java:360) at java.base/java.util.zip.DeflaterOutputStream.close(DeflaterOutputStream.java:237) at java.base/java.util.zip.ZipOutputStream.close(ZipOutputStream.java:377) at jdk.jlink/jdk.tools.jmod.JmodTask$Hasher.updateModularJar(JmodTask.java:976) ... 6 more Caused by: java.util.zip.ZipException: invalid entry compressed size (expected 58 but got 57 bytes) at java.base/java.util.zip.ZipOutputStream.closeEntry(ZipOutputStream.java:267) at jdk.jlink/jdk.tools.jmod.JmodTask$Hasher.lambda$updateModularJar$3(JmodTask.java:992) ... 10 more For jmod files it looks like this: $ jmod hash --module-path . --hash-modules .* Error: java.util.zip.ZipException: invalid entry compressed size (expected 383 but got 377 bytes) java.io.UncheckedIOException: java.util.zip.ZipException: invalid entry compressed size (expected 383 but got 377 bytes) at jdk.jlink/jdk.tools.jmod.JmodTask$Hasher.lambda$updateJmodFile$4(JmodTask.java:1021) at java.base/java.util.stream.ForEachOps$ForEachOp$OfRef.accept(ForEachOps.java:183) at java.base/java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:195) at java.base/java.util.zip.ZipFile$EntrySpliterator.tryAdvance(ZipFile.java:571) at java.base/java.util.Spliterator.forEachRemaining(Spliterator.java:326) at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:484) at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:474) at java.base/java.util.stream.ForEachOps$ForEachOp.evaluateSequential(ForEachOps.java:150) at java.base/java.util.stream.ForEachOps$ForEachOp$OfRef.evaluateSequential(ForEachOps.java:173) at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234) at java.base/java.util.stream.ReferencePipeline.forEach(ReferencePipeline.java:497) at jdk.jlink/jdk.tools.jmod.JmodTask$Hasher.updateJmodFile(JmodTask.java:1009) at jdk.jlink/jdk.tools.jmod.JmodTask$Hasher.updateModuleInfo(JmodTask.java:955) at jdk.jlink/jdk.tools.jmod.JmodTask.lambda$hashModules$3(JmodTask.java:300) at java.base/java.util.HashMap.forEach(HashMap.java:1425) at jdk.jlink/jdk.tools.jmod.JmodTask.hashModules(JmodTask.java:291) at jdk.jlink/jdk.tools.jmod.JmodTask.run(JmodTask.java:220) at jdk.jlink/jdk.tools.jmod.Main.main(Main.java:34) Suppressed: java.util.zip.ZipException: invalid entry compressed size (expected 383 but got 377 bytes) at java.base/java.util.zip.ZipOutputStream.closeEntry(ZipOutputStream.java:267) at java.base/java.util.zip.ZipOutputStream.finish(ZipOutputStream.java:360) at java.base/java.util.zip.DeflaterOutputStream.close(DeflaterOutputStream.java:237) at java.base/java.util.zip.ZipOutputStream.close(ZipOutputStream.java:377) at jdk.jlink/jdk.tools.jmod.JmodOutputStream.close(JmodOutputStream.java:114) at jdk.jlink/jdk.tools.jmod.JmodTask$Hasher.updateJmodFile(JmodTask.java:1006) ... 6 more Caused by: java.util.zip.ZipException: invalid entry compressed size (expected 383 but got 377 bytes) at java.base/java.util.zip.ZipOutputStream.closeEntry(ZipOutputStream.java:267) at jdk.jlink/jdk.tools.jmod.JmodOutputStream.writeEntry(JmodOutputStream.java:97) at jdk.jlink/jdk.tools.jmod.JmodTask$Hasher.lambda$updateJmodFile$4(JmodTask.java:1018) ... 17 more The errors can be provoked by creating the initial jar or jmod files with a different zlib implementation (e.g. LD_LIBRARY_PATH=/ziptests/build/zlib-cloudflare jar -cf ../mb_cf.jar *) The fix is simple. Instead of copying the ZipEntry/JarEntry entries verbatim to the output stream, simply write newly created ZipEntry entries to the output stream which only contain the required attributes. This is also the way how this is done by the jar tool, when it updates jar files. For the modular jar file case, creating a new entry is not feasible, because at that specific location we are dealing with JerEntry entries which can contain additional information. Unfortunately, it is not possible to set this information on a newly created JarEntry entry. But we can still reset the "compressedSize" of the JarEntry to "-1" which also prevents the additional check after the entry has been written. Thank you and best regards, Volker PS: and by the way, this is the last bug related to the wrong usage of ZipOutputSream.putNextEntry() within the OpenJDK :) I've scanned all the sources and couldn't find any other suspicious places.
Looks Good To Me ... except you should fix that "delfated" typo. Those looking for a zip implementation career path can try to fix up all the compressedSize in the local headers so that they are correctly filled in. Which is hard because you don't know the compressedSize when you are writing the local entry header with putNextEntry. On Tue, Mar 3, 2020 at 3:02 AM Volker Simonis <volker.simonis@gmail.com> wrote:
Hi,
can I please get a review for the following small fix:
http://cr.openjdk.java.net/~simonis/webrevs/2020/8240333/ https://bugs.openjdk.java.net/browse/JDK-8240333
The "jmod" tool needs to update .jar files as well as .jmod files when adding hashes to the module-info file. This is handled in the method jdk.tools.jmod.JmodTask.updateModularJar() for modular jar files and in the methods jdk.tools.jmod.JmodTask.updateJmodFile() and jdk.tools.jmod.JmodOutputStream.writeEntry() for .jmod files.
Unfortunately, both implementations are incorrect because they are writing the JarEntry/ZipEntry entries which they reading from the ZipInputStream verbatim to the ZipOutputStream. This approach is not correct, because Zip/Entry/JarEntry contains both, the compressed and uncompressed size of the corresponding entry. But the original Deflater which initially compressed that entry can be either different from the current one or it might have used a different compression level compared to the current Deflater which re-deflates the entry data.
When the newly created entry is closed, the new compressed size will be checked against the original compressed size if that was recorded in the ZipEntry/JarEntry entry and an exception will be thrown, when they differ. For modular jar files it looks as follows:
$ jmod hash --module-path . --hash-modules .* Error: java.util.zip.ZipException: invalid entry compressed size (expected 58 but got 57 bytes) java.io.UncheckedIOException: java.util.zip.ZipException: invalid entry compressed size (expected 58 but got 57 bytes) at jdk.jlink/jdk.tools.jmod.JmodTask$Hasher.lambda$updateModularJar$3(JmodTask.java:995) at java.base/java.util.zip.ZipFile$EntrySpliterator.tryAdvance(ZipFile.java:571) at java.base/java.util.Spliterator.forEachRemaining(Spliterator.java:326) at java.base/java.util.stream.ReferencePipeline$Head.forEach(ReferencePipeline.java:658) at jdk.jlink/jdk.tools.jmod.JmodTask$Hasher.updateModularJar(JmodTask.java:980) at jdk.jlink/jdk.tools.jmod.JmodTask$Hasher.updateModuleInfo(JmodTask.java:957) at jdk.jlink/jdk.tools.jmod.JmodTask.lambda$hashModules$3(JmodTask.java:300) at java.base/java.util.HashMap.forEach(HashMap.java:1425) at jdk.jlink/jdk.tools.jmod.JmodTask.hashModules(JmodTask.java:291) at jdk.jlink/jdk.tools.jmod.JmodTask.run(JmodTask.java:220) at jdk.jlink/jdk.tools.jmod.Main.main(Main.java:34) Suppressed: java.util.zip.ZipException: invalid entry compressed size (expected 58 but got 57 bytes) at java.base/java.util.zip.ZipOutputStream.closeEntry(ZipOutputStream.java:267) at java.base/java.util.zip.ZipOutputStream.finish(ZipOutputStream.java:360) at java.base/java.util.zip.DeflaterOutputStream.close(DeflaterOutputStream.java:237) at java.base/java.util.zip.ZipOutputStream.close(ZipOutputStream.java:377) at jdk.jlink/jdk.tools.jmod.JmodTask$Hasher.updateModularJar(JmodTask.java:976) ... 6 more Caused by: java.util.zip.ZipException: invalid entry compressed size (expected 58 but got 57 bytes) at java.base/java.util.zip.ZipOutputStream.closeEntry(ZipOutputStream.java:267) at jdk.jlink/jdk.tools.jmod.JmodTask$Hasher.lambda$updateModularJar$3(JmodTask.java:992) ... 10 more
For jmod files it looks like this:
$ jmod hash --module-path . --hash-modules .* Error: java.util.zip.ZipException: invalid entry compressed size (expected 383 but got 377 bytes) java.io.UncheckedIOException: java.util.zip.ZipException: invalid entry compressed size (expected 383 but got 377 bytes) at jdk.jlink/jdk.tools.jmod.JmodTask$Hasher.lambda$updateJmodFile$4(JmodTask.java:1021) at java.base/java.util.stream.ForEachOps$ForEachOp$OfRef.accept(ForEachOps.java:183) at java.base/java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:195) at java.base/java.util.zip.ZipFile$EntrySpliterator.tryAdvance(ZipFile.java:571) at java.base/java.util.Spliterator.forEachRemaining(Spliterator.java:326) at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:484) at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:474) at java.base/java.util.stream.ForEachOps$ForEachOp.evaluateSequential(ForEachOps.java:150) at java.base/java.util.stream.ForEachOps$ForEachOp$OfRef.evaluateSequential(ForEachOps.java:173) at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234) at java.base/java.util.stream.ReferencePipeline.forEach(ReferencePipeline.java:497) at jdk.jlink/jdk.tools.jmod.JmodTask$Hasher.updateJmodFile(JmodTask.java:1009) at jdk.jlink/jdk.tools.jmod.JmodTask$Hasher.updateModuleInfo(JmodTask.java:955) at jdk.jlink/jdk.tools.jmod.JmodTask.lambda$hashModules$3(JmodTask.java:300) at java.base/java.util.HashMap.forEach(HashMap.java:1425) at jdk.jlink/jdk.tools.jmod.JmodTask.hashModules(JmodTask.java:291) at jdk.jlink/jdk.tools.jmod.JmodTask.run(JmodTask.java:220) at jdk.jlink/jdk.tools.jmod.Main.main(Main.java:34) Suppressed: java.util.zip.ZipException: invalid entry compressed size (expected 383 but got 377 bytes) at java.base/java.util.zip.ZipOutputStream.closeEntry(ZipOutputStream.java:267) at java.base/java.util.zip.ZipOutputStream.finish(ZipOutputStream.java:360) at java.base/java.util.zip.DeflaterOutputStream.close(DeflaterOutputStream.java:237) at java.base/java.util.zip.ZipOutputStream.close(ZipOutputStream.java:377) at jdk.jlink/jdk.tools.jmod.JmodOutputStream.close(JmodOutputStream.java:114) at jdk.jlink/jdk.tools.jmod.JmodTask$Hasher.updateJmodFile(JmodTask.java:1006) ... 6 more Caused by: java.util.zip.ZipException: invalid entry compressed size (expected 383 but got 377 bytes) at java.base/java.util.zip.ZipOutputStream.closeEntry(ZipOutputStream.java:267) at jdk.jlink/jdk.tools.jmod.JmodOutputStream.writeEntry(JmodOutputStream.java:97) at jdk.jlink/jdk.tools.jmod.JmodTask$Hasher.lambda$updateJmodFile$4(JmodTask.java:1018) ... 17 more
The errors can be provoked by creating the initial jar or jmod files with a different zlib implementation (e.g. LD_LIBRARY_PATH=/ziptests/build/zlib-cloudflare jar -cf ../mb_cf.jar *)
The fix is simple. Instead of copying the ZipEntry/JarEntry entries verbatim to the output stream, simply write newly created ZipEntry entries to the output stream which only contain the required attributes. This is also the way how this is done by the jar tool, when it updates jar files.
For the modular jar file case, creating a new entry is not feasible, because at that specific location we are dealing with JerEntry entries which can contain additional information. Unfortunately, it is not possible to set this information on a newly created JarEntry entry. But we can still reset the "compressedSize" of the JarEntry to "-1" which also prevents the additional check after the entry has been written.
Thank you and best regards, Volker
PS: and by the way, this is the last bug related to the wrong usage of ZipOutputSream.putNextEntry() within the OpenJDK :) I've scanned all the sources and couldn't find any other suspicious places.
On Tue, Mar 3, 2020 at 5:20 PM Martin Buchholz <martinrb@google.com> wrote:
Looks Good To Me ... except you should fix that "delfated" typo.
Thanks. Corrected the typo along with some comment shortening suggested by Alan: http://cr.openjdk.java.net/~simonis/webrevs/2020/8240333.01/
Those looking for a zip implementation career path
:-))
can try to fix up all the compressedSize in the local headers so that they are correctly filled in. Which is hard because you don't know the compressedSize when you are writing the local entry header with putNextEntry.
You can't know it be fore you are doing it :)
On Tue, Mar 3, 2020 at 3:02 AM Volker Simonis <volker.simonis@gmail.com> wrote:
Hi,
can I please get a review for the following small fix:
http://cr.openjdk.java.net/~simonis/webrevs/2020/8240333/ https://bugs.openjdk.java.net/browse/JDK-8240333
The "jmod" tool needs to update .jar files as well as .jmod files when adding hashes to the module-info file. This is handled in the method jdk.tools.jmod.JmodTask.updateModularJar() for modular jar files and in the methods jdk.tools.jmod.JmodTask.updateJmodFile() and jdk.tools.jmod.JmodOutputStream.writeEntry() for .jmod files.
Unfortunately, both implementations are incorrect because they are writing the JarEntry/ZipEntry entries which they reading from the ZipInputStream verbatim to the ZipOutputStream. This approach is not correct, because Zip/Entry/JarEntry contains both, the compressed and uncompressed size of the corresponding entry. But the original Deflater which initially compressed that entry can be either different from the current one or it might have used a different compression level compared to the current Deflater which re-deflates the entry data.
When the newly created entry is closed, the new compressed size will be checked against the original compressed size if that was recorded in the ZipEntry/JarEntry entry and an exception will be thrown, when they differ. For modular jar files it looks as follows:
$ jmod hash --module-path . --hash-modules .* Error: java.util.zip.ZipException: invalid entry compressed size (expected 58 but got 57 bytes) java.io.UncheckedIOException: java.util.zip.ZipException: invalid entry compressed size (expected 58 but got 57 bytes) at jdk.jlink/jdk.tools.jmod.JmodTask$Hasher.lambda$updateModularJar$3(JmodTask.java:995) at java.base/java.util.zip.ZipFile$EntrySpliterator.tryAdvance(ZipFile.java:571) at java.base/java.util.Spliterator.forEachRemaining(Spliterator.java:326) at java.base/java.util.stream.ReferencePipeline$Head.forEach(ReferencePipeline.java:658) at jdk.jlink/jdk.tools.jmod.JmodTask$Hasher.updateModularJar(JmodTask.java:980) at jdk.jlink/jdk.tools.jmod.JmodTask$Hasher.updateModuleInfo(JmodTask.java:957) at jdk.jlink/jdk.tools.jmod.JmodTask.lambda$hashModules$3(JmodTask.java:300) at java.base/java.util.HashMap.forEach(HashMap.java:1425) at jdk.jlink/jdk.tools.jmod.JmodTask.hashModules(JmodTask.java:291) at jdk.jlink/jdk.tools.jmod.JmodTask.run(JmodTask.java:220) at jdk.jlink/jdk.tools.jmod.Main.main(Main.java:34) Suppressed: java.util.zip.ZipException: invalid entry compressed size (expected 58 but got 57 bytes) at java.base/java.util.zip.ZipOutputStream.closeEntry(ZipOutputStream.java:267) at java.base/java.util.zip.ZipOutputStream.finish(ZipOutputStream.java:360) at java.base/java.util.zip.DeflaterOutputStream.close(DeflaterOutputStream.java:237) at java.base/java.util.zip.ZipOutputStream.close(ZipOutputStream.java:377) at jdk.jlink/jdk.tools.jmod.JmodTask$Hasher.updateModularJar(JmodTask.java:976) ... 6 more Caused by: java.util.zip.ZipException: invalid entry compressed size (expected 58 but got 57 bytes) at java.base/java.util.zip.ZipOutputStream.closeEntry(ZipOutputStream.java:267) at jdk.jlink/jdk.tools.jmod.JmodTask$Hasher.lambda$updateModularJar$3(JmodTask.java:992) ... 10 more
For jmod files it looks like this:
$ jmod hash --module-path . --hash-modules .* Error: java.util.zip.ZipException: invalid entry compressed size (expected 383 but got 377 bytes) java.io.UncheckedIOException: java.util.zip.ZipException: invalid entry compressed size (expected 383 but got 377 bytes) at jdk.jlink/jdk.tools.jmod.JmodTask$Hasher.lambda$updateJmodFile$4(JmodTask.java:1021) at java.base/java.util.stream.ForEachOps$ForEachOp$OfRef.accept(ForEachOps.java:183) at java.base/java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:195) at java.base/java.util.zip.ZipFile$EntrySpliterator.tryAdvance(ZipFile.java:571) at java.base/java.util.Spliterator.forEachRemaining(Spliterator.java:326) at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:484) at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:474) at java.base/java.util.stream.ForEachOps$ForEachOp.evaluateSequential(ForEachOps.java:150) at java.base/java.util.stream.ForEachOps$ForEachOp$OfRef.evaluateSequential(ForEachOps.java:173) at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234) at java.base/java.util.stream.ReferencePipeline.forEach(ReferencePipeline.java:497) at jdk.jlink/jdk.tools.jmod.JmodTask$Hasher.updateJmodFile(JmodTask.java:1009) at jdk.jlink/jdk.tools.jmod.JmodTask$Hasher.updateModuleInfo(JmodTask.java:955) at jdk.jlink/jdk.tools.jmod.JmodTask.lambda$hashModules$3(JmodTask.java:300) at java.base/java.util.HashMap.forEach(HashMap.java:1425) at jdk.jlink/jdk.tools.jmod.JmodTask.hashModules(JmodTask.java:291) at jdk.jlink/jdk.tools.jmod.JmodTask.run(JmodTask.java:220) at jdk.jlink/jdk.tools.jmod.Main.main(Main.java:34) Suppressed: java.util.zip.ZipException: invalid entry compressed size (expected 383 but got 377 bytes) at java.base/java.util.zip.ZipOutputStream.closeEntry(ZipOutputStream.java:267) at java.base/java.util.zip.ZipOutputStream.finish(ZipOutputStream.java:360) at java.base/java.util.zip.DeflaterOutputStream.close(DeflaterOutputStream.java:237) at java.base/java.util.zip.ZipOutputStream.close(ZipOutputStream.java:377) at jdk.jlink/jdk.tools.jmod.JmodOutputStream.close(JmodOutputStream.java:114) at jdk.jlink/jdk.tools.jmod.JmodTask$Hasher.updateJmodFile(JmodTask.java:1006) ... 6 more Caused by: java.util.zip.ZipException: invalid entry compressed size (expected 383 but got 377 bytes) at java.base/java.util.zip.ZipOutputStream.closeEntry(ZipOutputStream.java:267) at jdk.jlink/jdk.tools.jmod.JmodOutputStream.writeEntry(JmodOutputStream.java:97) at jdk.jlink/jdk.tools.jmod.JmodTask$Hasher.lambda$updateJmodFile$4(JmodTask.java:1018) ... 17 more
The errors can be provoked by creating the initial jar or jmod files with a different zlib implementation (e.g. LD_LIBRARY_PATH=/ziptests/build/zlib-cloudflare jar -cf ../mb_cf.jar *)
The fix is simple. Instead of copying the ZipEntry/JarEntry entries verbatim to the output stream, simply write newly created ZipEntry entries to the output stream which only contain the required attributes. This is also the way how this is done by the jar tool, when it updates jar files.
For the modular jar file case, creating a new entry is not feasible, because at that specific location we are dealing with JerEntry entries which can contain additional information. Unfortunately, it is not possible to set this information on a newly created JarEntry entry. But we can still reset the "compressedSize" of the JarEntry to "-1" which also prevents the additional check after the entry has been written.
Thank you and best regards, Volker
PS: and by the way, this is the last bug related to the wrong usage of ZipOutputSream.putNextEntry() within the OpenJDK :) I've scanned all the sources and couldn't find any other suspicious places.
On 03/03/2020 11:01, Volker Simonis wrote:
Hi,
can I please get a review for the following small fix:
http://cr.openjdk.java.net/~simonis/webrevs/2020/8240333/ https://bugs.openjdk.java.net/browse/JDK-8240333
The code changes okay but I think the comment needs a bit of clean-up. Maybe drop L96-97 so that it just says that it preserves the attributes that don't change. -Alan.
I think this all looks OK. I would consider the same comment clean-up in JmodTask.java as well Best Lance
On Mar 3, 2020, at 11:27 AM, Alan Bateman <Alan.Bateman@oracle.com> wrote:
On 03/03/2020 11:01, Volker Simonis wrote:
Hi,
can I please get a review for the following small fix:
http://cr.openjdk.java.net/~simonis/webrevs/2020/8240333/ https://bugs.openjdk.java.net/browse/JDK-8240333
The code changes okay but I think the comment needs a bit of clean-up. Maybe drop L96-97 so that it just says that it preserves the attributes that don't change.
-Alan.
<http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 Lance.Andersen@oracle.com <mailto:Lance.Andersen@oracle.com>
Thanks Lance. I've just sent out an updated webrev with the shortened comments: http://cr.openjdk.java.net/~simonis/webrevs/2020/8240333.01/ On Tue, Mar 3, 2020 at 7:00 PM Lance Andersen <lance.andersen@oracle.com> wrote:
I think this all looks OK. I would consider the same comment clean-up in JmodTask.java as well
Best Lance
On Mar 3, 2020, at 11:27 AM, Alan Bateman <Alan.Bateman@oracle.com> wrote:
On 03/03/2020 11:01, Volker Simonis wrote:
Hi,
can I please get a review for the following small fix:
http://cr.openjdk.java.net/~simonis/webrevs/2020/8240333/ https://bugs.openjdk.java.net/browse/JDK-8240333
The code changes okay but I think the comment needs a bit of clean-up. Maybe drop L96-97 so that it just says that it preserves the attributes that don't change.
-Alan.
Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 Lance.Andersen@oracle.com
Looks good to me Thank you Volker
On Mar 3, 2020, at 1:03 PM, Volker Simonis <volker.simonis@gmail.com> wrote:
Thanks Lance.
I've just sent out an updated webrev with the shortened comments:
http://cr.openjdk.java.net/~simonis/webrevs/2020/8240333.01/ <http://cr.openjdk.java.net/~simonis/webrevs/2020/8240333.01/>
On Tue, Mar 3, 2020 at 7:00 PM Lance Andersen <lance.andersen@oracle.com <mailto:lance.andersen@oracle.com>> wrote:
I think this all looks OK. I would consider the same comment clean-up in JmodTask.java as well
Best Lance
On Mar 3, 2020, at 11:27 AM, Alan Bateman <Alan.Bateman@oracle.com> wrote:
On 03/03/2020 11:01, Volker Simonis wrote:
Hi,
can I please get a review for the following small fix:
http://cr.openjdk.java.net/~simonis/webrevs/2020/8240333/ https://bugs.openjdk.java.net/browse/JDK-8240333
The code changes okay but I think the comment needs a bit of clean-up. Maybe drop L96-97 so that it just says that it preserves the attributes that don't change.
-Alan.
Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 Lance.Andersen@oracle.com <mailto:Lance.Andersen@oracle.com>
<http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif> <http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 Lance.Andersen@oracle.com <mailto:Lance.Andersen@oracle.com>
On Tue, Mar 3, 2020 at 5:27 PM Alan Bateman <Alan.Bateman@oracle.com> wrote:
On 03/03/2020 11:01, Volker Simonis wrote:
Hi,
can I please get a review for the following small fix:
http://cr.openjdk.java.net/~simonis/webrevs/2020/8240333/ https://bugs.openjdk.java.net/browse/JDK-8240333
The code changes okay but I think the comment needs a bit of clean-up. Maybe drop L96-97 so that it just says that it preserves the attributes that don't change.
Thanks. Shortened the comments as suggested: http://cr.openjdk.java.net/~simonis/webrevs/2020/8240333.01/ OK, now?
-Alan.
On 03/03/2020 18:01, Volker Simonis wrote:
: Thanks. Shortened the comments as suggested:
http://cr.openjdk.java.net/~simonis/webrevs/2020/8240333.01/
OK, now?
Thanks, looks good to me. -Alan
participants (4)
-
Alan Bateman
-
Lance Andersen
-
Martin Buchholz
-
Volker Simonis