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

[Marcus Larsson]

Thanks Robbin!

Marcus


On 09/01/2016 02:43 PM, Robbin Ehn wrote:
> Hi Marcus,
>
> On 08/26/2016 02:11 PM, Marcus Larsson wrote:
>> Hi David,
>>
>> Thanks for looking at this!
>>
>> New webrev:
>> http://cr.openjdk.java.net/~mlarsson/8157948/webrev.01/
>
> Looks good, thanks!
>
> /Robbin
>
>>
>> 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
>>