RFR: 8157948: UL allows same log file with multiple file=

[Marcus Larsson]

Hi,


On 09/05/2016 11:56 AM, Dmitry Samersoff wrote:
> Marcus,
>
>> Not sure why that wouldn't work.
> Below is output of simple unit test, is it expected behavior?
>
> NORMALIZE: {file="/tmp/=testfile"} => {file=/tmp/=testfile}
> NORMALIZE: {file=/tmp/=testfile} => {file=/tmp/=testfile}
>
> NORMALIZE: {file==testfile} => {file==testfile}
> NORMALIZE: {file="=testfile"} => {file==testfile}
>
> NORMALIZE: {"=testfile"} => {file==testfile}
> NORMALIZE: {=testfile} => {=testfile}
>
> NORMALIZE: {file=stderr} => {file=stderr}
> NORMALIZE: {file="stderr"} => {file=stderr}
>
> NORMALIZE: {stderr} => {stderr}
> NORMALIZE: {"stderr"} => {file=stderr}

Yes, that is how I expect it to work. Do you find anything odd with it?

>> It might be valid, but it could just as well be rewritten as
>> "/var/tmp/my file with space", so I don't see the value in supporting
>> that syntax.
> Output filename might be a result of env variable combination, so I
> think it's better to handle quotes the same way as the rest of the unix
> tools do.

Hmm, remember that quotes in command lines such as:

java -Xlog::"file with space"

will typically be stripped from quotes before passed to the VM, so it's 
not really an issue for us.

What we're talking about here is really the case when someone types:

java -Xlog::\"my:file\"

for which we should avoid parsing the colon as a separator, and not 
include the quotes in the actual name.

Thanks,
Marcus

>
> -Dmitry
>
> On 2016-09-05 10:36, Marcus Larsson wrote:
>> Hi Dmitry,
>>
>>
>> On 09/01/2016 03:51 PM, Dmitry Samersoff wrote:
>>> Marcus,
>>>
>>> logConfiguration.cpp:
>>>
>>>    116 const char* start_quote = strchr(full_name, '"');
>>>    117 const char* equals = strchr(full_name, '=');
>>>
>>>    121   // ignore equals sign within quotes
>>>    122   if (quoted && equals > start_quote) {
>>>    123     equals = NULL;
>>>    124   }
>>>
>>> file="/var/tmp/=myfile" most likely will not be parsed correctly.
>> Not sure why that wouldn't work. There's an equals sign before the
>> quote, so we correctly parse the type to be file, and the name to be
>> "/var/tmp/=myfile". If we would leave out the 'file=', then the first
>> equals sign comes after the quote, so the if-check above will be true
>> and we will simply pretend we didn't see the equals.
>>
>>>    147: You prohibit a construction like:
>>>
>>>         /var/tmp/"my file with space"
>>>
>>>     that is perfectly valid.
>> It might be valid, but it could just as well be rewritten as
>> "/var/tmp/my file with space", so I don't see the value in supporting
>> that syntax.
>>
>> Thanks,
>> Marcus
>>
>>> -Dmitry
>>>
>>> On 2016-08-26 15:11, Marcus Larsson wrote:
>>>> Hi David,
>>>>
>>>> Thanks for looking at this!
>>>>
>>>> New webrev:
>>>> http://cr.openjdk.java.net/~mlarsson/8157948/webrev.01/
>>>>
>>>> Incremental:
>>>> http://cr.openjdk.java.net/~mlarsson/8157948/webrev.00-01/
>>>>
>>>> See replies below.
>>>>
>>>> On 08/26/2016 03:44 AM, David Holmes wrote:
>>>>> Hi Marcus,
>>>>>
>>>>> We really need a better way to specify and verify these mini-grammars
>>>>> for command-line options. :(
>>>> Yeah, I'm all for something like that.
>>>>
>>>>> On 25/08/2016 7:31 PM, Marcus Larsson wrote:
>>>>>> Hi,
>>>>>>
>>>>>> Please review the following patch to fix the issue where you could
>>>>>> have
>>>>>> the same file added twice as different log outputs in UL if it had the
>>>>>> "file=" prefix or if it was quoted. Log output names are now
>>>>>> normalized
>>>>>> during log argument parsing to ensure they are always normalized when
>>>>>> finding existing or adding new outputs.
>>>>> So does this mean that whereas today
>>>>>
>>>>> -Xlog:gc=debug:foo
>>>>>
>>>>> assumes foo is the log file, with this fix you will get an error?
>>>> No, the file= prefix will be assumed just like before. The parse step
>>>> will now explicitly add it in the case that it wasn't specified. So
>>>> every LogFileOutput instance created will have the prefix in its name.
>>>>
>>>>>> Webrev:
>>>>>> http://cr.openjdk.java.net/~mlarsson/8157948/webrev.00/
>>>>> src/share/vm/logging/logFileOutput.cpp
>>>>>
>>>>> Suggestion:
>>>>>
>>>>>      const char* prefix = "file=";
>>>>>      assert(strstr(name, prefix) == name, "invalid output name '%s':
>>>>> missing prefix: %s", name, prefix);
>>>>>      _file_name = make_file_name(name + strlen(prefix), _pid_str,
>>>>> _vm_start_time_str);
>>>> Fixed, see below.
>>>>
>>>>> ---
>>>>>
>>>>> src/share/vm/logging/logConfiguration.cpp
>>>>>
>>>>> Suggestion:
>>>>>
>>>>> static const char* prefix = "file=";
>>>> I've refactored all "file=" literals into constants, but I made the
>>>> constant a field of LogFileOutput. I think it fits better there, let me
>>>> know if you think otherwise.
>>>>
>>>>> In normalize_output_name it is hard for me to work out what the
>>>>> possible "grammar" is, or how different cases will be handled.
>>>>> Currently -Xlog:gc=debug:"file"=foo is treated as
>>>>> -Xlog:gc=debug:file=foo. But with your changes I think the quoting
>>>>> will be handled differently.
>>>> Actually -Xlog:gc=debug:"file"=foo should give an error, since quoting
>>>> the output types isn't supported (only the name can be quoted). This
>>>> should just be a refactoring to make sure we're always managing the
>>>> output names in a uniform manner (so that file="foo" and file=foo isn't
>>>> treated as two different log outputs).
>>>>
>>>> BTW, take care if you're testing this on the command line, as the shell
>>>> might be stripping away quotes in the arguments for you.
>>>>
>>>> Thanks,
>>>> Marcus
>>>>
>>>>> Thanks,
>>>>> David
>>>>>
>>>>>> Issue:
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8157948
>>>>>>
>>>>>> Testing:
>>>>>> New unit test through JPRT
>>>>>>
>>>>>> Thanks,
>>>>>> Marcus
>