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

[Daniel D. Daugherty]

The replacement bug should be filed with this description:

     [REDO] 8222489 jcmd VM.system_properties gives unusable paths on 
Windows

and should be linked to the original bug also.

Dan


On 3/11/20 3:52 PM, serguei.spitsyn at oracle.com wrote:
> 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
>>>>>>>>>
>>
>