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

David Holmes david.holmes at oracle.com
Tue Aug 30 03:50:43 UTC 2016


On 29/08/2016 10:47 PM, Marcus Larsson wrote:
> 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.

It seems unlikely we would ever change this, but I'm fine either way.

Thanks,
David

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


More information about the hotspot-runtime-dev mailing list