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

[Marcus Larsson]

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