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

[David Holmes]

Hi Thomas,

On 3/08/2021 2:00 pm, Thomas Stuefe wrote:
> On Fri, 30 Jul 2021 18:17:54 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)
> 
> Hi Calvin,
> 
> Sorry, I think your patch is not a good solution.
> 
> 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(), but maybe we should do that since its 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.

Do we really rely on the write(2) call for atomicity? The defaultStream 
uses a lock and we could pass `add_cr` through to defaultStream::write 
so that the cr is added under the lock.

Cheers,
David
-----

> This means that `print_cr("%s", s)` does not benefit from your improvement. Only `print("%s", s)` would, but here, we do already handle the two special cases in exactly the way you would.
> 
> 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()
> 
> 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:
> 
> 
> thomas at starfish$ CLASSPATH=abcabc ./images/jdk/bin/java -XX:ErrorHandlerTest=14 -XX:+ErrorFileToStdout | grep CLASSPATH
> CLASSPATH=abcabc
> 
> 
> Cheers, Thomas
> 
> -------------
> 
> Changes requested by stuefe (Reviewer).
> 
> PR: https://git.openjdk.java.net/jdk/pull/4947
>