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

[David Holmes]

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