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

[Chihiro Ito]

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

> 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>>:
> >
> >     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>>>:
> >      >
> >      >     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/20200222/f99bab4d/attachment-0001.htm>