fix: app defaults should override libtransmission defaults (#6495)

cli/cli.cc

@@ -321,7 +321,7 @@ int tr_main(int argc, char* argv[])
 
     /* load the defaults from config file + libtransmission defaults */
     auto const config_dir = getConfigDir(argc, (char const**)argv);
-    auto settings = tr_sessionLoadSettings(config_dir.c_str(), MyConfigName);
+    auto settings = tr_sessionLoadSettings(nullptr, config_dir.c_str(), MyConfigName);
 
     /* the command line overrides defaults */
     if (parseCommandLine(&settings, argc, (char const**)argv) != 0)

daemon/daemon.cc

@@ -6,7 +6,6 @@
 #include <array>
 #include <cerrno>
 #include <cstdio> /* printf */
-#include <cstdlib> /* atoi */
 #include <iostream>
 #include <iterator> /* std::back_inserter */
 #include <memory>
@@ -360,6 +359,14 @@ tr_rpc_callback_status on_rpc_callback(tr_session* /*session*/, tr_rpc_callback_
     }
     return TR_RPC_OK;
 }
+
+tr_variant load_settings(char const* config_dir)
+{
+    auto app_defaults = tr_variant::make_map();
+    tr_variantDictAddBool(&app_defaults, TR_KEY_rpc_enabled, true);
+    return tr_sessionLoadSettings(&app_defaults, config_dir, MyName);
+}
+
 } // namespace
 
 bool tr_daemon::reopen_log_file(char const* filename)
@@ -394,7 +401,7 @@ bool tr_daemon::reopen_log_file(char const* filename)
     return true;
 }
 
-void tr_daemon::report_status(void)
+void tr_daemon::report_status()
 {
     double const up = tr_sessionGetRawSpeed_KBps(my_session_, TR_UP);
     double const dn = tr_sessionGetRawSpeed_KBps(my_session_, TR_DOWN);
@@ -409,7 +416,7 @@ void tr_daemon::report_status(void)
     }
 }
 
