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

[serguei.spitsyn at oracle.com]

Hi Chihiro,

I've tested and pushed your fix but the impact of fix was underestimated.
The fix caused several regressions and the following bug was filed:
   https://bugs.openjdk.java.net/browse/JDK-8240881

Now, I'm working on removing the fix of JDK-8222489 with the anti-delta.
You can find and review my RFR posted on the serviceability-dev mailing 
list:
   RFR: 8240881: several tests are failing due to encoding failures

You can file another bug as a replacement of JDK-8222489.
I will help you with the information about test regressions caused by it.

Thanks,
Serguei


On 3/10/20 02:54, serguei.spitsyn at oracle.com wrote:
> Hi Chihiro,
>
> Yes, I'll sponsor it.
> Thank you for the update.
>
> Thanks,
> Serguei
>
>
> On 3/8/20 06:05, Chihiro Ito wrote:
>> Hi,
>>
>> I'm sorry. I included "JDK-" in the changeset title. I removed it and
>> updated it.
>>
>> Change set : 
>> http://cr.openjdk.java.net/~cito/JDK-8222489/webrev.06/changeset
>>
>> Regards,
>> Chihiro
>>
>> 2020年3月7日(土) 23:13 Chihiro Ito <chiroito107 at gmail.com>:
>>> Hi Serguei and Yasumasa,
>>>
>>> I update the copyright year and created the change set.
>>>
>>> Could you sponsor this, please?
>>>
>>> Webrev : http://cr.openjdk.java.net/~cito/JDK-8222489/webrev.06/
>>> Change set : 
>>> http://cr.openjdk.java.net/~cito/JDK-8222489/webrev.06/changeset
>>>
>>> Regards,
>>> Chihiro
>>>
>>>
>>> 2020年3月7日(土) 16:03 Yasumasa Suenaga <suenaga at oss.nttdata.com>:
>>>
>>>
>>>> Hi Chihiro,
>>>>
>>>> I'm also ok with webrev.05 after updating copyright year.
>>>>
>>>>
>>>> Yasumasa
>>>>
>>>>
>>>> On 2020/03/07 3:32, serguei.spitsyn at oracle.com wrote:
>>>>> Hi Chichiro,
>>>>>
>>>>> I'm okay with the fix.
>>>>> Could you, please, update the copyright date in || 
>>>>> src/java.base/share/classes/jdk/internal/vm/VMSupport.java before 
>>>>> push?
>>>>>
>>>>> Thanks,
>>>>> Serguei
>>>>>
>>>>>
>>>>> On 3/6/20 07:24, Chihiro Ito wrote:
>>>>>> Hi Serguei,
>>>>>>
>>>>>> Could you review this again, please?
>>>>>>
>>>>>> Regards,
>>>>>> Chihiro
>>>>>>
>>>>>>
>>>>>> 2020年2月27日(木) 22:11 Chihiro Ito<chiroito107 at gmail.com>:
>>>>>>> Hi Ralf,
>>>>>>>
>>>>>>> Thank you for your advice.
>>>>>>>
>>>>>>> 1.
>>>>>>> The comment of serializePropertiesToByteArray in VMSupport is 
>>>>>>> "The stream written to the byte array is ISO 8859-1 encoded.".
>>>>>>> But the previous implementation does not keep this. I think we 
>>>>>>> need to implement encode by ISO 8859-1.
>>>>>>>
>>>>>>> 2.
>>>>>>> According to help, the feature of VM.system_properties is just 
>>>>>>> "Print system properties". The users should not use this output 
>>>>>>> for loading. The users use it when they want to see System 
>>>>>>> Properties soon.
>>>>>>>
>>>>>>> Regards,
>>>>>>> Chihiro
>>>>>>>
>>>>>>>
>>>>>>> 2020年2月26日(水) 18:53 Schmelter, Ralf<ralf.schmelter at sap.com>:
>>>>>>>> Hi Chihiro,
>>>>>>>>
>>>>>>>> I have two remarks:
>>>>>>>>
>>>>>>>> 1. ISO Latin 1 characters which are not ASCII will not work 
>>>>>>>> with the code. While the Properties.store() method claims to 
>>>>>>>> create ISO Latin 1 String, it really only will create printable 
>>>>>>>> ASCII characters (apart from the comment, but it is ASCII too 
>>>>>>>> in this case). See Properties.saveConvert, where the char is 
>>>>>>>> checked for < 0x20 or > 0x7e and then printed as \uxxxx. This 
>>>>>>>> is important, since the bytes of the ByteArrayOutputStream are 
>>>>>>>> then send to the jcmd. And jcmd expects UTF-8 encoded strings, 
>>>>>>>> which is OK if we only used ASCII characters. But a ISO Latin 1 
>>>>>>>> character >= 0x80 will break the encoding. Just try using 
>>>>>>>> \u00DC in your test.
>>>>>>>>
>>>>>>>> 2. Your change makes it impossible to load the output with 
>>>>>>>> properties.load(). The old output could be loaded, since it was 
>>>>>>>> a valid properties file. But yours is not. For example, 
>>>>>>>> consider the filename c:\test\new. Formerly it would be encoded 
>>>>>>>> as:
>>>>>>>> C\:\\test\\new
>>>>>>>> And now it is:
>>>>>>>> C:\test\new
>>>>>>>> But the properties code would see "\n" as the newline character 
>>>>>>>> in your encoding. In fact you cannot differentiate between \n, 
>>>>>>>> \t, \f and \r originally being one or two characters.
>>>>>>>>
>>>>>>>> Best regards,
>>>>>>>> Ralf
>>>>>>>>
>>>>>>>>
>>>>>>>> From: 
>>>>>>>> serviceability-dev<serviceability-dev-bounces at openjdk.java.net> 
>>>>>>>> On Behalf Of Chihiro Ito
>>>>>>>> Sent: Dienstag, 25. Februar 2020 04:45
>>>>>>>> To:serguei.spitsyn at oracle.com
>>>>>>>> Cc:serviceability-dev at openjdk.java.net
>>>>>>>> Subject: Re: RFR: JDK-8222489: jcmd VM.system_properties gives 
>>>>>>>> unusable paths on Windows
>>>>>>>>
>>>>>>>> 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
>>>>>>>>
>