RFR: JDK-8222489: jcmd VM.system_properties gives unusable paths on Windows

Yasumasa Suenaga suenaga at oss.nttdata.com
Sat Feb 22 12:53:40 UTC 2020 

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
>      >      >
>      >
>

More information about the serviceability-dev mailing list