Skip to content

[all] all simulation output written to a single configurable directory#783

Open
EskoDijk wants to merge 1 commit into
openthread:mainfrom
EskoDijk:pr-cli-logfile
Open

[all] all simulation output written to a single configurable directory#783
EskoDijk wants to merge 1 commit into
openthread:mainfrom
EskoDijk:pr-cli-logfile

Conversation

@EskoDijk
Copy link
Copy Markdown
Contributor

@EskoDijk EskoDijk commented Apr 16, 2026

For running multiple OTNS simulations in sequence, an issue is that presently output files from previous runs may be overwritten. Not all output artefacts are stored in the 'tmp' directory also. This PR ensures that all output files are written to into a single, configurable directory. The default name is 'tmp' but this can be changed by the commandline argument -output. Furthermore, also the OTNS main log which was previously not saved is stored as a file in this directory.

Particular commandline argument changes:
-logfile is changed to -lognode which better expresses its purpose: the log level for OT node logs. -noreplay is changed to -replay, with the default being that the replay file is not generated. Typically,
this file is not used, so now it needs to be explicitly requested.

For logging to terminal, there's now an improved check for whether stdout is a terminal (accepting ANSI sequences) or not a terminal. The dispatcher socket is also stored in the output directory. Because this might be a long name, and socket paths are limited, there's a length check added also.

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request refactors the logging and output management system, introducing a configurable output directory for logs, PCAP files, and simulation replays. Key changes include a revamped logger with file support, updated CLI arguments in otns_main, and adjustments to the dispatcher and simulation logic to respect the new output structure. Feedback highlights a potential compilation error in ensureOutputDirExists due to an incorrect errors package import and redundant logic. Additionally, the Python library implementation currently hardcodes the output path, which breaks consistency with the Go changes and existing methods like save_pcap. There is also a concern regarding the log file being opened twice—once manually and once via Zap—which could lead to interleaved output.

Comment thread otns_main/otns_main.go
Comment thread pylibs/otns/cli/OTNS.py
Comment thread logger/logger.go Outdated
For running multiple OTNS simulations in sequence, an issue is that presently output files from previous runs
may be overwritten. Not all output artefacts are stored in the 'tmp' directory also.
This PR ensures that all output files to into a single, configurable directory. The default name is 'tmp' but
this can be changed by the commandline argument `-output`. Furthermore, also the OTNS main log which was
previously not saved is now stored as a file in this directory.

Particular commandline argument parsing changes:
-logfile  is changed to -log-node which better expresses its purpose: the log level for OT nodes.
-noreplay is changed to -replay, with the default being that the replay file is not generated. Typically,
          this file is not used, so now it needs to be explicitly requested.
-log-stdout is added as a way to select whether OTNS log gets written to stdout/stderr too, or only to the
            OTNS log file.
- cross-checks are now being done for mutually incompatible cmd arguments.
- improved error display format in case of a user error in using the cmd arguments.

For logging to terminal, there's now an improved check for whether stdout is a terminal (accepting ANSI
sequences) or not a terminal. The dispatcher socket is also stored in the new output directory. Because this
might be a long name, and socket paths are limited, there's a length check added also.

Flash files are also stored in the output directory, using an environment var to let OTNS tell the OT node
where to store it (OTNS_DATA_PATH). Flash files are not deleted at the start of the simulation anymore, to
allow a user to use the 'restore' option to restore a node from a previous simulation based on flash.
By default, restore=false and then flash files are deleted (which is the common case).
@EskoDijk
Copy link
Copy Markdown
Contributor Author

/gemini review

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a configurable output directory for simulation results, logs, and PCAP files, moving away from hardcoded paths like /tmp and current.pcap. It includes a significant refactor of the logging system to support simultaneous output to console and files, along with updated CLI flags and environment variable support (OTNS_DATA_PATH) for node flash storage. Feedback suggests ensuring consistent timestamping in the Println function for better event correlation and increasing the file path buffer size in the C-based flash implementation to handle longer paths more robustly.

Comment thread logger/logger.go
Comment on lines +262 to 275
func Println(msg string, toConsole bool, toLogFile bool) {
if toConsole {
if isLogToTerminal {
_, _ = fmt.Fprint(os.Stdout, "\033[2K\r") // ANSI sequence to clear the CLI line
}
_, _ = fmt.Fprintln(os.Stdout, msg)
if isLogToTerminal && cbStdout != nil {
cbStdout.OnStdout()
}
}
_, _ = fmt.Fprint(os.Stdout, msg+"\n")
if isLogToTerminal && cbStdout != nil {
cbStdout.OnStdout()
if toLogFile && logFileHandle != nil {
_, _ = logFileHandle.WriteString(msg + "\n")
}
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The Println function writes directly to the log file without any timestamping or formatting, whereas Logf uses Zap which includes a timestamp. This results in an inconsistent log file where some lines (like echoed commands and node UART output) lack timestamps, making it harder to correlate events during post-simulation analysis. Consider adding a timestamp to Println when toLogFile is true, similar to how it's done manually in SetLevel.

Suggested change
func Println(msg string, toConsole bool, toLogFile bool) {
if toConsole {
if isLogToTerminal {
_, _ = fmt.Fprint(os.Stdout, "\033[2K\r") // ANSI sequence to clear the CLI line
}
_, _ = fmt.Fprintln(os.Stdout, msg)
if isLogToTerminal && cbStdout != nil {
cbStdout.OnStdout()
}
}
_, _ = fmt.Fprint(os.Stdout, msg+"\n")
if isLogToTerminal && cbStdout != nil {
cbStdout.OnStdout()
if toLogFile && logFileHandle != nil {
_, _ = logFileHandle.WriteString(msg + "\n")
}
}
func Println(msg string, toConsole bool, toLogFile bool) {
if toConsole {
if isLogToTerminal {
_, _ = fmt.Fprint(os.Stdout, "\033[2K\r") // ANSI sequence to clear the CLI line
}
_, _ = fmt.Fprintln(os.Stdout, msg)
if isLogToTerminal && cbStdout != nil {
cbStdout.OnStdout()
}
}
if toLogFile && logFileHandle != nil {
ts := time.Now().Format(dateTimeFormat)
_, _ = logFileHandle.WriteString(ts + " " + msg + "\n")
}
}

Comment thread ot-rfsim/src/flash.c
char fileName[sizeof(OPENTHREAD_CONFIG_POSIX_SETTINGS_PATH) + 32];
const char *envPath = getenv("OTNS_DATA_PATH");
const char *path = (envPath != NULL) ? envPath : OPENTHREAD_CONFIG_POSIX_SETTINGS_PATH;
char fileName[512];
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

While 512 bytes is often sufficient for file paths in a simulation environment, it is smaller than the standard PATH_MAX (usually 4096 on Linux). If a user provides a very long path via the -output argument, snprintf will safely truncate the string, but the resulting path might be invalid or point to the wrong location. Consider using PATH_MAX for the buffer size to be more robust.

    char        fileName[4096];

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant