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

[Dmitry Samersoff]

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}

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

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