RFR: JDK-8222489: jcmd VM.system_properties gives unusable paths on Windows
Chihiro Ito
chiroito107 at gmail.com
Tue Feb 25 03:44:31 UTC 2020
Hi Serguei,
Thanks for your review and advice.
I modified these.
Could you review this again, please?
Webrev : http://cr.openjdk.java.net/~cito/JDK-8222489/webrev.05/
Regards,
Chihiro
2020年2月25日(火) 3:36 serguei.spitsyn at oracle.com <serguei.spitsyn at oracle.com>:
> Hi Chihiro,
>
> Thank you for taking care about this issue!
>
> It looks good to me.
> Just a couple of minor comments.
>
>
> http://cr.openjdk.java.net/~cito/JDK-8222489/webrev.04/src/java.base/share/classes/jdk/internal/vm/VMSupport.java.frames.html
>
> 88 private static String toEscapeSpecialChar(String theString) {
>
> I'd suggest to use replace parameter name "theString" with "source" as you have below:
>
> 92 private static String toEscapeSpace(String source) {
> 96 private static String toISO88591(String source) throws CharacterCodingException {
>
>
> 98 var byteBuf = ByteBuffer.allocate(charBuf.length() * 6); // 6 is 2 bytes for '\\u' as String and 4 bytes for code point.
>
> I'd suggest to put the comment before as a separate line.
>
>
> 107 byteBuf.put(String.format("\\u%04X", (int) charBuf.get()).getBytes());
>
> Space is not needed after the cast in: (int) charBuf.get().
>
>
>
> http://cr.openjdk.java.net/~cito/JDK-8222489/webrev.04/test/jdk/sun/tools/jcmd/TestVM.java.html
>
> 2 * Copyright (c) 2020, 2020, Oracle and/or its affiliates. All rights reserved.
>
> Could you, please, replace: 2020, 2020 => "2020?
> I don't think two numbers are needed for the same year.
>
>
> Thanks,
> Serguei
>
>
> On 2/22/20 07:37, Chihiro Ito wrote:
>
> Hi Yasumasa,
>
> Thank you for your reviews so many times.
> How is this fix?
> Could you review this again, please?
>
> Webrev : http://cr.openjdk.java.net/~cito/JDK-8222489/webrev.04/
>
> Regards,
> Chihiro
>
> 2020年2月22日(土) 21:53 Yasumasa Suenaga <suenaga at oss.nttdata.com>:
>
>> Hi Chihiro,
>>
>>
>> - My proposal is not enough, so you should refine as below.
>> - Exception types in saveConvert() should be limited. Please do
>> not use `throws Exception`.
>> - I guess you use try-catch statement in
>> serializePropertiesToByteArray due to above checked exception.
>> It should be throw runtime exception when an exception occurs.
>> - Capacity of byteBuf (charBuf.length() * 5) should be
>> (charBuf.length() * 6)
>> because non 8859-1 chars would be "\uxxxx" (6 chars).
>> Also please leave comment for it because a maintainer might not
>> understand the meaning of multiplying 6 in future.
>>
>> - `output.shouldNotContain("C:\\:\\\\");` in testcase is correct?
>> I guess you want to check "C\\:\\\\" is not contained.
>>
>> - To check '\n', you can use Platform::isWindows as below:
>> output.shouldContain(Platform.isWindows() ?
>> "line.separator=\\r\\n" : "lineseparator=\\n");
>>
>>
>> Yasumasa
>>
>>
>> On 2020/02/22 19:23, Chihiro Ito wrote:
>> > Hi Yasumasa,
>> >
>> > The line separator is not modified because it depends on the
>> environment, but the others have been modified.
>> >
>> > Could you review this again?
>> >
>> > Webrev : http://cr.openjdk.java.net/~cito/JDK-8222489/webrev.03/
>> >
>> > Regards,
>> > Chihiro
>> >
>> > 2020年2月22日(土) 12:32 Yasumasa Suenaga <suenaga at oss.nttdata.com <mailto:
>> suenaga at oss.nttdata.com>>:
>> >
>> > Hi Chihiro,
>> >
>> > Thank you for updating the webrev.
>> >
>> >
>> > - You use BufferedWriter to create the output, however I think
>> it would be more simply if you use PrintWriter.
>> >
>> > - Your change would work incorrectly when system property
>> contains mixture of ascii and non-ascii.
>> > You can see it with "-Dmixture=aあi". It would be converted to
>> "a\u0061\u3042", it should be "a\u3042i".
>> >
>> > - Currently key value which contains space char, it would be
>> escaped, but your change does not do so.
>> > You can see it with "-D"space space=blank blank"".
>> >
>> > - You should not use String::trim to create String from
>> ByteBuffer because property value might be contain blank in its tail.
>> > You might use ByteBuffer::slice or part of ByteBuffer::array
>> for it.
>> >
>> > - Did you try to use escaped chars in jtreg testcase? I guess
>> you can set multibytes chars (e.g. CJK chars) with "\u".
>> > In case of mixture of Japanese (Hiragana) and ASCII chars,
>> you can embed "-Dmixture=a\u3042i" to testcase. (I'm not sure that...)
>> >
>> > - In test case, I recommend you to evaluate entire of line.
>> > For example, if you want to check line.separator, you should
>> evaluate as below:
>> > output.shouldContain("line.separator=\\n");
>> >
>> >
>> > Thanks,
>> >
>> > Yasumasa
>> >
>> >
>> > On 2020/02/22 0:44, Chihiro Ito wrote:
>> > > Hi Yasumasa,
>> > >
>> > > Thank you for your advice.
>> > >
>> > > I decided not to use regular expressions. because of the number
>> of \is confusing.
>> > > I stopped using codePointAt() and used CharsetEncoder to work
>> with ISO 8859 -1.
>> > > I added some environment variables to the test. However,
>> environment variables that contain multi bytes or spaces are not included
>> because jtreg does not support them.
>> > >
>> > > Could you review this again, please?
>> > >
>> > > Webrev : http://cr.openjdk.java.net/~cito/JDK-8222489/webrev.02/
>> > >
>> > > Regards,
>> > > Chihiro
>> > >
>> > > 2020年2月20日(木) 22:39 Yasumasa Suenaga <suenaga at oss.nttdata.com
>> <mailto:suenaga at oss.nttdata.com> <mailto:suenaga at oss.nttdata.com <mailto:
>> suenaga at oss.nttdata.com>>>:
>> > >
>> > > Hi Chihiro,
>> > >
>> > > On 2020/02/20 20:20, Chihiro Ito wrote:
>> > > > Hi Yasumasa,
>> > > >
>> > > > Thank you for your quick review.
>> > > >
>> > > > I modified the code without Properties::store.
>> > > >
>> > > > Could you review this again, please?
>> > > >
>> > > > Webrev :
>> http://cr.openjdk.java.net/~cito/JDK-8222489/webrev.01/
>> > >
>> > > - Your change shows "\n" as "\\n". Is it ok? Currently
>> "\n" would be shown straightly.
>> > > - Your change uses Character::codePointAt to convert
>> char to int value.
>> > > According to Javadoc, it would be different value if a
>> char is in surrogate range.
>> > > - Description of serializePropertiesToByteArray() says
>> the return value is encoded in ISO 8859-1,
>> > > but it does not seems to be so because the logic
>> depends on the spec of Properties::store. Is it ok?
>> > > - Test case does not stable because system properties
>> might be different from your environment.
>> > > I suggest you to set system properties for testing
>> explicitly. E.g.
>> > > -Dnormal=normal_val -D"space space=blank blank"
>> -Dnonascii=あいうえお -Dopenjdk_url=http://openjdk.java.net/ -Dbackslash="\\"
>> > > * Also I recommend you to check "\n" in the test
>> from `line.separator`. I think it is stable property.
>> > >
>> > > I've not convinced whether we should compliant to the
>> comment which says for ISO 8859-1.
>> > > If it is important, we can use CharsetEncoder from
>> ISO_8859_1 as below:
>> > >
>> > >
>> http://cr.openjdk.java.net/~ysuenaga/JDK-8222489/proposal-encoder/
>> > >
>> > > OTOH we can keep current behavior, we can implement more
>> simply as below:
>> > > (It's similar to yours.)
>> > >
>> > >
>> http://cr.openjdk.java.net/~ysuenaga/JDK-8222489/proposal-props-style/
>> > >
>> > >
>> > > Thanks,
>> > >
>> > > Yasumasa
>> > >
>> > >
>> > > > Regards,
>> > > > Chihiro
>> > > >
>> > > >
>> > > > 2020年2月20日(木) 9:34 Yasumasa Suenaga <
>> suenaga at oss.nttdata.com <mailto:suenaga at oss.nttdata.com> <mailto:
>> suenaga at oss.nttdata.com <mailto:suenaga at oss.nttdata.com>> <mailto:
>> suenaga at oss.nttdata.com <mailto:suenaga at oss.nttdata.com> <mailto:
>> suenaga at oss.nttdata.com <mailto:suenaga at oss.nttdata.com>>>>:
>> > > >
>> > > > Hi Chihiro,
>> > > >
>> > > > I think this problem is caused by spec of
>> `Properties::store(Writer)`.
>> > > >
>> > > > `Properties::store(OutputStream)` says that the
>> output format is as same as `store(Writer)` [1].
>> > > > `Properties::store(Writer)` says that `#`, `!`, `=`,
>> `:` are written with a preceding backslash [2].
>> > > >
>> > > > So I think we should not use `Properties::store` to
>> serialize properties.
>> > > >
>> > > >
>> > > > Thanks,
>> > > >
>> > > > Yasumasa
>> > > >
>> > > >
>> > > > [1]
>> https://download.java.net/java/early_access/jdk15/docs/api/java.base/java/util/Properties.html#store(java.io.OutputStream,java.lang.String)
>> > > > [2]
>> https://download.java.net/java/early_access/jdk15/docs/api/java.base/java/util/Properties.html#store(java.io.Writer,java.lang.String)
>> > > >
>> > > >
>> > > > On 2020/02/19 22:36, Chihiro Ito wrote:
>> > > > > Hi,
>> > > > >
>> > > > > Could you review this tiny fix, please?
>> > > > >
>> > > > > This problem affected not the only path on
>> Windows, but also Linux and URLs using ":".
>> > > > >
>> > > > > Webrev :
>> http://cr.openjdk.java.net/~cito/JDK-8222489/webrev.00/
>> > > > > JBS :
>> https://bugs.openjdk.java.net/browse/JDK-8222489
>> > > > >
>> > > > > Regards,
>> > > > > Chihiro
>> > > >
>> > >
>> >
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20200225/7ac59d1f/attachment-0001.htm>
More information about the serviceability-dev
mailing list