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

[Dmitry Samersoff]

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.

 147: You prohibit a construction like:

      /var/tmp/"my file with space"

  that is perfectly valid.

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