refactor: replace tr_sys_file_write_line() with fmt::print("...\n") (#6619)

* refactor: use fmt::print in log.cc

https://github.com/fmtlib/fmt/issues/428#issuecomment-395442159
> You can use fmt::print("...\n") on Windows as well.

Use this mechanism instead of tr_sys_file_write_line()

* refactor: use FILE* in daemon

* refactor: remove unused tr_sys_file_flush_possible()

* refactor: remove unused tr_sys_file_write_line()

* refactor: remove unused tr_sys_file_get_std()

* refactor: remove unused tr_std_sys_file_t

daemon/daemon.cc

@@ -268,7 +268,7 @@ auto onFileAdded(tr_session* session, std::string_view dirname, std::string_view
 }
 
 void printMessage(
-    tr_sys_file_t file,
+    FILE* ostream,
     tr_log_level level,
     std::string_view name,
     std::string_view message,
@@ -286,11 +286,11 @@ void printMessage(
         fmt::format_to(std::back_inserter(out), "{:s} {:s} ({:s}:{:d})", name, message, filename, line);
     }
 
-    if (file != TR_BAD_SYS_FILE)
+    if (ostream != nullptr)
     {
         auto timestr = std::array<char, 64>{};
         tr_logGetTimeStr(std::data(timestr), std::size(timestr));
-        tr_sys_file_write_line(file, fmt::format("[{:s}] {:s} {:s}", std::data(timestr), levelName(level), std::data(out)));
+        fmt::print(ostream, "[{:s}] {:s} {:s}\n", std::data(timestr), levelName(level), out.c_str());
     }
 
 #ifdef HAVE_SYSLOG
@@ -329,18 +329,21 @@ void printMessage(
 #endif
 }
 
-void pumpLogMessages(tr_sys_file_t file, bool flush)
+void pumpLogMessages(FILE* log_stream)
 {
     tr_log_message* list = tr_logGetQueue();
 
     for (tr_log_message const* l = list; l != nullptr; l = l->next)
     {
-        printMessage(file, l->level, l->name, l->message, l->file, l->line);
+        printMessage(log_stream, l->level, l->name, l->message, l->file, l->line);
     }
 
-    if (flush && file != TR_BAD_SYS_FILE)
+    // two reasons to not flush stderr:
+    // 1. it's usually redundant, since stderr flushes itself
+    // 2. when running as a systemd unit, it's redirected to a socket
+    if (log_stream != stderr)
     {
-        tr_sys_file_flush(file);
+        fflush(log_stream);
     }
 
     tr_logFreeQueue(list);
@@ -374,31 +377,26 @@ tr_variant load_settings(char const* config_dir)
 
 bool tr_daemon::reopen_log_file(char const* filename)
 {
-    auto error = tr_error{};
+    auto* const old_stream = log_stream_;
-    tr_sys_file_t const old_log_file = logfile_;
-    tr_sys_file_t const new_log_file = tr_sys_file_open(
-        filename,
-        TR_SYS_FILE_WRITE | TR_SYS_FILE_CREATE | TR_SYS_FILE_APPEND,
-        0666,
-        &error);
 
-    if (new_log_file == TR_BAD_SYS_FILE)
+    auto* new_stream = std::fopen(filename, "a");
+    if (new_stream == nullptr)
     {
+        auto const err = errno;
         auto const errmsg = fmt::format(
             "Couldn't open '{path}': {error} ({error_code})",
             fmt::arg("path", filename),
-            fmt::arg("error", error.message()),
+            fmt::arg("error", tr_strerror(err)),
-            fmt::arg("error_code", error.code()));
+            fmt::arg("error_code", err));
         fmt::print(stderr, "{:s}\n", errmsg);
         return false;
     }
 
-    logfile_ = new_log_file;
+    log_stream_ = new_stream;
-    logfile_flush_ = tr_sys_file_flush_possible(logfile_);
 
-    if (old_log_file != TR_BAD_SYS_FILE)
+    if (old_stream != nullptr && old_stream != stderr)
     {
-        tr_sys_file_close(old_log_file);
+        fclose(old_stream);
     }
 
     return true;
@@ -421,7 +419,7 @@ void tr_daemon::report_status()
 
 void tr_daemon::periodic_update()
 {
-    pumpLogMessages(logfile_, logfile_flush_);
+    pumpLogMessages(log_stream_);
     report_status();
 }
 
@@ -710,7 +708,7 @@ int tr_daemon::start([[maybe_unused]] bool foreground)
             _("Couldn't initialize daemon: {error} ({error_code})"),
             fmt::arg("error", tr_strerror(error_code)),
             fmt::arg("error_code", error_code));
-        printMessage(logfile_, TR_LOG_ERROR, MyName, errmsg, __FILE__, __LINE__);
+        printMessage(log_stream_, TR_LOG_ERROR, MyName, errmsg, __FILE__, __LINE__);
         cleanup_signals(sig_ev);
         return 1;
     }
@@ -870,7 +868,7 @@ CLEANUP:
 
     tr_sessionSaveSettings(my_session_, cdir, settings_);
     tr_sessionClose(my_session_);
-    pumpLogMessages(logfile_, logfile_flush_);
+    pumpLogMessages(log_stream_);
     printf(" done.\n");
 
     /* shutdown */
@@ -912,10 +910,9 @@ bool tr_daemon::init(int argc, char const* const argv[], bool* foreground, int*
         return false;
     }
 
-    if (*foreground && logfile_ == TR_BAD_SYS_FILE)
+    if (*foreground && log_stream_ == nullptr)
     {
-        logfile_ = tr_sys_file_get_std(TR_STD_SYS_FILE_ERR);
+        log_stream_ = stderr;
-        logfile_flush_ = tr_sys_file_flush_possible(logfile_);
     }
 
     if (dumpSettings)
@@ -930,7 +927,7 @@ bool tr_daemon::init(int argc, char const* const argv[], bool* foreground, int*
 void tr_daemon::handle_error(tr_error const& error) const
 {
     auto const errmsg = fmt::format("Couldn't daemonize: {:s} ({:d})", error.message(), error.code());
-    printMessage(logfile_, TR_LOG_ERROR, MyName, errmsg, __FILE__, __LINE__);
+    printMessage(log_stream_, TR_LOG_ERROR, MyName, errmsg, __FILE__, __LINE__);
 }
 
 int tr_main(int argc, char* argv[])

daemon/daemon.h

@@ -5,6 +5,7 @@
 
 #pragma once
 
+#include <cstdio>
 #include <string>
 
 #ifdef HAVE_SYS_SIGNALFD_H
@@ -13,7 +14,6 @@
 
 #include <libtransmission/variant.h>
 #include <libtransmission/quark.h>
-#include <libtransmission/file.h>
 
 struct event_base;
 struct tr_error;
@@ -50,11 +50,10 @@ private:
     bool seen_hup_ = false;
     std::string config_dir_;
     tr_variant settings_ = {};
-    bool logfile_flush_ = false;
     tr_session* my_session_ = nullptr;
     char const* log_file_name_ = nullptr;
     struct event_base* ev_base_ = nullptr;
-    tr_sys_file_t logfile_ = TR_BAD_SYS_FILE;
+    FILE* log_stream_ = nullptr;
 
     bool parse_args(int argc, char const* const* argv, bool* dump_settings, bool* foreground, int* exit_code);
     bool reopen_log_file(char const* filename);

libtransmission/file-posix.cc

@@ -545,37 +545,6 @@ char* tr_sys_path_native_separators(char* path)
     return path;
 }
 
-tr_sys_file_t tr_sys_file_get_std(tr_std_sys_file_t std_file, tr_error* error)
-{
-    tr_sys_file_t ret = TR_BAD_SYS_FILE;
-
-    switch (std_file)
-    {
-    case TR_STD_SYS_FILE_IN:
-        ret = STDIN_FILENO;
-        break;
-
-    case TR_STD_SYS_FILE_OUT:
-        ret = STDOUT_FILENO;
-        break;
-
-    case TR_STD_SYS_FILE_ERR:
-        ret = STDERR_FILENO;
-        break;
-
-    default:
-        TR_ASSERT_MSG(false, fmt::format("unknown standard file {:d}", static_cast<int>(std_file)));
-
-        if (error != nullptr)
-        {
-            error->set_from_errno(EINVAL);
-        }
-        break;
-    }
-
-    return ret;
-}
-
 tr_sys_file_t tr_sys_file_open(char const* path, int flags, int permissions, tr_error* error)
 {
     TR_ASSERT(path != nullptr);
@@ -811,23 +780,6 @@ bool tr_sys_file_flush(tr_sys_file_t handle, tr_error* error)
     return ret;
 }
 
-bool tr_sys_file_flush_possible(tr_sys_file_t handle, tr_error* error)
-{
-    TR_ASSERT(handle != TR_BAD_SYS_FILE);
-
-    if (struct stat statbuf = {}; fstat(handle, &statbuf) == 0)
-    {
-        return S_ISREG(statbuf.st_mode);
-    }
-
-    if (error != nullptr)
-    {
-        error->set_from_errno(errno);
-    }
-
-    return false;
-}
-
 bool tr_sys_file_truncate(tr_sys_file_t handle, uint64_t size, tr_error* error)
 {
     TR_ASSERT(handle != TR_BAD_SYS_FILE);

libtransmission/file-win32.cc

@@ -819,42 +819,6 @@ char* tr_sys_path_native_separators(char* path)
     return path;
 }
 
-tr_sys_file_t tr_sys_file_get_std(tr_std_sys_file_t std_file, tr_error* error)
-{
-    tr_sys_file_t ret = TR_BAD_SYS_FILE;
-
-    switch (std_file)
-    {
-    case TR_STD_SYS_FILE_IN:
-        ret = GetStdHandle(STD_INPUT_HANDLE);
-        break;
-
-    case TR_STD_SYS_FILE_OUT:
-        ret = GetStdHandle(STD_OUTPUT_HANDLE);
-        break;
-
-    case TR_STD_SYS_FILE_ERR:
-        ret = GetStdHandle(STD_ERROR_HANDLE);
-        break;
-
-    default:
-        TR_ASSERT_MSG(false, fmt::format("unknown standard file {:d}", std_file));
-        set_system_error(error, ERROR_INVALID_PARAMETER);
-        return TR_BAD_SYS_FILE;
-    }
-
-    if (ret == TR_BAD_SYS_FILE)
-    {
-        set_system_error(error, GetLastError());
-    }
-    else if (ret == nullptr)
-    {
-        ret = TR_BAD_SYS_FILE;
-    }
-
-    return ret;
-}
-
 tr_sys_file_t tr_sys_file_open(char const* path, int flags, int /*permissions*/, tr_error* error)
 {
     TR_ASSERT(path != nullptr);
@@ -1097,21 +1061,6 @@ bool tr_sys_file_flush(tr_sys_file_t handle, tr_error* error)
     return ret;
 }
 
-bool tr_sys_file_flush_possible(tr_sys_file_t handle, tr_error* error)
-{
-    TR_ASSERT(handle != TR_BAD_SYS_FILE);
-
-    DWORD type = GetFileType(handle);
-
-    if (type == FILE_TYPE_UNKNOWN)
-    {
-        set_system_error(error, GetLastError());
-        return false;
-    }
-
-    return type == FILE_TYPE_DISK;
-}
-
 bool tr_sys_file_truncate(tr_sys_file_t handle, uint64_t size, tr_error* error)
 {
     TR_ASSERT(handle != TR_BAD_SYS_FILE);

libtransmission/file.cc

@@ -11,22 +11,6 @@
 #include "libtransmission/file.h"
 #include "libtransmission/tr-assert.h"
 
-using namespace std::literals;
-
-#ifdef _WIN32
-static auto constexpr NativeEol = "\r\n"sv;
-#else
-static auto constexpr NativeEol = "\n"sv;
-#endif
-
-bool tr_sys_file_write_line(tr_sys_file_t handle, std::string_view buffer, tr_error* error)
-{
-    TR_ASSERT(handle != TR_BAD_SYS_FILE);
-
-    return tr_sys_file_write(handle, std::data(buffer), std::size(buffer), nullptr, error) &&
-        tr_sys_file_write(handle, std::data(NativeEol), std::size(NativeEol), nullptr, error);
-}
-
 std::vector<std::string> tr_sys_dir_get_files(
     std::string_view folder,
     std::function<bool(std::string_view)> const& test,

libtransmission/file.h

@@ -45,13 +45,6 @@ using tr_sys_dir_t = tr_sys_dir_win32*;
 /** @brief Platform-specific invalid directory descriptor constant. */
 #define TR_BAD_SYS_DIR ((tr_sys_dir_t) nullptr)
 
-enum tr_std_sys_file_t
-{
-    TR_STD_SYS_FILE_IN,
-    TR_STD_SYS_FILE_OUT,
-    TR_STD_SYS_FILE_ERR
-};
-
 enum tr_sys_file_open_flags_t
 {
     TR_SYS_FILE_READ = (1 << 0),
@@ -301,19 +294,6 @@ char* tr_sys_path_native_separators(char* path);
 
 /* File-related wrappers */
 
-/**
- * @brief Get handle to one of standard I/O files.
- *
- * @param[in]  std_file Standard file identifier.
- * @param[out] error    Pointer to error object. Optional, pass `nullptr` if you
- *                      are not interested in error details.
- *
- * @return Opened file descriptor on success, `TR_BAD_SYS_FILE` otherwise (with
- *         `error` set accordingly). DO NOT pass this descriptor to
- *         @ref tr_sys_file_close (unless you know what you are doing).
- */
-tr_sys_file_t tr_sys_file_get_std(tr_std_sys_file_t std_file, tr_error* error = nullptr);
-
 /**
  * @brief Portability wrapper for `open()`.
  *
@@ -448,9 +428,6 @@ bool tr_sys_file_write_at(
  */
 bool tr_sys_file_flush(tr_sys_file_t handle, tr_error* error = nullptr);
 
-/* @brief Check whether `handle` may be flushed via `tr_sys_file_flush()`. */
-bool tr_sys_file_flush_possible(tr_sys_file_t handle, tr_error* error = nullptr);
-
 /**
  * @brief Portability wrapper for `ftruncate()`.
  *
@@ -491,26 +468,6 @@ bool tr_sys_file_preallocate(tr_sys_file_t handle, uint64_t size, int flags, tr_
  */
 bool tr_sys_file_lock(tr_sys_file_t handle, int operation, tr_error* error = nullptr);
 
-/* File-related wrappers (utility) */
-
-/**
- * @brief Portability wrapper for `fputs()`, appending EOL internally.
- *
- * Special care should be taken when writing to one of standard output streams
- * (@ref tr_std_sys_file_t) since no UTF-8 conversion is currently being made.
- *
- * Writing to other streams (files, pipes) also leaves data untouched, so it
- * should already be in UTF-8 encoding, or whichever else you expect.
- *
- * @param[in]  handle Valid file descriptor.
- * @param[in]  buffer String to write.
- * @param[out] error  Pointer to error object. Optional, pass `nullptr` if you
- *                    are not interested in error details.
- *
- * @return `True` on success, `false` otherwise (with `error` set accordingly).
- */
-bool tr_sys_file_write_line(tr_sys_file_t handle, std::string_view buffer, tr_error* error = nullptr);
-
 /* Directory-related wrappers */
 
 /**

libtransmission/log.cc

@@ -134,26 +134,17 @@ void logAddImpl(
     }
     else
     {
-        static auto const fp = tr_sys_file_get_std(TR_STD_SYS_FILE_ERR);
-        if (fp == TR_BAD_SYS_FILE)
-        {
-            return;
-        }
-
         auto timestr = std::array<char, 64U>{};
         tr_logGetTimeStr(std::data(timestr), std::size(timestr));
 
-        auto buf = tr_strbuf<char, 2048U>{};
         if (std::empty(name))
         {
-            fmt::format_to(std::back_inserter(buf), "[{:s}] {:s}", std::data(timestr), msg);
+            fmt::print(stderr, "[{:s}] {:s}\n", std::data(timestr), msg);
         }
         else
         {
-            fmt::format_to(std::back_inserter(buf), "[{:s}] {:s}: {:s}", std::data(timestr), name, msg);
+            fmt::print("[{:s}] {:s}: {:s}\n", std::data(timestr), name, msg);
         }
-        tr_sys_file_write_line(fp, buf);
-        tr_sys_file_flush(fp);
     }
 #endif
 }

tests/libtransmission/subprocess-test-program.cc

@@ -3,9 +3,12 @@
 // or any future license endorsed by Mnemosyne LLC.
 // License text can be found in the licenses/ folder.
 
-#include <libtransmission/file.h> // tr_sys_file_write_line(), tr_sys_file_close(), tr_sy...
+#include <libtransmission/file.h> // tr_sys_dir_get_current()
 #include <libtransmission/utils.h> // tr_env_get_string()
 
+#include <fmt/core.h>
+
+#include <cstdio>
 #include <string>
 
 int main(int argc, char** argv)
@@ -19,13 +22,8 @@ int main(int argc, char** argv)
     auto const test_action = std::string{ argv[2] };
     auto const tmp_result_path = result_path + ".tmp";
 
-    auto fd = tr_sys_file_open(
+    FILE* out = std::fopen(tmp_result_path.c_str(), "w+");
-        tmp_result_path.data(), // NOLINT
+    if (out == nullptr)
-        TR_SYS_FILE_WRITE | TR_SYS_FILE_CREATE | TR_SYS_FILE_TRUNCATE,
-        0644,
-        nullptr);
-
-    if (fd == TR_BAD_SYS_FILE)
     {
         return 1;
     }
@@ -34,30 +32,35 @@ int main(int argc, char** argv)
     {
         for (int i = 3; i < argc; ++i)
         {
-            tr_sys_file_write_line(fd, argv[i]);
+            fmt::print(out, "{:s}\n", argv[i]);
         }
     }
     else if (test_action == "--dump-env")
     {
         for (int i = 3; i < argc; ++i)
         {
-            auto const value = tr_env_get_string(argv[i], "<null>");
+            fmt::print(out, "{:s}\n", tr_env_get_string(argv[i], "<null>"));
-            tr_sys_file_write_line(fd, value);
         }
     }
     else if (test_action == "--dump-cwd")
     {
-        auto const value = tr_sys_dir_get_current(nullptr);
+        auto cwd = tr_sys_dir_get_current(nullptr);
-        tr_sys_file_write_line(fd, !std::empty(value) ? value : "<null>");
+
+        if (std::empty(cwd))
+        {
+            cwd = "<null>";
+        }
+
+        fmt::print(out, "{:s}\n", cwd);
     }
     else
     {
-        tr_sys_file_close(fd);
+        std::fclose(out);
-        tr_sys_path_remove(tmp_result_path.data());
+        std::remove(tmp_result_path.c_str());
         return 1;
     }
 
-    tr_sys_file_close(fd);
+    std::fclose(out);
-    tr_sys_path_rename(tmp_result_path.data(), result_path.data());
+    tr_sys_path_rename(tmp_result_path.c_str(), result_path.c_str());
     return 0;
 }