logging: split FileHandlerFactory into two classes

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

diff --git a/folly/Makefile.am b/folly/Makefile.am
index 65e856080c0f028647806fb474b586bae71387d2..121c1eeb9ff89d1862f97ac4ee584de833704601 100644 (file)
--- a/folly/Makefile.am
+++ b/folly/Makefile.am
@@ -158,6 +158,7 @@ nobase_follyinclude_HEADERS = \
        experimental/LockFreeRingBuffer.h \
        experimental/logging/AsyncFileWriter.h \
        experimental/logging/FileHandlerFactory.h \
        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 \
        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/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 \
        experimental/logging/xlog.h \
        experimental/NestedCommandLineApp.h \
        experimental/observer/detail/Core.h \

diff --git a/folly/experimental/logging/FileHandlerFactory.cpp b/folly/experimental/logging/FileHandlerFactory.cpp
index c398654f6a5f5a1df16f2ca8e94843558d9a51d4..c6b66459e0da236848eee37fce57409babedbcc0 100644 (file)
--- a/folly/experimental/logging/FileHandlerFactory.cpp
+++ b/folly/experimental/logging/FileHandlerFactory.cpp
@@ -15,16 +15,10 @@
  */
 #include <folly/experimental/logging/FileHandlerFactory.h>
 
  */
 #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>
 
 #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
 namespace folly {
 
 class FileHandlerFactory::WriterFactory


@@ -32,73 +26,27 @@ class FileHandlerFactory::WriterFactory
  public:
   bool processOption(StringPiece name, StringPiece value) override {
     if (name == "path") {
  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;
       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
   }
 
   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 path_;

-  std::string stream_;
-  bool async_{true};
-  Optional<size_t> maxBufferSize_;
+  FileWriterFactory fileWriterFactory_;

 };
 
 std::shared_ptr<LogHandler> FileHandlerFactory::createHandler(
 };
 
 std::shared_ptr<LogHandler> FileHandlerFactory::createHandler(

diff --git a/folly/experimental/logging/FileHandlerFactory.h b/folly/experimental/logging/FileHandlerFactory.h
index 2e8cbba8edc7e6d1e16c8b942a261e7c387f45ac..29e74abfb44edba14b3dd8ad9a7f8f4a8d2c884f 100644 (file)
--- a/folly/experimental/logging/FileHandlerFactory.h
+++ b/folly/experimental/logging/FileHandlerFactory.h
@@ -23,8 +23,13 @@ namespace folly {
  * FileHandlerFactory is a LogHandlerFactory that constructs log handlers
  * that write to a file.
  *
  * 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:
  */
 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
+++ b/folly/experimental/logging/FileWriterFactory.cpp
@@ -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
+++ b/folly/experimental/logging/FileWriterFactory.h
@@ -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

diff --git a/folly/experimental/logging/Init.cpp b/folly/experimental/logging/Init.cpp
index 59d4f8b8b950f4228d9c5ecba860fbc21d8a09b2..24600510da59d04813160cc498f95032f660b58c 100644 (file)
--- a/folly/experimental/logging/Init.cpp
+++ b/folly/experimental/logging/Init.cpp
@@ -15,10 +15,10 @@
  */
 #include <folly/experimental/logging/Init.h>
 
  */
 #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/LogConfig.h>
 #include <folly/experimental/logging/LogConfigParser.h>
 #include <folly/experimental/logging/LoggerDB.h>

+#include <folly/experimental/logging/StreamHandlerFactory.h>

 
 namespace folly {
 
 
 namespace folly {
 


@@ -44,16 +44,24 @@ namespace folly {
  * handler, the handler will be automatically forgotten by the LoggerDB code.
  */
 constexpr StringPiece kDefaultLoggingConfig =
  * 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) {
 
 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.
   // 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.
 
   // Parse the default log level settings before processing the input config
   // string.

diff --git a/folly/experimental/logging/Makefile.am b/folly/experimental/logging/Makefile.am
index 4edca2ace03d98023657b020d6fe58e0d13edfd5..e22576faddd128419ab64e2d4162f6ff49151827 100644 (file)
--- a/folly/experimental/logging/Makefile.am
+++ b/folly/experimental/logging/Makefile.am
@@ -5,6 +5,7 @@ lib_LTLIBRARIES = libfollylogging.la
 libfollylogging_la_SOURCES = \
        AsyncFileWriter.cpp \
        FileHandlerFactory.cpp \
 libfollylogging_la_SOURCES = \
        AsyncFileWriter.cpp \
        FileHandlerFactory.cpp \

+       FileWriterFactory.cpp \

        GlogStyleFormatter.cpp \
        ImmediateFileWriter.cpp \
        Init.cpp \
        GlogStyleFormatter.cpp \
        ImmediateFileWriter.cpp \
        Init.cpp \


@@ -24,6 +25,7 @@ libfollylogging_la_SOURCES = \
        RateLimiter.cpp \
        StandardLogHandler.cpp \
        StandardLogHandlerFactory.cpp \
        RateLimiter.cpp \
        StandardLogHandler.cpp \
        StandardLogHandlerFactory.cpp \

+       StreamHandlerFactory.cpp \

        xlog.cpp
 
 libfollylogging_la_LIBADD = $(top_builddir)/libfolly.la
        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
+++ b/folly/experimental/logging/StreamHandlerFactory.cpp
@@ -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
+++ b/folly/experimental/logging/StreamHandlerFactory.h
@@ -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

diff --git a/folly/experimental/logging/docs/Config.md b/folly/experimental/logging/docs/Config.md
index b7cb42515e551e0bdb5e4e77f04e15f75bfa6355..482a9890876448bf4d2b1333d4e5fb1772911f4a 100644 (file)
--- a/folly/experimental/logging/docs/Config.md
+++ b/folly/experimental/logging/docs/Config.md
@@ -143,21 +143,21 @@ Example log configuration strings:
   therefore be discarded, even though they are enabled for one of its parent
   categories.
 
   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.
 
 
   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
   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
 
   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.
 
   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
 
   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": {
   }
   "handlers": {
     "stderr": {

-      "type": "file",
+      "type": "stream",

       "options": {
         "stream": "stderr",
         "async": true,
       "options": {
         "stream": "stderr",
         "async": true,

diff --git a/folly/experimental/logging/test/FileHandlerFactoryTest.cpp b/folly/experimental/logging/test/FileHandlerFactoryTest.cpp
index 83724f29d11c59038575847c4503387a31a28784..f9cfe83d8286f70ef97b7a07af85e579f1a17c5b 100644 (file)
--- a/folly/experimental/logging/test/FileHandlerFactoryTest.cpp
+++ b/folly/experimental/logging/test/FileHandlerFactoryTest.cpp
@@ -14,6 +14,7 @@
  * limitations under the License.
  */
 #include <folly/experimental/logging/FileHandlerFactory.h>
  * 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>
 
 #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/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;
 
 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)
     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
   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)
     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());
 }
   EXPECT_EQ(expectedMaxBufferSize, asyncWriter->getMaxBufferSize());
   EXPECT_EQ(expectedFD, asyncWriter->getFile().fd());
 }


@@ -61,7 +63,7 @@ TEST(FileHandlerFactory, pathOnly) {
   FileHandlerFactory factory;
 
   TemporaryFile tmpFile{"logging_test"};
   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);
       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)
   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(),
 
   checkAsyncWriter(
       stdHandler->getWriter().get(),


@@ -80,11 +82,11 @@ TEST(FileHandlerFactory, pathOnly) {
       AsyncFileWriter::kDefaultMaxBufferSize);
 }
 
       AsyncFileWriter::kDefaultMaxBufferSize);
 }
 

-TEST(FileHandlerFactory, stderrStream) {
-  FileHandlerFactory factory;
+TEST(StreamHandlerFactory, stderrStream) {
+  StreamHandlerFactory factory;

 
   TemporaryFile tmpFile{"logging_test"};
 
   TemporaryFile tmpFile{"logging_test"};

-  auto options = FileHandlerFactory::Options{
+  auto options = StreamHandlerFactory::Options{

       make_pair("stream", "stderr"),
   };
   auto handler = factory.createHandler(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)
   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(),
 
   checkAsyncWriter(
       stdHandler->getWriter().get(),


@@ -103,11 +105,11 @@ TEST(FileHandlerFactory, stderrStream) {
       AsyncFileWriter::kDefaultMaxBufferSize);
 }
 
       AsyncFileWriter::kDefaultMaxBufferSize);
 }
 

-TEST(FileHandlerFactory, stdoutWithMaxBuffer) {
-  FileHandlerFactory factory;
+TEST(StreamHandlerFactory, stdoutWithMaxBuffer) {
+  StreamHandlerFactory factory;

 
   TemporaryFile tmpFile{"logging_test"};
 
   TemporaryFile tmpFile{"logging_test"};

-  auto options = FileHandlerFactory::Options{
+  auto options = StreamHandlerFactory::Options{

       make_pair("stream", "stdout"),
       make_pair("max_buffer_size", "4096"),
   };
       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)
   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);
 }
 
   checkAsyncWriter(stdHandler->getWriter().get(), STDOUT_FILENO, 4096);
 }


@@ -128,7 +130,7 @@ TEST(FileHandlerFactory, pathWithMaxBufferSize) {
   FileHandlerFactory factory;
 
   TemporaryFile tmpFile{"logging_test"};
   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"),
   };
       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)
   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);
 }
 
 
   checkAsyncWriter(
       stdHandler->getWriter().get(), tmpFile.path().string().c_str(), 4096000);
 }
 

