On Thu, 9 Mar 2023 19:53:07 GMT, Roger Riggs <rri...@openjdk.org> wrote:
>> Runtime.exec and ProcessBuilder.start methods create a new operating system
>> process with the program and arguments. Many applications configure a
>> logging subsystem to monitor application events. Logging a process start
>> message with the program, arguments, and stack trace can identify the caller
>> and purpose.
>> Logging the process start event is complementary to the process start event
>> generated for JFR (Java Flight Recorder).
>
> Roger Riggs has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Revise logging of ProcessBuilder.start to support DEBUG and TRACE logging.
> Revise the implNote to include a warning about logging security sensistive
> information.
> DEBUG logs only the command, directory, stack trace, and pid.
> TRACE additionally logs arguments.
LGTM. This looks really useful. We had a similar trace at SAP, a bit more
involved (with env vars), and solved a lot of issues quickly with it.
Small nits remain, up to you if you take my suggestions.
src/java.base/share/classes/java/lang/ProcessBuilder.java line 1155:
> 1153: "cmd: " + cmdargs +
> 1154: ", dir: " + dir +
> 1155: ", pid: " + process.pid(),
It may make sense to revert the printout order, print pid first, cwd second,
then the args, since argument vectors can get very large. Does Logger limit the
line length?
Furthermore - but up to you - when I do printouts like this I always enclose
the command args in quotes. That helps people analyze situations like
accidental trailing spaces in arguments.
Pity that we cannot show at least PATH and LD_LIBRARY_PATH/LIBPATH.
test/jdk/java/lang/ProcessBuilder/ProcessStartLoggingTest.java line 81:
> 79: File nullDirectory = null;
> 80: File thisDirectory = new File(".");
> 81:
I started to heavily use different `@test` sections with speaking names instead
of running a bunch of test in sequence; the advantage is better parallelization
of tests, that I can omit printing out the test name manually (if the test name
itself is speaking), and the ability to start individual tests manually. It
does come with lots of test sections though.
-------------
Marked as reviewed by stuefe (Reviewer).
PR: https://git.openjdk.org/jdk/pull/12862
