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

[Harold David Seigel]

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

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.


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.

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20181221/d7833ce7/attachment.html>