logging: split FileHandlerFactory into two classes
authorAdam Simpkins <simpkins@fb.com>
Thu, 7 Dec 2017 01:29:30 +0000 (17:29 -0800)
committerFacebook Github Bot <facebook-github-bot@users.noreply.github.com>
Thu, 7 Dec 2017 01:41:43 +0000 (17:41 -0800)
Summary:
Split FileHandlerFactory into separate FileHandlerFactory and
StreamHandlerFactory classes, where FileHandlerFactory only handles logging to
files by path name, and StreamHandlerFactory only supports logging to stdout or
stderr.

The primary motivation for this is to allow logging to stdout or stderr in some
cases without allowing arbitrary files to be opened by FileHandlerFactory.
This can be achieved now by registering StreamHandlerFactory but not
FileHandlerFactory.  This makes it safer to allow controlling logging
configuration via command line flags even in setuid binaries.

Reviewed By: yfeldblum

Differential Revision: D6494226

fbshipit-source-id: a3ec371ca4266424d07dff20be18e6e13c057593

12 files changed:
folly/Makefile.am
folly/experimental/logging/FileHandlerFactory.cpp
folly/experimental/logging/FileHandlerFactory.h
folly/experimental/logging/FileWriterFactory.cpp [new file with mode: 0644]
folly/experimental/logging/FileWriterFactory.h [new file with mode: 0644]
folly/experimental/logging/Init.cpp
folly/experimental/logging/Makefile.am
folly/experimental/logging/StreamHandlerFactory.cpp [new file with mode: 0644]
folly/experimental/logging/StreamHandlerFactory.h [new file with mode: 0644]
folly/experimental/logging/docs/Config.md
folly/experimental/logging/test/FileHandlerFactoryTest.cpp
folly/experimental/logging/test/fatal_test.py

index 65e8560..121c1ee 100644 (file)
@@ -158,6 +158,7 @@ nobase_follyinclude_HEADERS = \
        experimental/LockFreeRingBuffer.h \
        experimental/logging/AsyncFileWriter.h \
        experimental/logging/FileHandlerFactory.h \
+       experimental/logging/FileWriterFactory.h \
        experimental/logging/GlogStyleFormatter.h \
        experimental/logging/ImmediateFileWriter.h \
        experimental/logging/Init.h \
@@ -181,6 +182,7 @@ nobase_follyinclude_HEADERS = \
        experimental/logging/RateLimiter.h \
        experimental/logging/StandardLogHandler.h \
        experimental/logging/StandardLogHandlerFactory.h \
+       experimental/logging/StreamHandlerFactory.h \
        experimental/logging/xlog.h \
        experimental/NestedCommandLineApp.h \
        experimental/observer/detail/Core.h \
index c398654..c6b6645 100644 (file)
  */
 #include <folly/experimental/logging/FileHandlerFactory.h>
 
-#include <folly/Conv.h>
-#include <folly/experimental/logging/AsyncFileWriter.h>
-#include <folly/experimental/logging/ImmediateFileWriter.h>
+#include <folly/experimental/logging/FileWriterFactory.h>
 #include <folly/experimental/logging/StandardLogHandler.h>
 #include <folly/experimental/logging/StandardLogHandlerFactory.h>
 
