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
@@ -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(NewLineOptionKey, 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::NewLineOptionKey = "newline";

static bool initialized;
static union {
char stdoutmem[sizeof(LogStdoutOutput)];
@@ -51,6 +53,33 @@ 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(NewLineOptionKey, key) == 0) {
_new_line = os::strdup_check_oom(value_str, mtLogging);
success = true;
} 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 +146,29 @@ bool LogFileStreamOutput::flush() {
total += result; \
}

int LogFileStreamOutput::write_internal(const char* msg) {
int written = 0;
if (_new_line == NULL) {
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 = strchr(cur, '\n');
if (next == NULL) {
WRITE_LOG_WITH_RESULT_CHECK(jio_fprintf(_stream, "%s\n", cur), written);
} else {
*next = '\0';
WRITE_LOG_WITH_RESULT_CHECK(jio_fprintf(_stream, "%s%s", cur, _new_line), 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 +178,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 +193,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:
char* _new_line;
bool _write_error_is_shown;

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

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

public:

virtual ~LogFileStreamOutput() {
if (_new_line != NULL) {
os::free(_new_line);
}
}

virtual bool initialize(const char* options, outputStream* errstream);
virtual int write(const LogDecorations& decorations, const char* msg);
virtual int write(LogMessageBuffer::Iterator msg_iterator);
};
@@ -4391,8 +4391,8 @@ 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]newline=\f[R]\f[I]<chars to replace newline char (\\n)>\f[R]
.RE
.RE
.SS Default Configuration