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

[David Holmes]

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.

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

Thanks,
David

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