RFR: JDK-8222489: jcmd VM.system_properties gives unusable paths on Windows
Chihiro Ito
chiroito107 at gmail.com
Sun Feb 23 09:26:04 UTC 2020
Hi Yasumasa,
I appreciate that you have reviewed so many times.
Regards,
Chihiro
2020年2月23日(日) 11:10 Yasumasa Suenaga <suenaga at oss.nttdata.com>:
> Hi Chihiro,
>
> Looks good.
> Thank you for your updates and patience!
>
>
> Yasumasa
>
>
> On 2020/02/23 0: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 <mailto:
> 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> <mailto: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>> <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,
> > > >
> > > > 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>>> <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 <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/20200223/c9362954/attachment-0001.htm>
More information about the serviceability-dev
mailing list