RFR: 8294402: Add diagnostic logging to VMProps.checkDockerSupport [v7]

Leonid Mesnik lmesnik at openjdk.org
Thu Feb 9 19:25:48 UTC 2023


On Wed, 8 Feb 2023 23:18:20 GMT, Mikhailo Seledtsov <mseledtsov at openjdk.org> wrote:

>> Add log() and logToFile() methods to VMProps for diagnostic purposes.
>
> Mikhailo Seledtsov has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Re-added conditional logging per review discussion

Marked as reviewed by lmesnik (Reviewer).

test/jtreg-ext/requires/VMProps.java line 516:

> 514: 
> 515:     // Returns comma-separated file names for stdout and stderr.
> 516:     private String redirectOutputToLogFile(String msg, ProcessBuilder pb, String fileNameBase) {

It would be better to return List<String> fileNames rather to combine/parse the String.

test/jtreg-ext/requires/VMProps.java line 517:

> 515:     // Returns comma-separated file names for stdout and stderr.
> 516:     private String redirectOutputToLogFile(String msg, ProcessBuilder pb, String fileNameBase) {
> 517:         if (!Boolean.getBoolean("jtreg.log.vmprops")) {

Why not dump output always? It is printed only if exitcode is not zero anyway and doesn't add noise.
However not having it in the case of failure require reproducing the bug.

test/jtreg-ext/requires/VMProps.java line 693:

> 691:     protected static void log(String msg) {
> 692:         if (!Boolean.getBoolean("jtreg.log.vmprops")) {
> 693:             return;

Why not always log in to the file like vmprops.log? Also, add try/catch into the call() and redirect file to stderr if something is wrong. And redirect if the property is set for passed tests.

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

PR: https://git.openjdk.org/jdk/pull/12239


More information about the hotspot-dev mailing list