-TEST(FileHandlerFactory, nonAsyncStderr) {
-  FileHandlerFactory factory;
+TEST(StreamHandlerFactory, nonAsyncStderr) {
+  StreamHandlerFactory factory;

 
   TemporaryFile tmpFile{"logging_test"};
 
   TemporaryFile tmpFile{"logging_test"};

-  auto options = FileHandlerFactory::Options{
+  auto options = LogHandlerFactory::Options{

       make_pair("stream", "stderr"),
       make_pair("async", "no"),
   };
       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)
   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());
 
   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"};
 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"),
     };
         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\"");

   }
 }
   }
 }

diff --git a/folly/experimental/logging/test/fatal_test.py b/folly/experimental/logging/test/fatal_test.py
index 04dc9ed0400d11644dc4acae10864daa6b5fcfb0..fa7a2a9ba6419c6efd27fc7a9542b713f18e5e57 100644 (file)
--- a/folly/experimental/logging/test/fatal_test.py
+++ b/folly/experimental/logging/test/fatal_test.py
@@ -69,12 +69,12 @@ class FatalTests(unittest.TestCase):
         subprocess.check_output([self.helper, '--crash=no'])
 
     def test_async(self):
         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):
         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())
 
         err = self.run_helper('--logging=;' + handler_setings)
         self.assertRegex(err, self.get_crash_regex())