-using std::make_shared;
-using std::shared_ptr;
-using std::string;
-
 namespace folly {
 
 class FileHandlerFactory::WriterFactory
@@ -32,73 +26,27 @@ class FileHandlerFactory::WriterFactory
  public:
   bool processOption(StringPiece name, StringPiece value) override {
     if (name == "path") {
-      if (!stream_.empty()) {
-        throw std::invalid_argument(
-            "cannot specify both \"path\" and \"stream\" "
-            "parameters for FileHandlerFactory");
-      }
       path_ = value.str();
       return true;
-    } else if (name == "stream") {
-      if (!path_.empty()) {
-        throw std::invalid_argument(
-            "cannot specify both \"path\" and \"stream\" "
-            "parameters for FileHandlerFactory");
-      }
-      stream_ = value.str();
-      return true;
-    } else if (name == "async") {
-      async_ = to<bool>(value);
-      return true;
-    } else if (name == "max_buffer_size") {
-      auto size = to<size_t>(value);
-      if (size == 0) {
-        throw std::invalid_argument(to<string>("must be a postive number"));
-      }
-      maxBufferSize_ = size;
-      return true;
-    } else {
-      return false;
     }
+
+    // TODO: In the future it would be nice to support log rotation, and
+    // add parameters to control when the log file should be rotated.
+
+    return fileWriterFactory_.processOption(name, value);
   }
 
   std::shared_ptr<LogWriter> createWriter() override {
     // Get the output file to use
-    File outputFile;
-    if (!path_.empty()) {
-      outputFile = File{path_, O_WRONLY | O_APPEND | O_CREAT};
-    } else if (stream_ == "stderr") {
-      outputFile = File{STDERR_FILENO, /* ownsFd */ false};
-    } else if (stream_ == "stdout") {
-      outputFile = File{STDOUT_FILENO, /* ownsFd */ false};
-    } else {
-      throw std::invalid_argument(to<string>(
-          "unknown stream for FileHandlerFactory: \"",
-          stream_,
-          "\" expected one of stdout or stderr"));
-    }
-
-    // Determine whether we should use ImmediateFileWriter or AsyncFileWriter
-    if (async_) {
-      auto asyncWriter = make_shared<AsyncFileWriter>(std::move(outputFile));
-      if (maxBufferSize_.hasValue()) {
-        asyncWriter->setMaxBufferSize(maxBufferSize_.value());
-      }
-      return asyncWriter;
-    } else {
-      if (maxBufferSize_.hasValue()) {
-        throw std::invalid_argument(to<string>(
-            "the \"max_buffer_size\" option is only valid for async file "
-            "handlers"));
-      }
-      return make_shared<ImmediateFileWriter>(std::move(outputFile));
+    if (path_.empty()) {
+      throw std::invalid_argument("no path specified for file handler");
     }
+    return fileWriterFactory_.createWriter(
+        File{path_, O_WRONLY | O_APPEND | O_CREAT});
   }
 
   std::string path_;
-  std::string stream_;
-  bool async_{true};
-  Optional<size_t> maxBufferSize_;
+  FileWriterFactory fileWriterFactory_;
 };
 
 std::shared_ptr<LogHandler> FileHandlerFactory::createHandler(
index 2e8cbba..29e74ab 100644 (file)
@@ -23,8 +23,13 @@ namespace folly {
  * FileHandlerFactory is a LogHandlerFactory that constructs log handlers
  * that write to a file.
  *
- * It can construct handlers that use either ImmediateFileWriter or
- * AsyncFileWriter.
+ * Note that FileHandlerFactory allows opening and appending to arbitrary files
+ * based on the handler options.  This may make it unsafe to use
+ * FileHandlerFactory in some contexts: for instance, a setuid binary should
+ * generally avoid registering the FileHandlerFactory if they allow log
+ * handlers to be configured via command line parameters, since otherwise this
+ * may allow non-root users to append to files that they otherwise would not
+ * have write permissions for.
  */
 class FileHandlerFactory : public LogHandlerFactory {
  public:
diff --git a/folly/experimental/logging/FileWriterFactory.cpp b/folly/experimental/logging/FileWriterFactory.cpp
new file mode 100644 (file)
index 0000000..3b545d8
--- /dev/null
@@ -0,0 +1,62 @@
+/*
+ * Copyright 2004-present Facebook, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+#include <folly/experimental/logging/FileWriterFactory.h>
+
+#include <folly/Conv.h>
+#include <folly/File.h>
+#include <folly/experimental/logging/AsyncFileWriter.h>
+#include <folly/experimental/logging/ImmediateFileWriter.h>
+
+using std::make_shared;
+using std::string;
+
+namespace folly {
+
+bool FileWriterFactory::processOption(StringPiece name, StringPiece value) {
+  if (name == "async") {
+    async_ = to<bool>(value);
+    return true;
+  } else if (name == "max_buffer_size") {
+    auto size = to<size_t>(value);
+    if (size == 0) {
+      throw std::invalid_argument(to<string>("must be a positive integer"));
+    }
+    maxBufferSize_ = size;
+    return true;
+  } else {
+    return false;
+  }
+}
+
+std::shared_ptr<LogWriter> FileWriterFactory::createWriter(File file) {
+  // Determine whether we should use ImmediateFileWriter or AsyncFileWriter
+  if (async_) {
+    auto asyncWriter = make_shared<AsyncFileWriter>(std::move(file));
+    if (maxBufferSize_.hasValue()) {
+      asyncWriter->setMaxBufferSize(maxBufferSize_.value());
+    }
+    return asyncWriter;
+  } else {
+    if (maxBufferSize_.hasValue()) {
+      throw std::invalid_argument(to<string>(
+          "the \"max_buffer_size\" option is only valid for async file "
+          "handlers"));
+    }
+    return make_shared<ImmediateFileWriter>(std::move(file));
+  }
+}
+
+} // namespace folly
diff --git a/folly/experimental/logging/FileWriterFactory.h b/folly/experimental/logging/FileWriterFactory.h
new file mode 100644 (file)
index 0000000..32442a7
--- /dev/null
@@ -0,0 +1,43 @@
+/*
+ * Copyright 2004-present Facebook, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+#pragma once
+
+#include <folly/Optional.h>
+#include <folly/Range.h>
+#include <memory>
+
+namespace folly {
+
+class File;
+class LogWriter;
+
+/**
+ * A helper class for creating an AsyncFileWriter or ImmediateFileWriter based
+ * on log handler options settings.
+ *
+ * This is used by StreamHandlerFactory and FileHandlerFactory.
+ */
+class FileWriterFactory {
+ public:
+  bool processOption(StringPiece name, StringPiece value);
+  std::shared_ptr<LogWriter> createWriter(File file);
+
+ private:
+  bool async_{true};
+  Optional<size_t> maxBufferSize_;
+};
+
+} // namespace folly
index 59d4f8b..2460051 100644 (file)
  */
 #include <folly/experimental/logging/Init.h>
 
-#include <folly/experimental/logging/FileHandlerFactory.h>
 #include <folly/experimental/logging/LogConfig.h>
 #include <folly/experimental/logging/LogConfigParser.h>
 #include <folly/experimental/logging/LoggerDB.h>
+#include <folly/experimental/logging/StreamHandlerFactory.h>
 
 namespace folly {
 
@@ -44,16 +44,24 @@ namespace folly {
  * handler, the handler will be automatically forgotten by the LoggerDB code.
  */
 constexpr StringPiece kDefaultLoggingConfig =
-    ".=WARN:default; default=file,stream=stderr,async=false";
+    ".=WARN:default; default=stream,stream=stderr,async=false";
 
 void initLogging(StringPiece configString) {
-  // Register the FileHandlerFactory
-  //
+  // Register the StreamHandlerFactory
+  LoggerDB::get()->registerHandlerFactory(
+      std::make_unique<StreamHandlerFactory>());
+
   // TODO: In the future it would be nice to build a better mechanism so that
   // additional LogHandlerFactory objects could be automatically registered on
   // startup if they are linked into our current executable.
-  LoggerDB::get()->registerHandlerFactory(
-      std::make_unique<FileHandlerFactory>());
+  //
+  // For now we register only the StreamHandlerFactory.  There is a
+  // FileHandlerFactory, but we do not register it by default: it allows
+  // appending to arbitrary files based on the config settings, and we do not
+  // want to allow this by default for security reasons.  (In the future
+  // maybe it would be worth enabling the FileHandlerFactory by default if we
+  // confirm that we are not a setuid or setgid binary.  i.e., if the real
+  // UID+GID is the same as the effective UID+GID.)
 
   // Parse the default log level settings before processing the input config
   // string.
index 4edca2a..e22576f 100644 (file)
@@ -5,6 +5,7 @@ lib_LTLIBRARIES = libfollylogging.la
 libfollylogging_la_SOURCES = \
        AsyncFileWriter.cpp \
        FileHandlerFactory.cpp \
+       FileWriterFactory.cpp \
        GlogStyleFormatter.cpp \
        ImmediateFileWriter.cpp \
        Init.cpp \
@@ -24,6 +25,7 @@ libfollylogging_la_SOURCES = \
        RateLimiter.cpp \
        StandardLogHandler.cpp \
        StandardLogHandlerFactory.cpp \
+       StreamHandlerFactory.cpp \
        xlog.cpp
 
 libfollylogging_la_LIBADD = $(top_builddir)/libfolly.la
diff --git a/folly/experimental/logging/StreamHandlerFactory.cpp b/folly/experimental/logging/StreamHandlerFactory.cpp
new file mode 100644 (file)
index 0000000..55c5c42
--- /dev/null
@@ -0,0 +1,67 @@
+/*
+ * Copyright 2004-present Facebook, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+#include <folly/experimental/logging/StreamHandlerFactory.h>
+
+#include <folly/Conv.h>
+#include <folly/experimental/logging/FileWriterFactory.h>
+#include <folly/experimental/logging/StandardLogHandler.h>
+#include <folly/experimental/logging/StandardLogHandlerFactory.h>
+
+namespace folly {
+
+class StreamHandlerFactory::WriterFactory
+    : public StandardLogHandlerFactory::WriterFactory {
+ public:
+  bool processOption(StringPiece name, StringPiece value) override {
+    if (name == "stream") {
+      stream_ = value.str();
+      return true;
+    }
+    return fileWriterFactory_.processOption(name, value);
+  }
+
+  std::shared_ptr<LogWriter> createWriter() override {
+    // Get the output file to use
+    File outputFile;
+    if (stream_.empty()) {
+      throw std::invalid_argument(
+          "no stream name specified for stream handler");
+    } else if (stream_ == "stderr") {
+      outputFile = File{STDERR_FILENO, /* ownsFd */ false};
+    } else if (stream_ == "stdout") {
+      outputFile = File{STDOUT_FILENO, /* ownsFd */ false};
+    } else {
+      throw std::invalid_argument(to<std::string>(
+          "unknown stream \"",
+          stream_,
+          "\": expected one of stdout or stderr"));
+    }
+
+    return fileWriterFactory_.createWriter(std::move(outputFile));
+  }
+
+  std::string stream_;
+  FileWriterFactory fileWriterFactory_;
+};
+
+std::shared_ptr<LogHandler> StreamHandlerFactory::createHandler(
+    const Options& options) {
+  WriterFactory writerFactory;
+  return StandardLogHandlerFactory::createHandler(
+      getType(), &writerFactory, options);
+}
+
+} // namespace folly
diff --git a/folly/experimental/logging/StreamHandlerFactory.h b/folly/experimental/logging/StreamHandlerFactory.h
new file mode 100644 (file)
index 0000000..e08f7b8
--- /dev/null
@@ -0,0 +1,44 @@
+/*
+ * Copyright 2004-present Facebook, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+#pragma once
+
+#include <folly/experimental/logging/LogHandlerFactory.h>
+
+namespace folly {
+
+/**
+ * StreamHandlerFactory is a LogHandlerFactory that constructs log handlers
+ * that write to stdout or stderr.
+ *
+ * This is quite similar to FileHandlerFactory, but it always writes to an
+ * existing open file descriptor rather than opening a new file.  This handler
+ * factory is separate from FileHandlerFactory primarily for safety reasons:
+ * FileHandlerFactory supports appending to arbitrary files via config
+ * parameters, while StreamHandlerFactory does not.
+ */
+class StreamHandlerFactory : public LogHandlerFactory {
+ public:
+  StringPiece getType() const override {
+    return "stream";
+  }
+
+  std::shared_ptr<LogHandler> createHandler(const Options& options) override;
+
+ private:
+  class WriterFactory;
+};
+
+} // namespace folly
index b7cb425..482a989 100644 (file)
@@ -143,21 +143,21 @@ Example log configuration strings:
   therefore be discarded, even though they are enabled for one of its parent
   categories.
 
