RFR: 8273104: Refactoring option parser for UL

[David Holmes]

On Sun, 29 Aug 2021 03:48:06 GMT, Yasumasa Suenaga <ysuenaga at openjdk.org> wrote:

> As we discussed in PR #4885 and in [its CSR](https://bugs.openjdk.java.net/browse/JDK-8271188), we want to introduce `foldmultilines` to stdout/err UL output. However we have no chance to configure `LogOutput` for stdout/err (`LogStdoutOutput`/`LogStderrOutput`) becasuse they will be instantiated statically in logFileStreamOutput.cpp.
> 
> So we need to refactor UL option parser to propagate options to all extended classes of `LogOutput`.
> 
> * Move `parse_option()` to `LogOutput` from `LogFileOutput`, then it becames public member.
> * Introduce `set_option()` in `LogOutput` as a pure virtual member to apply options to each log output, then it is implemented in both `LogFileStreamOutput` and `LogFileOutput`.
> * Move both `FoldMultilinesOptionKey` and `_fold_mulilines` in `LogFileStreamOutput` to private member because they are no longer to need to be public.
> 
> I want to send PR to introduce `foldmultilines` to stdout/err like https://github.com/YaSuenag/jdk/compare/ul-refactoring...YaSuenag:foldmultilines-for-stdout after this.

Hi Yasumasa,

I have one query below about a possible correctness issue with the error messages.

IIUC these dynamic changes to the options will only apply after argument processing is complete - so anything logged prior to that will not be affected. That isn't an issue for options that don't directly change the output but for FoldMultiLines it means we would not do the folding initially. That probably isn't a practical concern given what logging can happen prior to argument processing ... or am I mistaken in thinking that UL can be used prior to that point?

Thanks,
David

src/hotspot/share/logging/logOutput.cpp line 370:

> 368:     if (!success) {
> 369:       if (errstream->count() > errstream_count_before) {
> 370:         errstream->print_cr("Invalid option '%s' for log output (%s).", key, name());

I don't quite follow this. If the error count has increased then this was a known option with a bad value that has already been reported in set_option. Here we should be catching an unknown option that wasn't recognized by set_option, in which case the error count should be unchanged no?

-------------

PR: https://git.openjdk.java.net/jdk/pull/5293