-void tr_daemon::periodic_update(void)
+void tr_daemon::periodic_update()
 {
     pumpLogMessages(logfile_, logfile_flush_);
     report_status();
@@ -498,7 +505,10 @@ bool tr_daemon::parse_args(int argc, char const* const* argv, bool* dump_setting
             break;
 
         case 'p':
-            tr_variantDictAddInt(&settings_, TR_KEY_rpc_port, atoi(optstr));
+            if (auto const rpc_port = tr_num_parse<uint16_t>(optstr); rpc_port)
+            {
+                tr_variantDictAddInt(&settings_, TR_KEY_rpc_port, *rpc_port);
+            }
             break;
 
         case 't':
@@ -522,7 +532,10 @@ bool tr_daemon::parse_args(int argc, char const* const* argv, bool* dump_setting
             break;
 
         case 'P':
-            tr_variantDictAddInt(&settings_, TR_KEY_peer_port, atoi(optstr));
+            if (auto const peer_port = tr_num_parse<uint16_t>(optstr); peer_port)
+            {
+                tr_variantDictAddInt(&settings_, TR_KEY_peer_port, *peer_port);
+            }
             break;
 
         case 'm':
@@ -534,11 +547,17 @@ bool tr_daemon::parse_args(int argc, char const* const* argv, bool* dump_setting
             break;
 
         case 'L':
-            tr_variantDictAddInt(&settings_, TR_KEY_peer_limit_global, atoi(optstr));
+            if (auto const peer_limit_global = tr_num_parse<int64_t>(optstr); peer_limit_global && *peer_limit_global >= 0)
+            {
+                tr_variantDictAddInt(&settings_, TR_KEY_peer_limit_global, *peer_limit_global);
+            }
             break;
 
         case 'l':
-            tr_variantDictAddInt(&settings_, TR_KEY_peer_limit_per_torrent, atoi(optstr));
+            if (auto const peer_limit_tor = tr_num_parse<int64_t>(optstr); peer_limit_tor && *peer_limit_tor >= 0)
+            {
+                tr_variantDictAddInt(&settings_, TR_KEY_peer_limit_per_torrent, *peer_limit_tor);
+            }
             break;
 
         case 800:
@@ -570,7 +589,10 @@ bool tr_daemon::parse_args(int argc, char const* const* argv, bool* dump_setting
             break;
 
         case 953:
-            tr_variantDictAddReal(&settings_, TR_KEY_ratio_limit, atof(optstr));
+            if (auto const ratio_limit = tr_num_parse<double>(optstr); optstr)
+            {
+                tr_variantDictAddReal(&settings_, TR_KEY_ratio_limit, *ratio_limit);
+            }
             tr_variantDictAddBool(&settings_, TR_KEY_ratio_limit_enabled, true);
             break;
 
@@ -640,7 +662,7 @@ bool tr_daemon::parse_args(int argc, char const* const* argv, bool* dump_setting
     return true;
 }
 
-void tr_daemon::reconfigure(void)
+void tr_daemon::reconfigure()
 {
     if (my_session_ == nullptr)
     {
@@ -660,35 +682,24 @@ void tr_daemon::reconfigure(void)
         configDir = tr_sessionGetConfigDir(my_session_);
         tr_logAddInfo(fmt::format(_("Reloading settings from '{path}'"), fmt::arg("path", configDir)));
 
-        auto newsettings = tr_variant::make_map();
+        tr_sessionSet(my_session_, load_settings(configDir));
-        tr_variantDictAddBool(&newsettings, TR_KEY_rpc_enabled, true);
-        newsettings.merge(tr_sessionLoadSettings(configDir, MyName));
-
-        tr_sessionSet(my_session_, newsettings);
         tr_sessionReloadBlocklists(my_session_);
     }
 }
 
-void tr_daemon::stop(void)
+void tr_daemon::stop()
 {
     event_base_loopexit(ev_base_, nullptr);
 }
 
 int tr_daemon::start([[maybe_unused]] bool foreground)
 {
-    bool boolVal;
-    bool pidfile_created = false;
-    tr_session* session = nullptr;
-    struct event* status_ev = nullptr;
-    struct event* sig_ev = nullptr;
-    auto watchdir = std::unique_ptr<Watchdir>{};
-    char const* const cdir = this->config_dir_.c_str();
-
     sd_notifyf(0, "MAINPID=%d\n", (int)getpid());
 
     /* setup event state */
     ev_base_ = event_base_new();
 
+    event* sig_ev = nullptr;
     if (ev_base_ == nullptr || !setup_signals(sig_ev))
     {
         auto const error_code = errno;
@@ -702,7 +713,8 @@ int tr_daemon::start([[maybe_unused]] bool foreground)
     }
 
     /* start the session */
-    session = tr_sessionInit(cdir, true, settings_);
+    auto const* const cdir = this->config_dir_.c_str();
+    auto* session = tr_sessionInit(cdir, true, settings_);
     tr_sessionSetRPCCallback(session, on_rpc_callback, this);
     tr_logAddInfo(fmt::format(_("Loading settings from '{path}'"), fmt::arg("path", cdir)));
     tr_sessionSaveSettings(session, cdir, settings_);
@@ -710,6 +722,7 @@ int tr_daemon::start([[maybe_unused]] bool foreground)
     auto sv = std::string_view{};
     (void)tr_variantDictFindStrView(&settings_, key_pidfile_, &sv);
     auto const sz_pid_filename = std::string{ sv };
+    auto pidfile_created = false;
     if (!std::empty(sz_pid_filename))
     {
         auto error = tr_error{};
@@ -737,7 +750,7 @@ int tr_daemon::start([[maybe_unused]] bool foreground)
         }
     }
 
-    if (tr_variantDictFindBool(&settings_, TR_KEY_rpc_authentication_required, &boolVal) && boolVal)
+    if (auto tmp_bool = false; tr_variantDictFindBool(&settings_, TR_KEY_rpc_authentication_required, &tmp_bool) && tmp_bool)
     {
         tr_logAddInfo(_("Requiring authentication"));
     }
@@ -751,7 +764,8 @@ int tr_daemon::start([[maybe_unused]] bool foreground)
     }
 
     /* maybe add a watchdir */
-    if (tr_variantDictFindBool(&settings_, TR_KEY_watch_dir_enabled, &boolVal) && boolVal)
+    auto watchdir = std::unique_ptr<Watchdir>{};
+    if (auto tmp_bool = false; tr_variantDictFindBool(&settings_, TR_KEY_watch_dir_enabled, &tmp_bool) && tmp_bool)
     {
         auto force_generic = bool{ false };
         (void)tr_variantDictFindBool(&settings_, key_watch_dir_force_generic_, &force_generic);
@@ -796,6 +810,7 @@ int tr_daemon::start([[maybe_unused]] bool foreground)
 #endif
 
     /* Create new timer event to report daemon status */
+    event* status_ev;
     {
         constexpr auto one_sec = timeval{ 1, 0 }; // 1 second
         status_ev = event_new(ev_base_, -1, EV_PERSIST, &::periodic_update, this);
@@ -882,9 +897,7 @@ bool tr_daemon::init(int argc, char const* const argv[], bool* foreground, int*
     config_dir_ = getConfigDir(argc, argv);
 
     /* load settings from defaults + config file */
-    settings_ = tr_variant::make_map();
+    settings_ = load_settings(config_dir_.c_str());
-    tr_variantDictAddBool(&settings_, TR_KEY_rpc_enabled, true);
-    settings_.merge(tr_sessionLoadSettings(config_dir_.c_str(), MyName));
 
     bool dumpSettings;
 
@@ -893,7 +906,7 @@ bool tr_daemon::init(int argc, char const* const argv[], bool* foreground, int*
     /* overwrite settings from the command line */
     if (!parse_args(argc, argv, &dumpSettings, foreground, ret))
     {
-        goto EXIT_EARLY;
+        return false;
     }
 
     if (*foreground && logfile_ == TR_BAD_SYS_FILE)
@@ -905,13 +918,10 @@ bool tr_daemon::init(int argc, char const* const argv[], bool* foreground, int*
     if (dumpSettings)
     {
         fmt::print("{:s}\n", tr_variant_serde::json().to_string(settings_));
-        goto EXIT_EARLY;
+        return false;
     }
 
     return true;
-
-EXIT_EARLY:
-    return false;
 }
 
 void tr_daemon::handle_error(tr_error const& error) const

gtk/Prefs.cc

@@ -108,8 +108,8 @@ tr_variant& getPrefs()
 
     if (!settings.has_value())
     {
-        settings = get_default_app_settings();
+        auto const app_defaults = get_default_app_settings();
-        settings.merge(tr_sessionLoadSettings(gl_confdir.c_str(), nullptr));
+        settings.merge(tr_sessionLoadSettings(&app_defaults, gl_confdir.c_str(), nullptr));
         ensure_sound_cmd_is_a_list(&settings);
     }
 

libtransmission/session.cc

@@ -476,10 +476,16 @@ tr_variant tr_sessionGetSettings(tr_session const* session)
     return settings;
 }
 
-tr_variant tr_sessionLoadSettings(char const* config_dir, char const* app_name)
+tr_variant tr_sessionLoadSettings(tr_variant const* app_defaults, char const* config_dir, char const* app_name)
 {
     auto settings = tr_sessionGetDefaultSettings();
 
+    // if app defaults are provided, override libtransmission defaults
+    if (app_defaults != nullptr && app_defaults->holds_alternative<tr_variant::Map>())
+    {
+        settings.merge(*app_defaults);
+    }
+
     // if a settings file exists, use it to override the defaults
     if (auto const filename = fmt::format(
             "{:s}/settings.json",
@@ -554,11 +560,11 @@ tr_session* tr_sessionInit(char const* config_dir, bool message_queueing_enabled
     // - client settings
     // - previous session's values in settings.json
     // - hardcoded defaults
-    auto settings = tr_sessionLoadSettings(config_dir, nullptr);
+    auto settings = tr_sessionLoadSettings(nullptr, config_dir, nullptr);
     settings.merge(client_settings);
 
     // if logging is desired, start it now before doing more work
-    if (auto const* settings_map = client_settings.get_if<tr_variant::Map>(); settings_map != nullptr)
+    if (auto const* settings_map = settings.get_if<tr_variant::Map>(); settings_map != nullptr)
     {
         if (auto const* val = settings_map->find_if<bool>(TR_KEY_message_level); val != nullptr)
         {

libtransmission/transmission.h

@@ -175,6 +175,7 @@ tr_variant tr_sessionGetSettings(tr_session const* session);
  *
  * TODO: if we ever make libtransmissionapp, this would go there.
  *
+ * @param app_defaults tr_variant containing the app defaults
  * @param config_dir the configuration directory to find settings.json
  * @param app_name if config_dir is empty, app_name is used to find the default dir.
  * @return the loaded settings
@@ -182,7 +183,7 @@ tr_variant tr_sessionGetSettings(tr_session const* session);
  * @see `tr_sessionInit()`
  * @see `tr_sessionSaveSettings()`
  */
-tr_variant tr_sessionLoadSettings(char const* config_dir, char const* app_name);
+tr_variant tr_sessionLoadSettings(tr_variant const* app_defaults, char const* config_dir, char const* app_name);
 
 /**
  * Add the session's configuration settings to the benc dictionary

qt/Prefs.cc

@@ -227,13 +227,13 @@ Prefs::Prefs(QString config_dir)
     // when the application exits.
     temporary_prefs_.insert(FILTER_TEXT);
 
-    auto top = get_default_app_settings();
+    auto const app_defaults = get_default_app_settings();
-    top.merge(tr_sessionLoadSettings(config_dir_.toUtf8().constData(), nullptr));
+    auto settings = tr_sessionLoadSettings(&app_defaults, config_dir_.toUtf8().constData(), nullptr);
-    ensureSoundCommandIsAList(&top);
+    ensureSoundCommandIsAList(&settings);
 
     for (int i = 0; i < PREFS_COUNT; ++i)
     {
-        tr_variant const* b = tr_variantDictFind(&top, Items[i].key);
+        tr_variant const* b = tr_variantDictFind(&settings, Items[i].key);
 
         switch (Items[i].type)
         {

qt/Session.cc

@@ -357,7 +357,7 @@ void Session::start()
     }
     else
     {
-        auto const settings = tr_sessionLoadSettings(config_dir_.toUtf8().constData(), "qt");
+        auto const settings = tr_sessionLoadSettings(nullptr, config_dir_.toUtf8().constData(), "qt");
         session_ = tr_sessionInit(config_dir_.toUtf8().constData(), true, settings);
 
         rpc_.start(session_);