-* `ERROR:stderr, folly=INFO; stderr=file,stream=stderr`
+* `ERROR:stderr, folly=INFO; stderr=stream,stream=stderr`
 
   Sets the root log category level to ERROR, and sets its handler list to
   use the "stderr" handler.  Sets the folly log level to INFO.  Defines
   a log handler named "stderr" which writes to stderr.
 
-* `ERROR:default,folly=INFO:x;default=file,stream=stderr;x=file,path=/tmp/x.log`
+* `ERROR:x,folly=INFO:y;x=stream,stream=stderr;y=file,path=/tmp/y.log`
 
-  Defines two log handlers: "default" which writes to stderr and "x" which
-  writes to the file /tmp/x.log
+  Defines two log handlers: "x" which writes to stderr and "y" which
+  writes to the file /tmp/y.log
   Sets the root log catgory level to ERROR, and configures it to use the
-  "default" handler.  Sets the log level for the "folly" category to INFO and
-  configures it to use the "x" handler.
+  "x" handler.  Sets the log level for the "folly" category to INFO and
+  configures it to use the "y" handler.
 
-* `ERROR:default:x; default=file,stream=stderr; x=file,path=/tmp/x.log`
+* `ERROR:default:x; default=stream,stream=stderr; x=file,path=/tmp/x.log`
 
   Defines two log handlers: "default" which writes to stderr and "x" which
   writes to the file /tmp/x.log
