From a952486542153bb13e70d4df7e34707f801e0180 Mon Sep 17 00:00:00 2001 From: Patrick Ohly Date: Wed, 7 Sep 2022 14:46:14 +0200 Subject: [PATCH 1/2] add test for the command line usage The flags and their defaults are part of the klog API and must not change. --- klog_test.go | 45 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/klog_test.go b/klog_test.go index 71717c0d..1b572104 100644 --- a/klog_test.go +++ b/klog_test.go @@ -764,6 +764,9 @@ func TestFileSizeCheck(t *testing.T) { } func TestInitFlags(t *testing.T) { + defer CaptureState().Restore() + setFlags() + fs1 := flag.NewFlagSet("test1", flag.PanicOnError) InitFlags(fs1) fs1.Set("log_dir", "/test1") @@ -779,6 +782,48 @@ func TestInitFlags(t *testing.T) { } } +func TestCommandLine(t *testing.T) { + var fs flag.FlagSet + InitFlags(&fs) + + expectedFlags := ` -add_dir_header + If true, adds the file directory to the header of the log messages + -alsologtostderr + log to standard error as well as files (no effect when -logtostderr=true) + -log_backtrace_at value + when logging hits line file:N, emit a stack trace + -log_dir string + If non-empty, write log files in this directory (no effect when -logtostderr=true) + -log_file string + If non-empty, use this log file (no effect when -logtostderr=true) + -log_file_max_size uint + Defines the maximum size a log file can grow to (no effect when -logtostderr=true). Unit is megabytes. If the value is 0, the maximum file size is unlimited. (default 1800) + -logtostderr + log to standard error instead of files (default true) + -one_output + If true, only write logs to their native severity level (vs also writing to each lower severity level; no effect when -logtostderr=true) + -skip_headers + If true, avoid header prefixes in the log messages + -skip_log_headers + If true, avoid headers when opening log files (no effect when -logtostderr=true) + -stderrthreshold value + logs at or above this threshold go to stderr when writing to files and stderr (no effect when -logtostderr=true or -alsologtostderr=false) (default 2) + -v value + number for the log level verbosity + -vmodule value + comma-separated list of pattern=N settings for file-filtered logging +` + + var output bytes.Buffer + fs.SetOutput(&output) + fs.PrintDefaults() + actualFlags := output.String() + + if expectedFlags != actualFlags { + t.Fatalf("Command line changed.\nExpected:\n%q\nActual:\n%q\n", expectedFlags, actualFlags) + } +} + func TestInfoObjectRef(t *testing.T) { defer CaptureState().Restore() setFlags() From 28f790698e907ffbd2ac80fe4d6954429f827fbf Mon Sep 17 00:00:00 2001 From: Patrick Ohly Date: Wed, 7 Sep 2022 14:50:34 +0200 Subject: [PATCH 2/2] make InitFlags read-only The way it was implemented before, the current value of several flags was written to the global config struct each time InitFlags was called. Not all flags may get written concurrently, so this led to a data race when some other goroutine was actively using klog. InitFlags also created new flag.Flag instances for each call. By creating flags once during init, InitFlags becomes entirely read-only and thus safe to use concurrently. What remains ambiguous is which flags may get changed at runtime. That was underspecified before and doesn't get fixed here either, it just gets called out as a potential problem. --- klog.go | 61 +++++++++++++++++++++++++++------------------------------ 1 file changed, 29 insertions(+), 32 deletions(-) diff --git a/klog.go b/klog.go index 628117e6..1bd11b67 100644 --- a/klog.go +++ b/klog.go @@ -396,45 +396,48 @@ type flushSyncWriter interface { io.Writer } -// init sets up the defaults. +var logging loggingT +var commandLine flag.FlagSet + +// init sets up the defaults and creates command line flags. func init() { + commandLine.StringVar(&logging.logDir, "log_dir", "", "If non-empty, write log files in this directory (no effect when -logtostderr=true)") + commandLine.StringVar(&logging.logFile, "log_file", "", "If non-empty, use this log file (no effect when -logtostderr=true)") + commandLine.Uint64Var(&logging.logFileMaxSizeMB, "log_file_max_size", 1800, + "Defines the maximum size a log file can grow to (no effect when -logtostderr=true). Unit is megabytes. "+ + "If the value is 0, the maximum file size is unlimited.") + commandLine.BoolVar(&logging.toStderr, "logtostderr", true, "log to standard error instead of files") + commandLine.BoolVar(&logging.alsoToStderr, "alsologtostderr", false, "log to standard error as well as files (no effect when -logtostderr=true)") + logging.setVState(0, nil, false) + commandLine.Var(&logging.verbosity, "v", "number for the log level verbosity") + commandLine.BoolVar(&logging.addDirHeader, "add_dir_header", false, "If true, adds the file directory to the header of the log messages") + commandLine.BoolVar(&logging.skipHeaders, "skip_headers", false, "If true, avoid header prefixes in the log messages") + commandLine.BoolVar(&logging.oneOutput, "one_output", false, "If true, only write logs to their native severity level (vs also writing to each lower severity level; no effect when -logtostderr=true)") + commandLine.BoolVar(&logging.skipLogHeaders, "skip_log_headers", false, "If true, avoid headers when opening log files (no effect when -logtostderr=true)") logging.stderrThreshold = severityValue{ Severity: severity.ErrorLog, // Default stderrThreshold is ERROR. } - logging.setVState(0, nil, false) - logging.logDir = "" - logging.logFile = "" - logging.logFileMaxSizeMB = 1800 - logging.toStderr = true - logging.alsoToStderr = false - logging.skipHeaders = false - logging.addDirHeader = false - logging.skipLogHeaders = false - logging.oneOutput = false + commandLine.Var(&logging.stderrThreshold, "stderrthreshold", "logs at or above this threshold go to stderr when writing to files and stderr (no effect when -logtostderr=true or -alsologtostderr=false)") + commandLine.Var(&logging.vmodule, "vmodule", "comma-separated list of pattern=N settings for file-filtered logging") + commandLine.Var(&logging.traceLocation, "log_backtrace_at", "when logging hits line file:N, emit a stack trace") + + logging.settings.contextualLoggingEnabled = true logging.flushD = newFlushDaemon(logging.lockAndFlushAll, nil) } // InitFlags is for explicitly initializing the flags. +// It may get called repeatedly for different flagsets, but not +// twice for the same one. May get called concurrently +// to other goroutines using klog. However, only some flags +// may get set concurrently (see implementation). func InitFlags(flagset *flag.FlagSet) { if flagset == nil { flagset = flag.CommandLine } - flagset.StringVar(&logging.logDir, "log_dir", logging.logDir, "If non-empty, write log files in this directory (no effect when -logtostderr=true)") - flagset.StringVar(&logging.logFile, "log_file", logging.logFile, "If non-empty, use this log file (no effect when -logtostderr=true)") - flagset.Uint64Var(&logging.logFileMaxSizeMB, "log_file_max_size", logging.logFileMaxSizeMB, - "Defines the maximum size a log file can grow to (no effect when -logtostderr=true). Unit is megabytes. "+ - "If the value is 0, the maximum file size is unlimited.") - flagset.BoolVar(&logging.toStderr, "logtostderr", logging.toStderr, "log to standard error instead of files") - flagset.BoolVar(&logging.alsoToStderr, "alsologtostderr", logging.alsoToStderr, "log to standard error as well as files (no effect when -logtostderr=true)") - flagset.Var(&logging.verbosity, "v", "number for the log level verbosity") - flagset.BoolVar(&logging.addDirHeader, "add_dir_header", logging.addDirHeader, "If true, adds the file directory to the header of the log messages") - flagset.BoolVar(&logging.skipHeaders, "skip_headers", logging.skipHeaders, "If true, avoid header prefixes in the log messages") - flagset.BoolVar(&logging.oneOutput, "one_output", logging.oneOutput, "If true, only write logs to their native severity level (vs also writing to each lower severity level; no effect when -logtostderr=true)") - flagset.BoolVar(&logging.skipLogHeaders, "skip_log_headers", logging.skipLogHeaders, "If true, avoid headers when opening log files (no effect when -logtostderr=true)") - flagset.Var(&logging.stderrThreshold, "stderrthreshold", "logs at or above this threshold go to stderr when writing to files and stderr (no effect when -logtostderr=true or -alsologtostderr=false)") - flagset.Var(&logging.vmodule, "vmodule", "comma-separated list of pattern=N settings for file-filtered logging") - flagset.Var(&logging.traceLocation, "log_backtrace_at", "when logging hits line file:N, emit a stack trace") + commandLine.VisitAll(func(f *flag.Flag) { + flagset.Var(f.Value, f.Name, f.Usage) + }) } // Flush flushes all pending log I/O. @@ -549,12 +552,6 @@ type loggingT struct { vmap map[uintptr]Level } -var logging = loggingT{ - settings: settings{ - contextualLoggingEnabled: true, - }, -} - // setVState sets a consistent state for V logging. // l.mu is held. func (l *loggingT) setVState(verbosity Level, filter []modulePat, setFilter bool) {