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

Dmitry Samersoff dmitry.samersoff at oracle.com
Mon Sep 5 13:53:02 UTC 2016


Marcus,

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

NORMALIZE: {"=testfile"} => {file==testfile}
NORMALIZE: {=testfile} => {=testfile}

NORMALIZE: {stderr} => {stderr}
NORMALIZE: {"stderr"} => {file=stderr}

The reason of difference above is not obvious for me.

Could you add a comment to the code that explains
correct syntax and expected normalization result?

-Dmitry


On 2016-09-05 16:44, Marcus Larsson wrote:
> 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
>>
> 


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.


More information about the hotspot-runtime-dev mailing list