@@ -172,7 +172,7 @@ Example log configuration strings:
   to the empty list.  Not specifying handler information at all (no ':')
   will leave any pre-existing handlers as-is.
 
-* `;default=file,stream=stdout`
+* `;default=stream,stream=stdout`
 
   Does not change any log category settings, and defines a "default" handler
   that writes to stdout.  This format is useful to update log handler settings
@@ -252,7 +252,7 @@ following fields:
   }
   "handlers": {
     "stderr": {
-      "type": "file",
+      "type": "stream",
       "options": {
         "stream": "stderr",
         "async": true,
index 83724f2..f9cfe83 100644 (file)
@@ -14,6 +14,7 @@
  * limitations under the License.
  */
 #include <folly/experimental/logging/FileHandlerFactory.h>
+#include <folly/experimental/logging/StreamHandlerFactory.h>
 
 #include <folly/Exception.h>
 #include <folly/experimental/TestUtil.h>
@@ -22,6 +23,7 @@
 #include <folly/experimental/logging/ImmediateFileWriter.h>
 #include <folly/experimental/logging/StandardLogHandler.h>
 #include <folly/portability/GTest.h>
+#include <folly/test/TestUtils.h>
 
 using namespace folly;
 using folly::test::TemporaryFile;
@@ -33,7 +35,7 @@ void checkAsyncWriter(
     size_t expectedMaxBufferSize) {
   auto asyncWriter = dynamic_cast<const AsyncFileWriter*>(writer);
   ASSERT_TRUE(asyncWriter)
-      << "FileHandlerFactory should have created an AsyncFileWriter";
+      << "handler factory should have created an AsyncFileWriter";
   EXPECT_EQ(expectedMaxBufferSize, asyncWriter->getMaxBufferSize());
 
   // Make sure this refers to the expected output file
@@ -52,7 +54,7 @@ void checkAsyncWriter(
     size_t expectedMaxBufferSize) {
   auto asyncWriter = dynamic_cast<const AsyncFileWriter*>(writer);
   ASSERT_TRUE(asyncWriter)
-      << "FileHandlerFactory should have created an AsyncFileWriter";
+      << "handler factory should have created an AsyncFileWriter";
   EXPECT_EQ(expectedMaxBufferSize, asyncWriter->getMaxBufferSize());
   EXPECT_EQ(expectedFD, asyncWriter->getFile().fd());
 }
@@ -61,7 +63,7 @@ TEST(FileHandlerFactory, pathOnly) {
   FileHandlerFactory factory;
 
   TemporaryFile tmpFile{"logging_test"};
-  auto options = FileHandlerFactory::Options{
+  auto options = LogHandlerFactory::Options{
       make_pair("path", tmpFile.path().string()),
   };
   auto handler = factory.createHandler(options);
@@ -72,7 +74,7 @@ TEST(FileHandlerFactory, pathOnly) {
   auto formatter =
       std::dynamic_pointer_cast<GlogStyleFormatter>(stdHandler->getFormatter());
   EXPECT_TRUE(formatter)
-      << "FileHandlerFactory should have created a GlogStyleFormatter";
+      << "handler factory should have created a GlogStyleFormatter";
 
   checkAsyncWriter(
       stdHandler->getWriter().get(),
@@ -80,11 +82,11 @@ TEST(FileHandlerFactory, pathOnly) {
       AsyncFileWriter::kDefaultMaxBufferSize);
 }
 
-TEST(FileHandlerFactory, stderrStream) {
-  FileHandlerFactory factory;
+TEST(StreamHandlerFactory, stderrStream) {
+  StreamHandlerFactory factory;
 
   TemporaryFile tmpFile{"logging_test"};
-  auto options = FileHandlerFactory::Options{
+  auto options = StreamHandlerFactory::Options{
       make_pair("stream", "stderr"),
   };
   auto handler = factory.createHandler(options);
@@ -95,7 +97,7 @@ TEST(FileHandlerFactory, stderrStream) {
   auto formatter =
       std::dynamic_pointer_cast<GlogStyleFormatter>(stdHandler->getFormatter());
   EXPECT_TRUE(formatter)
-      << "FileHandlerFactory should have created a GlogStyleFormatter";
+      << "handler factory should have created a GlogStyleFormatter";
 
   checkAsyncWriter(
       stdHandler->getWriter().get(),
@@ -103,11 +105,11 @@ TEST(FileHandlerFactory, stderrStream) {
       AsyncFileWriter::kDefaultMaxBufferSize);
 }
 
-TEST(FileHandlerFactory, stdoutWithMaxBuffer) {
-  FileHandlerFactory factory;
+TEST(StreamHandlerFactory, stdoutWithMaxBuffer) {
+  StreamHandlerFactory factory;
 
   TemporaryFile tmpFile{"logging_test"};
-  auto options = FileHandlerFactory::Options{
+  auto options = StreamHandlerFactory::Options{
       make_pair("stream", "stdout"),
       make_pair("max_buffer_size", "4096"),
   };
@@ -119,7 +121,7 @@ TEST(FileHandlerFactory, stdoutWithMaxBuffer) {
   auto formatter =
       std::dynamic_pointer_cast<GlogStyleFormatter>(stdHandler->getFormatter());
   EXPECT_TRUE(formatter)
-      << "FileHandlerFactory should have created a GlogStyleFormatter";
+      << "handler factory should have created a GlogStyleFormatter";
 
   checkAsyncWriter(stdHandler->getWriter().get(), STDOUT_FILENO, 4096);
 }
@@ -128,7 +130,7 @@ TEST(FileHandlerFactory, pathWithMaxBufferSize) {
   FileHandlerFactory factory;
 
   TemporaryFile tmpFile{"logging_test"};
-  auto options = FileHandlerFactory::Options{
+  auto options = LogHandlerFactory::Options{
       make_pair("path", tmpFile.path().string()),
       make_pair("max_buffer_size", "4096000"),
   };
@@ -140,17 +142,17 @@ TEST(FileHandlerFactory, pathWithMaxBufferSize) {
   auto formatter =
       std::dynamic_pointer_cast<GlogStyleFormatter>(stdHandler->getFormatter());
   EXPECT_TRUE(formatter)
-      << "FileHandlerFactory should have created a GlogStyleFormatter";
+      << "handler factory should have created a GlogStyleFormatter";
 
   checkAsyncWriter(
       stdHandler->getWriter().get(), tmpFile.path().string().c_str(), 4096000);
 }
 
-TEST(FileHandlerFactory, nonAsyncStderr) {
-  FileHandlerFactory factory;
+TEST(StreamHandlerFactory, nonAsyncStderr) {
+  StreamHandlerFactory factory;
 
   TemporaryFile tmpFile{"logging_test"};
-  auto options = FileHandlerFactory::Options{
+  auto options = LogHandlerFactory::Options{
       make_pair("stream", "stderr"),
       make_pair("async", "no"),
   };
@@ -162,7 +164,7 @@ TEST(FileHandlerFactory, nonAsyncStderr) {
   auto formatter =
       std::dynamic_pointer_cast<GlogStyleFormatter>(stdHandler->getFormatter());
   EXPECT_TRUE(formatter)
-      << "FileHandlerFactory should have created a GlogStyleFormatter";
+      << "handler factory should have created a GlogStyleFormatter";
 
   auto writer =
       std::dynamic_pointer_cast<ImmediateFileWriter>(stdHandler->getWriter());
@@ -173,73 +175,185 @@ TEST(FileHandlerFactory, nonAsyncStderr) {
 TEST(FileHandlerFactory, errors) {
   FileHandlerFactory factory;
   TemporaryFile tmpFile{"logging_test"};
+  using Options = LogHandlerFactory::Options;
 
   {
-    auto options = FileHandlerFactory::Options{};
-    EXPECT_THROW(factory.createHandler(options), std::invalid_argument)
-        << "one of path or stream required";
+    auto options = Options{};
+    EXPECT_THROW_RE(
+        factory.createHandler(options),
+        std::invalid_argument,
+        "no path specified for file handler");
   }
 
   {
-    auto options = FileHandlerFactory::Options{
-        make_pair("path", tmpFile.path().string()),
-        make_pair("stream", "stderr"),
+    auto options = Options{
+        {"path", tmpFile.path().string()},
+        {"stream", "stderr"},
     };
-    EXPECT_THROW(factory.createHandler(options), std::invalid_argument)
-        << "path and stream cannot both be specified";
+    EXPECT_THROW_RE(
+        factory.createHandler(options),
+        std::invalid_argument,
+        "unknown option \"stream\"");
   }
 
   {
-    auto options = FileHandlerFactory::Options{
-        make_pair("stream", "nonstdout"),
+    auto options = Options{
+        {"stream", "nonstdout"},
     };
-    EXPECT_THROW(factory.createHandler(options), std::invalid_argument)
-        << "invalid stream";
+    EXPECT_THROW_RE(
+        factory.createHandler(options),
+        std::invalid_argument,
+        "unknown option \"stream\"");
   }
 
   {
-    auto options = FileHandlerFactory::Options{
-        make_pair("stream", "stderr"),
-        make_pair("async", "foobar"),
+    auto options = Options{
+        {"path", tmpFile.path().string()},
+        {"async", "xyz"},
     };
-    EXPECT_THROW(factory.createHandler(options), std::invalid_argument)
-        << "invalid async value";
+    EXPECT_THROW_RE(
+        factory.createHandler(options),
+        std::invalid_argument,
+        "^error processing option \"async\": Invalid value for bool: \"xyz\"$");
   }
 
   {
-    auto options = FileHandlerFactory::Options{
-        make_pair("stream", "stderr"),
-        make_pair("async", "false"),
-        make_pair("max_buffer_size", "1234"),
+    auto options = Options{
+        {"path", tmpFile.path().string()},
+        {"async", "false"},
+        {"max_buffer_size", "1234"},
     };
-    EXPECT_THROW(factory.createHandler(options), std::invalid_argument)
-        << "max_buffer_size only valid for async writers";
+    EXPECT_THROW_RE(
+        factory.createHandler(options),
+        std::invalid_argument,
+        "the \"max_buffer_size\" option is only valid for async file handlers");
   }
 
   {
-    auto options = FileHandlerFactory::Options{
-        make_pair("stream", "stderr"),
-        make_pair("max_buffer_size", "hello"),
+    auto options = Options{
+        {"path", tmpFile.path().string()},
+        {"max_buffer_size", "hello"},
     };
-    EXPECT_THROW(factory.createHandler(options), std::invalid_argument)
-        << "max_buffer_size must be an integer";
+    EXPECT_THROW_RE(
+        factory.createHandler(options),
+        std::invalid_argument,
+        "^error processing option \"max_buffer_size\": "
+        "Non-digit character found: \"hello\"$");
   }
 
   {
-    auto options = FileHandlerFactory::Options{
-        make_pair("stream", "stderr"),
-        make_pair("max_buffer_size", "0"),
+    auto options = Options{
+        {"path", tmpFile.path().string()},
+        {"max_buffer_size", "0"},
+    };
+    EXPECT_THROW_RE(
+        factory.createHandler(options),
+        std::invalid_argument,
+        "^error processing option \"max_buffer_size\": "
+        "must be a positive integer$");
+  }
+
+  {
+    auto options = Options{
+        {"path", tmpFile.path().string()},
+        {"foo", "bar"},
+    };
+    EXPECT_THROW_RE(
+        factory.createHandler(options),
+        std::invalid_argument,
+        "^unknown option \"foo\"$");
+  }
+}
+
+TEST(StreamHandlerFactory, errors) {
+  StreamHandlerFactory factory;
+  using Options = LogHandlerFactory::Options;
+
+  {
+    auto options = Options{};
+    EXPECT_THROW_RE(
+        factory.createHandler(options),
+        std::invalid_argument,
+        "no stream name specified for stream handler");
+  }
+
+  {
+    auto options = Options{
+        {"path", "/tmp/log.txt"},
+        {"stream", "stderr"},
+    };
+    EXPECT_THROW_RE(
+        factory.createHandler(options),
+        std::invalid_argument,
+        "unknown option \"path\"");
+  }
+
+  {
+    auto options = Options{
+        {"stream", "nonstdout"},
+    };
+    EXPECT_THROW_RE(
+        factory.createHandler(options),
+        std::invalid_argument,
+        "unknown stream \"nonstdout\": expected one of stdout or stderr");
+  }
+
+  {
+    auto options = Options{
+        {"stream", "stderr"},
+        {"async", "xyz"},
+    };
+    EXPECT_THROW_RE(
+        factory.createHandler(options),
+        std::invalid_argument,
+        "^error processing option \"async\": Invalid value for bool: \"xyz\"$");
+  }
+
+  {
+    auto options = Options{
+        {"stream", "stderr"},
+        {"async", "false"},
+        {"max_buffer_size", "1234"},
+    };
+    EXPECT_THROW_RE(
+        factory.createHandler(options),
+        std::invalid_argument,
+        "^the \"max_buffer_size\" option is only valid for "
+        "async file handlers$");
+  }
+
+  {
+    auto options = Options{
+        {"stream", "stderr"},
+        {"max_buffer_size", "hello"},
+    };
+    EXPECT_THROW_RE(
+        factory.createHandler(options),
+        std::invalid_argument,
+        "^error processing option \"max_buffer_size\": "
+        "Non-digit character found: \"hello\"$");
+  }
+
+  {
+    auto options = Options{
+        {"stream", "stderr"},
+        {"max_buffer_size", "0"},
     };
-    EXPECT_THROW(factory.createHandler(options), std::invalid_argument)
-        << "max_buffer_size must be a positive integer";
+    EXPECT_THROW_RE(
+        factory.createHandler(options),
+        std::invalid_argument,
+        "^error processing option \"max_buffer_size\": "
+        "must be a positive integer$");
   }
 
   {
-    auto options = FileHandlerFactory::Options{
+    auto options = Options{
         make_pair("stream", "stderr"),
         make_pair("foo", "bar"),
     };
-    EXPECT_THROW(factory.createHandler(options), std::invalid_argument)
-        << "unknown parameter foo";
+    EXPECT_THROW_RE(
+        factory.createHandler(options),
+        std::invalid_argument,
+        "unknown option \"foo\"");
   }
 }
index 04dc9ed..fa7a2a9 100644 (file)
@@ -69,12 +69,12 @@ class FatalTests(unittest.TestCase):
         subprocess.check_output([self.helper, '--crash=no'])
 
     def test_async(self):
-        handler_setings = 'default=file,stream=stderr,async=true'
+        handler_setings = 'default=stream,stream=stderr,async=true'
         err = self.run_helper('--logging=;' + handler_setings)
         self.assertRegex(err, self.get_crash_regex())
 
     def test_immediate(self):
-        handler_setings = 'default=file,stream=stderr,async=false'
+        handler_setings = 'default=stream,stream=stderr,async=false'
         err = self.run_helper('--logging=;' + handler_setings)
         self.assertRegex(err, self.get_crash_regex())