Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8271186: Add UL option to replace newline char #4885

Closed
wants to merge 10 commits into from
@@ -564,12 +564,20 @@ void LogConfiguration::print_command_line_help(outputStream* out) {
out->print_cr(" file=<filename>");
out->print_cr(" If the filename contains %%p and/or %%t, they will expand to the JVM's PID and startup timestamp, respectively.");
out->print_cr(" Additional output-options for file outputs:");
out->print_cr(" filesize=.. - Target byte size for log rotation (supports K/M/G suffix)."
" If set to 0, log rotation will not trigger automatically,"
" but can be performed manually (see the VM.log DCMD).");
out->print_cr(" filecount=.. - Number of files to keep in rotation (not counting the active file)."
" If set to 0, log rotation is disabled."
" This will cause existing log files to be overwritten.");
out->print_cr(" filesize=.. - Target byte size for log rotation (supports K/M/G suffix)."
" If set to 0, log rotation will not trigger automatically,"
" but can be performed manually (see the VM.log DCMD).");
out->print_cr(" filecount=.. - Number of files to keep in rotation (not counting the active file)."
" If set to 0, log rotation is disabled."
" This will cause existing log files to be overwritten.");
out->print_cr(" foldmultilines=.. - If set to true, a log event that consists of multiple lines"
" will be folded into a single line by replacing newline characters"
" with the sequence '\\' and 'n' in the output."
" Existing single backslash characters will also be replaced"
" with a sequence of two backslashes so that the conversion can be reversed."
" This option is safe to use with UTF-8 character encodings,"
" but other encodings may not work.");

out->cr();
out->print_cr("\nAsynchronous logging (off by default):");
out->print_cr(" -Xlog:async");
@@ -195,7 +195,16 @@ bool LogFileOutput::parse_options(const char* options, outputStream* errstream)
char* value_str = equals_pos + 1;
*equals_pos = '\0';

if (strcmp(FileCountOptionKey, key) == 0) {
if (strcmp(FoldMultilinesOptionKey, key) == 0) {
// We need to pass <key>=<value> style option to LogFileStreamOutput::initialize().
// Thus we restore '=' temporally.
*equals_pos = '=';
success = LogFileStreamOutput::initialize(pos, errstream);
*equals_pos = '\0';
if (!success) {
Copy link
Member

@iklam iklam Aug 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the code can be simplified by parsing the value here:

bool fold_multilines = ....;
LogFileStreamOutput::set_fold_multilines(foldmultilines);

Copy link
Member Author

@YaSuenag YaSuenag Aug 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

foldmultilines is a parameter which should be applied in initialization. LogOutput and extended classes of it look like to have a rule to apply initialization parameter at initialize().
We can simplify the code as you said if we can apply foldmultiline for LogFileStreamOutput at LogFileOutput::initialize(), but I concern it breaks the semantics for UL implementation.

Copy link
Member

@iklam iklam Aug 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand correctly, your concern is _fold_multilines is a property of LogFileStreamOutput, so the parsing of this option should be done inside the LogFileStreamOutput class. That way, (in the future) all subclasses of LogFileStreamOutput will be able to handle foldmultiline=true in their options by deferring to LogFileStreamOutput::initialize.

I suppose this would be useful if we have support for sockets in the future, like

-Xlog:all=socket::address=10.1.1.1,port=1234,foldmultiline=true

However, I don't think your current implementation has the right abstraction -- FoldMultilinesOptionKey is now checked both in LogFileStreamOutput::initialize() and LogFileOutput::initialize(). If we add a new LogSocketOutput in the future, we would have to duplicated the code in LogSocketOutput::initialize() as well.

I think we should defer the design of such an abstraction until we need it. Ideally, LogFileOutput and LogSocketOutput should not need to know what options are supported by their superclass, LogFileStreamOutput

For the time being, it's easier to parse all the file output options in LogFileOutput::initialize() to keep the code simple.

Copy link
Member Author

@YaSuenag YaSuenag Aug 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think your current implementation has the right abstraction

Agree, I think we can (should) improve more. Its improvement is not for LogSocketStream but also for LogStd{out,err}Output.
As we discussed in CSR about stdout/err, we will work for them in next step. But they extend LogFileStreamOutput directly. So I want to refactor it before next step.

Anyway I moved foldmultiline handling into LogFileOutput in new commit.

break;
}
} else if (strcmp(FileCountOptionKey, key) == 0) {
size_t value = parse_value(value_str);
if (value > MaxRotationFileCount) {
errstream->print_cr("Invalid option: %s must be in range [0, %u]",
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2015, 2020, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2015, 2021, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -30,6 +30,8 @@
#include "memory/allocation.inline.hpp"
#include "utilities/defaultStream.hpp"

const char* const LogFileStreamOutput::FoldMultilinesOptionKey = "foldmultilines";

static bool initialized;
static union {
char stdoutmem[sizeof(LogStdoutOutput)];
@@ -51,6 +53,40 @@ LogFileStreamInitializer::LogFileStreamInitializer() {
}
}

bool LogFileStreamOutput::initialize(const char* options, outputStream* errstream) {
if (options == NULL || strlen(options) == 0) {
return true;
}

char* opts = os::strdup_check_oom(options, mtLogging);
char* equals_pos = strchr(opts, '=');
bool success = false;
if (equals_pos == NULL) {
errstream->print_cr("Invalid option '%s' for log file stream output.", opts);
} else {
char* key = opts;
char* value_str = equals_pos + 1;
*equals_pos = '\0';

if (strcmp(FoldMultilinesOptionKey, key) == 0) {
if (strcmp(value_str, "true") == 0) {
_fold_multilines = true;
success = true;
} else if (strcmp(value_str, "false") == 0) {
_fold_multilines = false;
success = true;
} else {
errstream->print_cr("Invalid option '%s' for %s.", value_str, FoldMultilinesOptionKey);
}
} else {
errstream->print_cr("Invalid option '%s' for log file stream output.", options);
}
}

os::free(opts);
return success;
}

int LogFileStreamOutput::write_decorations(const LogDecorations& decorations) {
int total_written = 0;
char buf[LogDecorations::max_decoration_size + 1];
@@ -117,6 +153,30 @@ bool LogFileStreamOutput::flush() {
total += result; \
}

int LogFileStreamOutput::write_internal(const char* msg) {
int written = 0;
if (!_fold_multilines) {
WRITE_LOG_WITH_RESULT_CHECK(jio_fprintf(_stream, "%s\n", msg), written);
} else {
char *dupstr = os::strdup_check_oom(msg, mtLogging);
char *cur = dupstr;
char *next;
do {
next = strpbrk(cur, "\n\\");
if (next == NULL) {
WRITE_LOG_WITH_RESULT_CHECK(jio_fprintf(_stream, "%s\n", cur), written);
} else {
const char *found = (*next == '\n') ? "\\n" : "\\\\";
*next = '\0';
WRITE_LOG_WITH_RESULT_CHECK(jio_fprintf(_stream, "%s%s", cur, found), written);
cur = next + 1;
}
} while (next != NULL);
os::free(dupstr);
}
return written;
}

int LogFileStreamOutput::write(const LogDecorations& decorations, const char* msg) {
const bool use_decorations = !_decorators.is_empty();

@@ -126,7 +186,7 @@ int LogFileStreamOutput::write(const LogDecorations& decorations, const char* ms
WRITE_LOG_WITH_RESULT_CHECK(write_decorations(decorations), written);
WRITE_LOG_WITH_RESULT_CHECK(jio_fprintf(_stream, " "), written);
}
WRITE_LOG_WITH_RESULT_CHECK(jio_fprintf(_stream, "%s\n", msg), written);
written += write_internal(msg);

return flush() ? written : -1;
}
@@ -141,7 +201,7 @@ int LogFileStreamOutput::write(LogMessageBuffer::Iterator msg_iterator) {
WRITE_LOG_WITH_RESULT_CHECK(write_decorations(msg_iterator.decorations()), written);
WRITE_LOG_WITH_RESULT_CHECK(jio_fprintf(_stream, " "), written);
}
WRITE_LOG_WITH_RESULT_CHECK(jio_fprintf(_stream, "%s\n", msg_iterator.message()), written);
written += write_internal(msg_iterator.message());
}

return flush() ? written : -1;
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2015, 2020, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2015, 2021, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -26,6 +26,7 @@

#include "logging/logDecorators.hpp"
#include "logging/logOutput.hpp"
#include "runtime/os.hpp"
#include "utilities/globalDefinitions.hpp"

class LogDecorations;
@@ -41,12 +42,16 @@ static LogFileStreamInitializer log_stream_initializer;
// Base class for all FileStream-based log outputs.
class LogFileStreamOutput : public LogOutput {
private:
bool _fold_multilines;
bool _write_error_is_shown;

int write_internal(const char* msg);
protected:
static const char* const FoldMultilinesOptionKey;
FILE* _stream;
size_t _decorator_padding[LogDecorators::Count];

LogFileStreamOutput(FILE *stream) : _write_error_is_shown(false), _stream(stream) {
LogFileStreamOutput(FILE *stream) : _fold_multilines(false), _write_error_is_shown(false), _stream(stream) {
for (size_t i = 0; i < LogDecorators::Count; i++) {
_decorator_padding[i] = 0;
}
@@ -56,6 +61,7 @@ class LogFileStreamOutput : public LogOutput {
bool flush();

public:
virtual bool initialize(const char* options, outputStream* errstream);
virtual int write(const LogDecorations& decorations, const char* msg);
virtual int write(LogMessageBuffer::Iterator msg_iterator);
};
@@ -4418,8 +4418,12 @@ selected.
\f[I]output\-options\f[R] is
.RS
.PP
\f[CB]filecount=\f[R]\f[I]file\-count\f[R] \f[CB]filesize=\f[R]\f[I]file size
with optional K, M or G suffix\f[R]
\f[CB]filecount=\f[R]\f[I]file\-count\f[R] \f[CB]filesize=\f[R]\f[I]<file size
with optional K, M or G suffix>\f[R] \f[CB]foldmultilines=\f[R]\f[I]<true|false>\f[R]
.RE
.PP
When \f[I]foldmultilines\f[R] is true, a log event that consists of multiple lines will be folded into a single line by replacing newline characters with the sequence '\\' and 'n' in the output. Existing single backslash characters will also be replaced with a sequence of two backslashes so that the conversion can be reversed. This option is safe to use with UTF-8 character encodings, but other encodings may not work. For example, it may happen inadvertently conversion in multi-byte sequences in Shift JIS and BIG5.
This option is available only for file outputs.
Copy link
Member

@iklam iklam Aug 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about: "For example, it may incorrectly convert multi-byte sequences in Shift JIS and BIG5."

Copy link
Member Author

@YaSuenag YaSuenag Aug 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Ioi! I updated manpage, and also I formatted the description - it was too long than other lines!

.RE
.RE
.SS Default Configuration
@@ -0,0 +1,95 @@
/*
* Copyright (c) 2021, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2021 NTT DATA.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
* under the terms of the GNU General Public License version 2 only, as
* published by the Free Software Foundation.
*
* This code is distributed in the hope that it will be useful, but WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
* version 2 for more details (a copy is included in the LICENSE file that
* accompanied this code).
*
* You should have received a copy of the GNU General Public License version
* 2 along with this work; if not, write to the Free Software Foundation,
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
*
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
* or visit www.oracle.com if you need additional information or have any
* questions.
*/

/*
* @test
* @bug 8271186
* @library /test/lib
* @run driver FoldMultilinesTest
*/

import java.nio.file.Files;
import java.nio.file.Path;
import jdk.test.lib.process.OutputAnalyzer;
import jdk.test.lib.process.ProcessTools;

public class FoldMultilinesTest {

private static Path EXCEPTION_LOG_FILE = Path.of("exceptions.log");
private static String XLOG_BASE = "-Xlog:exceptions=info:file=" + EXCEPTION_LOG_FILE.toString();

private static void analyzeFoldMultilinesOn(ProcessBuilder pb) throws Exception {
OutputAnalyzer output = new OutputAnalyzer(pb.start());
output.shouldHaveExitValue(0);

String logs = Files.readString(EXCEPTION_LOG_FILE);
if (!logs.contains("line 1\\nline 2\\\\nstring")) {
throw new RuntimeException("foldmultilines=true did not work.");
}
}

private static void analyzeFoldMultilinesOff(ProcessBuilder pb) throws Exception {
OutputAnalyzer output = new OutputAnalyzer(pb.start());
output.shouldHaveExitValue(0);

String logs = Files.readString(EXCEPTION_LOG_FILE);
if (!logs.contains("line 1" + System.lineSeparator() + "line 2\\nstring")) {
throw new RuntimeException("foldmultilines=false did not work.");
}
}

private static void analyzeFoldMultilinesInvalid(ProcessBuilder pb) throws Exception {
OutputAnalyzer output = new OutputAnalyzer(pb.start());
output.shouldContain("Invalid option 'invalid' for foldmultilines.");
output.shouldNotHaveExitValue(0);
}

public static void main(String[] args) throws Exception {
String Xlog;
ProcessBuilder pb;

Xlog = XLOG_BASE + "::foldmultilines=true";
pb = ProcessTools.createJavaProcessBuilder(Xlog, InternalClass.class.getName());
analyzeFoldMultilinesOn(pb);

Xlog = XLOG_BASE + "::foldmultilines=false";
pb = ProcessTools.createJavaProcessBuilder(Xlog, InternalClass.class.getName());
analyzeFoldMultilinesOff(pb);

Xlog = XLOG_BASE + "::foldmultilines=invalid";
pb = ProcessTools.createJavaProcessBuilder(Xlog, InternalClass.class.getName());
analyzeFoldMultilinesInvalid(pb);
}

public static class InternalClass {
public static void main(String[] args) {
try {
throw new RuntimeException("line 1\nline 2\\nstring");
} catch (Exception e) {
// Do nothing to return exit code 0
}
}
}

}