From 9e1a1ce16ace21118f180ae28ade5ead129093d4 Mon Sep 17 00:00:00 2001 From: Adam Simpkins Date: Wed, 6 Dec 2017 17:29:30 -0800 Subject: [PATCH] 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 --- folly/Makefile.am | 2 + .../logging/FileHandlerFactory.cpp | 74 +----- .../experimental/logging/FileHandlerFactory.h | 9 +- .../logging/FileWriterFactory.cpp | 62 +++++ .../experimental/logging/FileWriterFactory.h | 43 ++++ folly/experimental/logging/Init.cpp | 20 +- folly/experimental/logging/Makefile.am | 2 + .../logging/StreamHandlerFactory.cpp | 67 ++++++ .../logging/StreamHandlerFactory.h | 44 ++++ folly/experimental/logging/docs/Config.md | 18 +- .../logging/test/FileHandlerFactoryTest.cpp | 222 +++++++++++++----- folly/experimental/logging/test/fatal_test.py | 4 +- 12 files changed, 431 insertions(+), 136 deletions(-) create mode 100644 folly/experimental/logging/FileWriterFactory.cpp create mode 100644 folly/experimental/logging/FileWriterFactory.h create mode 100644 folly/experimental/logging/StreamHandlerFactory.cpp create mode 100644 folly/experimental/logging/StreamHandlerFactory.h diff --git a/folly/Makefile.am b/folly/Makefile.am index 65e85608..121c1eeb 100644 --- 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/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 \ diff --git a/folly/experimental/logging/FileHandlerFactory.cpp b/folly/experimental/logging/FileHandlerFactory.cpp index c398654f..c6b66459 100644 --- a/folly/experimental/logging/FileHandlerFactory.cpp +++ b/folly/experimental/logging/FileHandlerFactory.cpp @@ -15,16 +15,10 @@ */ #include -#include -#include -#include +#include #include #include -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(value); - return true; - } else if (name == "max_buffer_size") { - auto size = to(value); - if (size == 0) { - throw std::invalid_argument(to("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 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( - "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(std::move(outputFile)); - if (maxBufferSize_.hasValue()) { - asyncWriter->setMaxBufferSize(maxBufferSize_.value()); - } - return asyncWriter; - } else { - if (maxBufferSize_.hasValue()) { - throw std::invalid_argument(to( - "the \"max_buffer_size\" option is only valid for async file " - "handlers")); - } - return make_shared(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 maxBufferSize_; + FileWriterFactory fileWriterFactory_; }; std::shared_ptr FileHandlerFactory::createHandler( diff --git a/folly/experimental/logging/FileHandlerFactory.h b/folly/experimental/logging/FileHandlerFactory.h index 2e8cbba8..29e74abf 100644 --- 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. * - * 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 index 00000000..3b545d8f --- /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 + +#include +#include +#include +#include + +using std::make_shared; +using std::string; + +namespace folly { + +bool FileWriterFactory::processOption(StringPiece name, StringPiece value) { + if (name == "async") { + async_ = to(value); + return true; + } else if (name == "max_buffer_size") { + auto size = to(value); + if (size == 0) { + throw std::invalid_argument(to("must be a positive integer")); + } + maxBufferSize_ = size; + return true; + } else { + return false; + } +} + +std::shared_ptr FileWriterFactory::createWriter(File file) { + // Determine whether we should use ImmediateFileWriter or AsyncFileWriter + if (async_) { + auto asyncWriter = make_shared(std::move(file)); + if (maxBufferSize_.hasValue()) { + asyncWriter->setMaxBufferSize(maxBufferSize_.value()); + } + return asyncWriter; + } else { + if (maxBufferSize_.hasValue()) { + throw std::invalid_argument(to( + "the \"max_buffer_size\" option is only valid for async file " + "handlers")); + } + return make_shared(std::move(file)); + } +} + +} // namespace folly diff --git a/folly/experimental/logging/FileWriterFactory.h b/folly/experimental/logging/FileWriterFactory.h new file mode 100644 index 00000000..32442a75 --- /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 +#include +#include + +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 createWriter(File file); + + private: + bool async_{true}; + Optional maxBufferSize_; +}; + +} // namespace folly diff --git a/folly/experimental/logging/Init.cpp b/folly/experimental/logging/Init.cpp index 59d4f8b8..24600510 100644 --- a/folly/experimental/logging/Init.cpp +++ b/folly/experimental/logging/Init.cpp @@ -15,10 +15,10 @@ */ #include -#include #include #include #include +#include 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()); + // 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()); + // + // 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. diff --git a/folly/experimental/logging/Makefile.am b/folly/experimental/logging/Makefile.am index 4edca2ac..e22576fa 100644 --- 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 \ + 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 index 00000000..55c5c428 --- /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 + +#include +#include +#include +#include + +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 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( + "unknown stream \"", + stream_, + "\": expected one of stdout or stderr")); + } + + return fileWriterFactory_.createWriter(std::move(outputFile)); + } + + std::string stream_; + FileWriterFactory fileWriterFactory_; +}; + +std::shared_ptr 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 index 00000000..e08f7b83 --- /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 + +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 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 b7cb4251..482a9890 100644 --- 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. -* `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, diff --git a/folly/experimental/logging/test/FileHandlerFactoryTest.cpp b/folly/experimental/logging/test/FileHandlerFactoryTest.cpp index 83724f29..f9cfe83d 100644 --- a/folly/experimental/logging/test/FileHandlerFactoryTest.cpp +++ b/folly/experimental/logging/test/FileHandlerFactoryTest.cpp @@ -14,6 +14,7 @@ * limitations under the License. */ #include +#include #include #include @@ -22,6 +23,7 @@ #include #include #include +#include using namespace folly; using folly::test::TemporaryFile; @@ -33,7 +35,7 @@ void checkAsyncWriter( size_t expectedMaxBufferSize) { auto asyncWriter = dynamic_cast(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(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(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(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(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(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(stdHandler->getFormatter()); EXPECT_TRUE(formatter) - << "FileHandlerFactory should have created a GlogStyleFormatter"; + << "handler factory should have created a GlogStyleFormatter"; auto writer = std::dynamic_pointer_cast(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\""); } } diff --git a/folly/experimental/logging/test/fatal_test.py b/folly/experimental/logging/test/fatal_test.py index 04dc9ed0..fa7a2a9b 100644 --- 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): - 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()) -- 2.34.1