[logging] Make fx_log_get_logger() always return a valid pointer
It currently only returns null when duping STDERR_FILENO fails
but we can avoid the dup altogether by not providing a fd at all
and triggering the logger's fallback behaviour.
Bug: 49001
Change-Id: I86702b3b1e23b7812d0cb95f5c3e3a7689f85770
Reviewed-on: https://fuchsia-review.googlesource.com/c/fuchsia/+/377391
Testability-Review: Adam Barth <abarth@google.com>
Commit-Queue: Saman Sami <samans@google.com>
Reviewed-by: Adam Barth <abarth@google.com>
Reviewed-by: Adam Perry <adamperry@google.com>
API-Review: Adam Barth <abarth@google.com>
diff --git a/sdk/lib/syslog/syslog.api b/sdk/lib/syslog/syslog.api
index 31ef4de..9169e6c 100644
--- a/sdk/lib/syslog/syslog.api
+++ b/sdk/lib/syslog/syslog.api
@@ -1,5 +1,5 @@
{
- "pkg/syslog/include/lib/syslog/global.h": "3136d8d600ca6b381233584ab12b1141",
- "pkg/syslog/include/lib/syslog/logger.h": "2abc3a24ebace570e47e40d13c805afd",
+ "pkg/syslog/include/lib/syslog/global.h": "2ce5b4c42fe5f394e4f13e1bc92219ee",
+ "pkg/syslog/include/lib/syslog/logger.h": "b50163886e109aa12f8c971cd89b4501",
"pkg/syslog/include/lib/syslog/wire_format.h": "d47e76b6b62a4af22f978067c0401e92"
}
\ No newline at end of file
diff --git a/src/lib/syslog/cpp/logger.cc b/src/lib/syslog/cpp/logger.cc
index d4aedc2..a6dac5e 100644
--- a/src/lib/syslog/cpp/logger.cc
+++ b/src/lib/syslog/cpp/logger.cc
@@ -42,16 +42,10 @@
LogMessage::~LogMessage() {
fx_logger_t* logger = fx_log_get_logger();
- if (logger) {
- if (status_ != INT32_MAX) {
- stream_ << ": " << status_ << " (" << zx_status_get_string(status_) << ")";
- }
-
- fx_logger_log(logger, severity_, tag_, stream_.str().c_str());
- } else if (severity_ == FX_LOG_FATAL) {
- // FX_LOG_FATAL should abort regardless of whether a logger exists
- FxLogFatal();
+ if (status_ != INT32_MAX) {
+ stream_ << ": " << status_ << " (" << zx_status_get_string(status_) << ")";
}
+ fx_logger_log(logger, severity_, tag_, stream_.str().c_str());
}
// Note that this implementation allows a data race on counter_, but
diff --git a/zircon/system/ulib/syslog/fx_logger.h b/zircon/system/ulib/syslog/fx_logger.h
index edbbc72..ea322e4 100644
--- a/zircon/system/ulib/syslog/fx_logger.h
+++ b/zircon/system/ulib/syslog/fx_logger.h
@@ -43,6 +43,8 @@
pid_ = GetCurrentProcessKoid();
dropped_logs_.store(0, std::memory_order_relaxed);
Reconfigure(config);
+ if (config->log_service_channel == ZX_HANDLE_INVALID && config->console_fd == -1)
+ ActivateFallback(-1);
}
~fx_logger() = default;
diff --git a/zircon/system/ulib/syslog/global.cc b/zircon/system/ulib/syslog/global.cc
index 7333950..8844a6d 100644
--- a/zircon/system/ulib/syslog/global.cc
+++ b/zircon/system/ulib/syslog/global.cc
@@ -27,7 +27,9 @@
.tags = &tag,
.num_tags = 1};
fx_logger_t* logger = NULL;
- fx_logger_create(&config, &logger);
+ status = fx_logger_create(&config, &logger);
+ // Making the default logger should never fail.
+ ZX_DEBUG_ASSERT(status == ZX_OK);
return logger;
}
@@ -40,18 +42,11 @@
}
SYSLOG_EXPORT
-zx_status_t fx_log_init(void) {
- if (!fx_log_get_logger())
- return ZX_ERR_INTERNAL;
- return ZX_OK;
-}
+zx_status_t fx_log_init(void) { return ZX_OK; }
SYSLOG_EXPORT
zx_status_t fx_log_init_with_config(const fx_logger_config_t* config) {
- fx_logger_t* logger = fx_log_get_logger();
- if (!logger)
- return ZX_ERR_INTERNAL;
- return logger->Reconfigure(config);
+ return fx_log_get_logger()->Reconfigure(config);
}
// This is here to force a definition to be included here for C99.
diff --git a/zircon/system/ulib/syslog/include/lib/syslog/global.h b/zircon/system/ulib/syslog/include/lib/syslog/global.h
index 3c93477..4c848d2 100644
--- a/zircon/system/ulib/syslog/include/lib/syslog/global.h
+++ b/zircon/system/ulib/syslog/include/lib/syslog/global.h
@@ -13,14 +13,16 @@
__BEGIN_CDECLS
// Gets the global logger for the process to which log messages emitted
-// using the FX_LOG macros will be written. This function is thread-safe.
+// using the FX_LOG macros will be written. This function returns the same
+// logger on all threads and is thread-safe. The returned pointer is never
+// null and it does not get invalidated when the logger is reconfigured.
fx_logger_t* fx_log_get_logger(void);
// Returns true if writing messages with the given severity is enabled in the
// global logger.
static inline bool fx_log_is_enabled(fx_log_severity_t severity) {
fx_logger_t* logger = fx_log_get_logger();
- return logger && severity >= fx_logger_get_min_severity(logger);
+ return severity >= fx_logger_get_min_severity(logger);
}
// Reconfigures the global logger for this process with the specified
@@ -31,15 +33,13 @@
// This function is NOT thread-safe and must be called early in the program
// before other threads are spawned.
// Returns:
-// - ZX_INTERNAL_ERR if the global logger had failed to instantiate,
// - ZX_ERR_INVALID_ARGS if config is invalid (i.e. is null or has more than
// FX_LOG_MAX_TAGS tags),
// - ZX_OK if the reconfiguration succeeds.
// TODO(samans): Rename this function. https://fxbug.dev/49001
zx_status_t fx_log_init_with_config(const fx_logger_config_t* config);
-// Deprecated. Doesn't do anything. Returns ZX_INTERNAL_ERR if the global logger
-// had failed to instantiate, and ZX_OK otherwise.
+// Deprecated. Doesn't do anything and always returns ZX_OK.
// TODO(samans): Remove this function. https://fxbug.dev/49001
zx_status_t fx_log_init(void);
@@ -67,28 +67,28 @@
// |verbosity| is positive number. Logger severity is set to -verbosity
#define FX_LOG_SET_VERBOSITY(verbosity) _FX_LOG_SET_SEVERITY(-(verbosity))
-#define _FX_LOG(severity, tag, message) \
- do { \
- fx_logger_t* logger = fx_log_get_logger(); \
- if (logger && fx_logger_get_min_severity(logger) <= (severity)) { \
- fx_logger_log(logger, (severity), (tag), (message)); \
- } \
+#define _FX_LOG(severity, tag, message) \
+ do { \
+ fx_logger_t* logger = fx_log_get_logger(); \
+ if (fx_logger_get_min_severity(logger) <= (severity)) { \
+ fx_logger_log(logger, (severity), (tag), (message)); \
+ } \
} while (0)
#define _FX_LOGF(severity, tag, message, ...) \
do { \
fx_logger_t* logger = fx_log_get_logger(); \
- if (logger && fx_logger_get_min_severity(logger) <= (severity)) { \
+ if (fx_logger_get_min_severity(logger) <= (severity)) { \
fx_logger_logf(logger, (severity), (tag), (message), __VA_ARGS__); \
} \
} while (0)
-#define _FX_LOGVF(severity, tag, message, args) \
- do { \
- fx_logger_t* logger = fx_log_get_logger(); \
- if (logger && fx_logger_get_min_severity(logger) <= (severity)) { \
- fx_logger_logvf(logger, (severity), (tag), (message), (args)); \
- } \
+#define _FX_LOGVF(severity, tag, message, args) \
+ do { \
+ fx_logger_t* logger = fx_log_get_logger(); \
+ if (fx_logger_get_min_severity(logger) <= (severity)) { \
+ fx_logger_logvf(logger, (severity), (tag), (message), (args)); \
+ } \
} while (0)
// Writes a message to the global logger.
diff --git a/zircon/system/ulib/syslog/include/lib/syslog/logger.h b/zircon/system/ulib/syslog/include/lib/syslog/logger.h
index d712ea2..c5cb89e 100644
--- a/zircon/system/ulib/syslog/include/lib/syslog/logger.h
+++ b/zircon/system/ulib/syslog/include/lib/syslog/logger.h
@@ -10,7 +10,6 @@
#include <stdarg.h>
#include <unistd.h>
-
#include <zircon/types.h>
// Max no of tags associated with a logger.
@@ -62,7 +61,7 @@
// Creates a logger object from the specified configuration.
//
// This will return ZX_ERR_INVALID_ARGS if |num_tags| is more than
-// |FX_LOG_MAX_TAGS| and return |ZX_ERR_INTERNAL} if dup fails.
+// |FX_LOG_MAX_TAGS|.
// |config| can be safely deleted after this function returns.
zx_status_t fx_logger_create(const fx_logger_config_t* config, fx_logger_t** out_logger);
diff --git a/zircon/system/ulib/syslog/logger.cc b/zircon/system/ulib/syslog/logger.cc
index 87b879d..cbbd586 100644
--- a/zircon/system/ulib/syslog/logger.cc
+++ b/zircon/system/ulib/syslog/logger.cc
@@ -67,20 +67,14 @@
return ZX_ERR_INVALID_ARGS;
}
fx_logger_config_t c = *config;
- // In the SYSLOG_STATIC mode, we cannot connect to the logging service. We should continue to
- // instantiate the logger and the client will provide the appropriate channel / fd later.
+ // In the SYSLOG_STATIC mode, we cannot connect to the logging service. We
+ // should continue to instantiate the logger (which defaults to using stderr)
+ // and the client can provide the appropriate channel / fd later.
#ifndef SYSLOG_STATIC
if (config->console_fd == -1 && config->log_service_channel == ZX_HANDLE_INVALID) {
zx::socket sock = connect_to_logger();
- if (sock.is_valid()) {
+ if (sock.is_valid())
c.log_service_channel = sock.release();
- } else {
- int newfd = dup(STDERR_FILENO);
- if (newfd < 0) {
- return ZX_ERR_INTERNAL;
- }
- c.console_fd = newfd;
- }
}
#endif
*out_logger = new fx_logger(&c);
diff --git a/zircon/system/ulib/syslog/test/syslog_tests.cc b/zircon/system/ulib/syslog/test/syslog_tests.cc
index 8665603..b3c27ab 100644
--- a/zircon/system/ulib/syslog/test/syslog_tests.cc
+++ b/zircon/system/ulib/syslog/test/syslog_tests.cc
@@ -7,6 +7,7 @@
#include <lib/syslog/global.h>
#include <lib/zx/socket.h>
#include <poll.h>
+#include <stdlib.h>
#include <unistd.h>
#include <unittest/unittest.h>
@@ -28,6 +29,16 @@
return strcmp(str, suffix) == 0;
}
+static int smallest_unused_fd() {
+ char name[] = "/tmp/syslog_test.XXXXXX";
+ int fd = mkstemp(name);
+ ASSERT_GT(fd, -1);
+ close(fd);
+ int status = remove(name);
+ ASSERT_EQ(0, status);
+ return fd;
+}
+
bool test_log_init(void) {
BEGIN_TEST;
fx_log_reset_global_for_testing();
@@ -304,6 +315,32 @@
END_TEST;
}
+bool test_log_dont_dup(void) {
+ BEGIN_TEST;
+
+ // Remember the current lowest ununsed fd.
+ int fd = smallest_unused_fd();
+
+ // Create a logger
+ fx_logger_t* logger;
+ zx_status_t status;
+ fx_logger_config_t config = {.min_severity = FX_LOG_INFO,
+ .console_fd = -1,
+ .log_service_channel = ZX_HANDLE_INVALID,
+ .tags = nullptr,
+ .num_tags = 0};
+ status = fx_logger_create(&config, &logger);
+ ASSERT_EQ(ZX_OK, status);
+
+ // No fd must be taken by the logger.
+ EXPECT_EQ(fd, smallest_unused_fd());
+
+ // Cleanup
+ fx_logger_destroy(logger);
+
+ END_TEST;
+}
+
BEGIN_TEST_CASE(syslog_tests)
RUN_TEST(test_log_init)
RUN_TEST(test_log_init_with_socket)
@@ -318,6 +355,7 @@
RUN_TEST(test_vlog_simple_write)
RUN_TEST(test_vlog_write)
RUN_TEST(test_log_reconfiguration)
+RUN_TEST(test_log_dont_dup)
END_TEST_CASE(syslog_tests)
BEGIN_TEST_CASE(syslog_tests_edge_cases)
