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

[Marcus Larsson]

Hi,


On 08/29/2016 03:45 AM, David Holmes wrote:
> Hi Marcus,
>
> On 26/08/2016 10:11 PM, 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/
>
> src/share/vm/logging/logConfiguration.cpp
>
> Why bother with implicit_output_prefix instead of using 
> LogFileOutput::Prefix directly? You do use the latter in two places so 
> I find the inconsistency strange.

The two instances of direct usage of LogFileOutput::Prefix are not 
related to the implicit prefix, which is why I don't use the constant 
there. I wanted the implicit_output_prefix constant to improve 
readability and make it easy to switch the default, should we ever want 
to. I'm fine with removing it if you think that's better.

>
>> See replies below.
>
> Follow up 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.
>
> Ok.
>
>>>
>>>> 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.
>
> Placement is fine.
>
>>>
>>> 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.
>
> Yes you are right it was stripping them away - it is an error.

Great!

Thanks,
Marcus

>
> Thanks,
> David
>
>>
>> Thanks,
>> Marcus
>>
>>>
>>> Thanks,
>>> David
>>>
>>>> Issue:
>>>> https://bugs.openjdk.java.net/browse/JDK-8157948
>>>>
>>>> Testing:
>>>> New unit test through JPRT
>>>>
>>>> Thanks,
>>>> Marcus
>>