Hi David,
Thanks for the review. I pushed the fix.
I think the existing test works because the double quoted arguments are
passed directly to the java process, with no intervening shell, and so
the double quotes are not removed. When running the same command from
the Windows prompt, the shell is stripping out the enclosing double
quotes before passing the -Xlog args to Java.
Harold
On 12/21/2018 5:08 PM, David Holmes wrote:
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
