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

[Marcus Larsson]

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
>