RFR 8215398: -Xlog option usage => Invalid decorator '\temp\app_cds.log'.

[David Holmes]

Hi Harold,

On 22/12/2018 4:32 am, Harold David Seigel wrote:
> Hi David,
> 
> Here is the latest webrev.  The only difference between this and the 
> previous webrev is the addition of "@bug 8215398" to the test:
> 
>     http://cr.openjdk.java.net/~hseigel/bug_8215398.3/webrev/index.html

Okay.

> 
> The point that I'm trying to make about logging's handling of single 
> quotes vs. double quotes can perhaps best be shown on Linux:
> 
> If I execute the following commands (on Linux):
> 
>     java -Xlog:safepoint=trace:\"dquotes.log\" -version
> 
>     java -Xlog:safepoint=trace:\'squotes.log\' -version
> 
> and then list out the current directory:
> 
>      > ls
>     'squotes.log'  dquotes.log
> 
> it shows that logging stripped off the double quotes, but not the single 
> quotes.

I see - that is very odd behaviour.

> With my fix, these commands succeed on Windows even when run from the 
> Windows shell:
> 
>     java -Xlog:safepoint=trace:d:\safepointtrace.txt -version
>     java -Xlog:safepoint=trace:"d:\safepointtrace.txt" -version
>     java -Xlog:safepoint=trace:"d:\safepoint trace.txt" -version
> 
> This one continues to fail because of the single quote issue described 
> above:
> 
>     java -Xlog:safepoint=trace:'d:\safepointtrace.txt' -version
> 
> I hope we can move forward with this change.

I don't have an issue with your fix. I just don't understand how the 
existing test actually works when the test cases seems to fail at the 
command-line for me.

Thanks,
David

> Thanks, Harold
> 
> 
> On 12/20/2018 9:28 PM, David Holmes wrote:
>> On 21/12/2018 7:30 am, Harold David Seigel wrote:
>>> Hi David,
>>>
>>> Thanks for looking at this!
>>>
>>> Please review this updated webrev.  The fix is the same but the 
>>> webrev contains a new test instead of modifying an existing test:
>>>
>>> http://cr.openjdk.java.net/~hseigel/bug_8215398.2/webrev/
>>
>> I think renaming the existing test would be better as the only 
>> difference is the use of quotes. But if you want a new test please add:
>>
>> @bug 8215398
>>
>>> The logging implementation does not handle single quotes as 
>>> expected.  For example, if the TestQuotedLogOutputs.java test is 
>>> changed to use single quotes instead of double quotes, it will fail, 
>>> even on Linux, because the single quotes are included as part of the 
>>> file name.  They are not stripped off.  That is why "java 
>>> -Xlog:safepoint=trace:'d:\safepointtrace.txt' -version" fails.
>>
>> It fails with double-quotes too! AFAICS it always fails in a Windows 
>> command shell. Maybe the test only passes in cygwin shell?
>>
>> That said I don't understand your comment. I expect the quotes to 
>> delimit the filename in case the filename has spaces in the path 
>> component. If they aren't stripped off it wouldn't be tokenizing at 
>> the internal ":", so ... ???
>>
>> Thanks,
>> David
>>
>>> Perhaps a new bug is needed to change logging's handling of single 
>>> quotes?  It's a bit surprising that this issue hasn't already been 
>>> reported.
>>>
>>> Thanks, Harold
>>>
>>> On 12/20/2018 2:07 AM, David Holmes wrote:
>>>> Hi Harold,
>>>>
>>>> On 19/12/2018 11:22 pm, Harold David Seigel wrote:
>>>>> Hi,
>>>>>
>>>>> Please review this small change to fix JDK-8215398.  The fix works 
>>>>> by not treating ':' as a delimiter in a -Xlog... option string if 
>>>>> it is following by a '\' and preceded by either a single character 
>>>>> or the text 'file='.  The fix is for Windows only.
>>>>>
>>>>> Open Webrev: 
>>>>> http://cr.openjdk.java.net/~hseigel/bug_8215398/webrev/index.html
>>>>
>>>> I think I can follow the fix.
>>>>
>>>> But I'm a bit concerned about the test. AFAICS the test thought it 
>>>> was already testing this case - albeit with the path quoted:
>>>>
>>>>   42         // Ensure log files can be specified with full path.
>>>>   43         // On windows, this means that the file name will contain
>>>>   44         // a colon ('C:\log.txt' for example), which is used to
>>>>   45         // separate -Xlog: options 
>>>> (-Xlog:tags:filename:decorators).
>>>>   46         // Try to log to a file in our current directory, using 
>>>> its absolute path.
>>>>   47         String baseName = "test file.log";
>>>>   48         Path filePath = Paths.get(baseName).toAbsolutePath();
>>>>   49         String fileName = filePath.toString();
>>>>   50         File file = filePath.toFile();
>>>> ...
>>>>   65         String[] validOutputs = new String[] {
>>>>   66             quote + fileName + quote,
>>>>   67             "file=" + quote + fileName + quote,
>>>>   68             quote + fileName + quote + ":",
>>>>   69             quote + fileName + quote + "::"
>>>>   70         };
>>>>
>>>> But even quoted if I specify the drive designator in the path, it 
>>>> fails! Here's a local attempt at this:
>>>>
>>>> D:\ade> apps\Java\jdk-11\fastdebug\bin\java 
>>>> -Xlog:safepoint=trace:'d:\safepointtrace.txt' -version
>>>> [0.014s][error][logging] Invalid decorator '\safepointtrace.txt''.
>>>> Invalid -Xlog option 
>>>> '-Xlog:safepoint=trace:'d:\safepointtrace.txt'', see error log for 
>>>> details.
>>>> Error: Could not create the Java Virtual Machine.
>>>> Error: A fatal exception has occurred. Program will exit.
>>>>
>>>> So AFAICS the test can't possibly have been testing what it thought 
>>>> it was testing! ???
>>>>
>>>> I'm also not sure about just adding some unquoted variants to the 
>>>> existing TestQuotedLogOutputs without renaming the test and updating 
>>>> the @summary
>>>>
>>>> Thanks,
>>>> David
>>>>
>>>>> JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8215398
>>>>>
>>>>> The fix was regression tested by running Mach5 tiers 1 and 2 tests 
>>>>> and builds on Linux-x64, Windows, and Mac OS X, running tiers 3-5 
>>>>> tests on Linux-x64, running JCK-12 Lang and VM tests on Linux-x64, 
>>>>> and by hand on Windows.
>>>>>
>>>>> Thanks, Harol
>>>>>
>>>
>