RFR: 8271003: hs_err improvement: handle CLASSPATH env setting longer than O_BUFLEN [v2]

[Calvin Cheung]

On Tue, 3 Aug 2021 16:14:57 GMT, Calvin Cheung <ccheung at openjdk.org> wrote:

>> Please review this small enhancement for addressing the problem of the CLASSPATH env variable setting being truncated in a hs err log.
>> 
>> For printing a char string, it doesn't need to go through `do_vsnprintf()` which does the truncation based on the input buffer length. The change is local to the code path pertaining to hs err log.
>> 
>> Testing:
>> 
>> - [x] tiers 1, 2 (including the new test)
>
> Calvin Cheung has updated the pull request incrementally with one additional commit since the last revision:
> 
>   @tstuefe comments

Hi Thomas,

Thanks for your review.

> Hi Calvin,
> 
> Sorry, I think we cannot do it the way you propose.
> 
> The problem is that the print_cr() functions are supposed to write the newline _with the same `write(2)` call_. That is not documented in do_vsnprintf(), it's an easy to miss a detail.
> 
> Writing with one single `write(2)` call guarantees some atomicity when writing concurrently from several threads. If you write the newline separately, you risk line tear - the newline appearing away from its line.
> 
> This means that `print_cr("%s", s)` should not write the newline separately and therefore would not benefit from your improvement. And `print("%s", s)` would already do exactly what you propose, since in `do_vsnprintf()` we already handle the two special cases.
> 
> I propose a specific fix for your problem in `os::print_environment_variables()` by replacing `st->print_cr("%s", envvar);` with either one of
> 
> ```
> st->print("%s", envvar);
> st->cr()
> ```
> 

I picked the above since it is more consistent with another `st->print()` in the same loop.

> or
> 
> ```
> st->print_raw(envvar);
> st->cr()
> ```
> 
> The test is useful though. But you can simplify it:
> 
> * as David said, use `-XX:ErrorHandlerTest`. For better realistic testing, I'd use an option which gives you a real signal, e.g. `-XX:ErrorHandlerTest=14`
> * just start the test with `-XX:+ErrorFileToStdout`. We added this option to pipe the error file to stdout. That way you can parse stdout directly, using OutputScanner methods and regex, no need to open and parse the hs-err file:
> 
> ```

I've updated the test and using `OutputAnalyzer.firstMatch()` to parse the stdout.

Thanks,
Calvin

> thomas at starfish$ CLASSPATH=abcabc ./images/jdk/bin/java -XX:ErrorHandlerTest=14 -XX:+ErrorFileToStdout | grep CLASSPATH
> CLASSPATH=abcabc
> ```
> 
> Cheers, Thomas

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

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