Parse multicall commands once per call

Keep the change to d, f, p and t multicalls. Parse each column lazily on first use and evaluate it through rpc::call_object, preserving per-target argument expansion.

Compared unpatched and patched builds on macOS: output was byte-identical across the test harness, including an erase-mid-call check.

configure.ac

@@ -1,6 +1,6 @@
 m4_pattern_allow([PKG_CHECK_EXISTS])
 
-AC_INIT([rtorrent],[0.16.24],[sundell.software@gmail.com])
+AC_INIT([rtorrent],[0.16.25],[sundell.software@gmail.com])
 
 AC_CONFIG_HEADERS([config.h])
 AC_CONFIG_MACRO_DIRS([scripts])
@@ -14,7 +14,7 @@ AX_CXX_COMPILE_STDCXX(20, noext, mandatory)
 
 PKG_PROG_PKG_CONFIG
 
-AC_DEFINE([API_VERSION], [28], [api version])
+AC_DEFINE([API_VERSION], [29], [api version])
 
 RAK_CHECK_CFLAGS
 RAK_CHECK_CXXFLAGS
@@ -49,7 +49,7 @@ fi
 
 PKG_CHECK_MODULES([CPPUNIT], [cppunit],, [no_cppunit="yes"])
 PKG_CHECK_MODULES([ZLIB], [zlib])
-PKG_CHECK_MODULES([DEPENDENCIES], [libtorrent >= 0.16.24])
+PKG_CHECK_MODULES([DEPENDENCIES], [libtorrent >= 0.16.25])
 
 AC_LANG_PUSH(C++)
 TORRENT_WITH_XMLRPC_C

src/command_download.cc

@@ -334,6 +334,11 @@ f_multicall(core::Download* download, const torrent::Object::list_type& args) {
 
   bool use_regex = true;
 
+  rpc::preparsed_commands commands([&args](auto& cmds) {
+    for (auto cItr = ++args.begin(); cItr != args.end(); ++cItr)
+      cmds.push_back(rpc::parse_command_object(cItr->as_string()));
+  });
+
   if (args.front().is_list())
     for (const auto& o : args.front().as_list())
       regex_list.push_back(o.as_string_c());
@@ -349,10 +354,10 @@ f_multicall(core::Download* download, const torrent::Object::list_type& args) {
 
     torrent::Object::list_type& row = result.insert(result.end(), torrent::Object::create_list())->as_list();
 
-    for (torrent::Object::list_const_iterator cItr = ++args.begin(); cItr != args.end(); cItr++) {
-      const std::string& cmd = cItr->as_string();
-      row.push_back(rpc::parse_command(rpc::make_target(file.get()), cmd.c_str(), cmd.c_str() + cmd.size()).first);
-    }
+    // Defer parsing until a file actually matches the multicall selection.
+    commands.prepare_if_needed();
+    for (auto& itr : commands)
+      row.push_back(rpc::call_object(itr, rpc::make_target(file.get())));
   }
 
   return resultRaw;
@@ -372,6 +377,11 @@ t_multicall(core::Download* download, const torrent::Object::list_type& args) {
   auto  result_raw = torrent::Object::create_list();
   auto& result     = result_raw.as_list();
 
+  rpc::preparsed_commands commands([&args](auto& cmds) {
+    for (auto cItr = ++args.begin(); cItr != args.end(); ++cItr)
+      cmds.push_back(rpc::parse_command_object(cItr->as_string()));
+  });
+
   for (uint32_t idx = 0, last = download->tracker_list_size(); idx < last; idx++) {
     auto& row     = result.insert(result.end(), torrent::Object::create_list())->as_list();
     auto  tracker = download->tracker_controller().at(idx);
@@ -379,11 +389,10 @@ t_multicall(core::Download* download, const torrent::Object::list_type& args) {
     if (!tracker.is_valid())
       continue;
 
-    for (auto cItr = ++args.begin(); cItr != args.end(); cItr++) {
-      auto& cmd = cItr->as_string();
-
-      row.push_back(rpc::parse_command(rpc::make_target(&tracker), cmd.c_str(), cmd.c_str() + cmd.size()).first);
-    }
+    // Do not parse columns when there are no valid tracker targets.
+    commands.prepare_if_needed();
+    for (auto& itr : commands)
+      row.push_back(rpc::call_object(itr, rpc::make_target(&tracker)));
   }
 
   return result_raw;
@@ -405,13 +414,18 @@ p_multicall(core::Download* download, const torrent::Object::list_type& args) {
   auto*      connection_list = download->connection_list();
   const auto change_counter  = connection_list->change_counter();
 
+  rpc::preparsed_commands commands([&args](auto& cmds) {
+    for (auto cItr = ++args.begin(); cItr != args.end(); ++cItr)
+      cmds.push_back(rpc::parse_command_object(cItr->as_string()));
+  });
+
   for (const auto& connection : *connection_list) {
     torrent::Object::list_type& row = result.insert(result.end(), torrent::Object::create_list())->as_list();
 
-    for (auto cItr = ++args.begin(); cItr != args.end(); cItr++) {
-      const std::string& cmd = cItr->as_string();
-
-      row.push_back(rpc::parse_command(rpc::make_target(connection), cmd.c_str(), cmd.c_str() + cmd.size()).first);
+    // Prepare only after a peer exists, preserving empty-list laziness.
+    commands.prepare_if_needed();
+    for (auto& itr : commands) {
+      row.push_back(rpc::call_object(itr, rpc::make_target(connection)));
 
       // Erasing a peer frees it and swaps the last element into its place, so
       // neither this peer nor the iteration survives a change to the list.

src/command_events.cc

@@ -247,20 +247,26 @@ d_multicall(const torrent::Object::list_type& args) {
   torrent::Object             resultRaw = torrent::Object::create_list();
   torrent::Object::list_type& result = resultRaw.as_list();
 
+  rpc::preparsed_commands commands([&args](auto& cmds) {
+    for (auto cItr = ++args.begin(); cItr != args.end(); ++cItr)
+      cmds.push_back(rpc::parse_command_object(cItr->as_string()));
+  });
+
   for (const auto& download : dlist) {
     if (download.use_count() == 1)
       continue;
 
     torrent::Object::list_type& row = result.insert(result.end(), torrent::Object::create_list())->as_list();
 
-    for (torrent::Object::list_const_iterator cItr = ++args.begin(); cItr != args.end(); cItr++) {
+    // Skip parsing if there are no usable download targets in the view.
+    commands.prepare_if_needed();
+    for (auto& itr : commands) {
       // A command may erase this download, which destroys the torrent object it
       // wraps; the list dropping its reference is what tells us.
       if (download.use_count() == 1)
         break;
 
-      auto& cmd = cItr->as_string();
-      row.push_back(rpc::parse_command(rpc::make_target(download), cmd.c_str(), cmd.c_str() + cmd.size()).first);
+      row.push_back(rpc::call_object(itr, rpc::make_target(download)));
     }
   }
 

src/command_system.cc

@@ -86,8 +86,10 @@ initialize_command_system() {
   CMD_VAR_VALUE   ("system.file.split_size",       -1);
   CMD_VAR_STRING  ("system.file.split_suffix",     ".part");
 
-  CMD_ANY         ("system.file_name.replace_slash",     [](auto, auto)      { return torrent::runtime::client_config()->file_name_replace_slash(); });
-  CMD_ANY_STRING_V("system.file_name.replace_slash.set", [](auto, auto& str) { return torrent::runtime::client_config()->set_file_name_replace_slash(str); });
+  CMD_ANY         ("system.file_name.replace_slash",         [](auto, auto)        { return torrent::runtime::client_config()->file_name_replace_slash(); });
+  CMD_ANY_STRING_V("system.file_name.replace_slash.set",     [](auto, auto& str)   { return torrent::runtime::client_config()->set_file_name_replace_slash(str); });
+  CMD_ANY         ("system.file_name.allow_legacy_utf8",     [](auto, auto)        { return torrent::runtime::client_config()->file_name_allow_legacy_utf8(); });
+  CMD_ANY_VALUE_V ("system.file_name.allow_legacy_utf8.set", [](auto, auto& value) { return torrent::runtime::client_config()->set_file_name_allow_legacy_utf8(value); });
 
   CMD_ANY         ("system.file_status_cache.size",      [](auto, auto)      { return control->core()->file_status_cache()->size(); });
   CMD_ANY_V       ("system.file_status_cache.prune",     [](auto, auto)      { return control->core()->file_status_cache()->prune(); });
@@ -167,10 +169,13 @@ initialize_command_system() {
   rpc::rpc.mark_safe("system.time");
   rpc::rpc.mark_safe("system.time_seconds");
   rpc::rpc.mark_safe("system.time_usec");
+  rpc::rpc.mark_safe("system.torrent_name.use_sanitized");
   rpc::rpc.mark_safe("system.file.allocate");
   rpc::rpc.mark_safe("system.file.max_size");
   rpc::rpc.mark_safe("system.file.split_size");
   rpc::rpc.mark_safe("system.file.split_suffix");
+  rpc::rpc.mark_safe("system.file_name.replace_slash");
+  rpc::rpc.mark_safe("system.file_name.allow_legacy_utf8");
 
   rpc::rpc.mark_safe("system.sockets.size");
   rpc::rpc.mark_safe("system.sockets.max_size");

src/core/download_list.h

@@ -1,6 +1,7 @@
 #ifndef RTORRENT_CORE_DOWNLOAD_LIST_H
 #define RTORRENT_CORE_DOWNLOAD_LIST_H
 
+#include <cstdint>
 #include <iosfwd>
 #include <list>
 #include <memory>

src/rpc/command.h

@@ -108,7 +108,7 @@ public:
 
     static stack_type* from_data(char* data) { return reinterpret_cast<stack_type*>(data); }
 
-    char buffer[sizeof(torrent::Object) * max_arguments];
+    alignas(optimal_alignment) char buffer[sizeof(torrent::Object) * max_arguments];
   };
 
   command_base() : m_copy_helper(nullptr), m_dest_helper(nullptr) {}

src/rpc/parse_commands.cc

@@ -123,6 +123,39 @@ parse_command(target_type target, const char* first, const char* last) {
   return std::make_pair(commands.call_command(key, args, target), first);
 }
 
+torrent::Object
+parse_command_object(const char* first, const char* last) {
+  first = std::find_if(first, last, [&](char c) { return !command_map_is_space(c); });
+
+  if (first == last || *first == '#')
+    return torrent::Object();
+
+  char key[128];
+
+  first = parse_command_name(first, last, key, key + 128);
+  first = std::find_if(first, last, [&](char c) { return !command_map_is_space(c); });
+
+  if (first == last || *first != '=')
+    throw torrent::input_error("Could not find '=' in command '" + std::string(key) + "'.");
+
+  torrent::Object result = torrent::Object::create_dict_key();
+
+  result.as_dict_key() = key;
+
+  first = parse_whole_list(first + 1, last, &result.as_dict_obj(), &parse_is_delim_command);
+
+  // Find the last character that is part of this command, skipping
+  // the whitespace at the end.
+  first = std::find_if(first, last, [&](char c) { return !command_map_is_space(c); });
+
+  // This helper accepts exactly one command and cannot return where a next
+  // command begins, so reject every non-whitespace suffix, including ';'.
+  if (first != last && *first != '\0')
+    throw torrent::input_error("Junk at end of input.");
+
+  return result;
+}
+
 torrent::Object
 parse_command_multiple(target_type target, const char* first, const char* last) {
   parse_command_type result;

src/rpc/parse_commands.h

@@ -37,6 +37,9 @@
 
 #include <string>
 #include <cstring>
+#include <functional>
+#include <utility>
+#include <vector>
 
 #include "xmlrpc.h"
 #include "rpc_manager.h"
@@ -67,6 +70,34 @@ parse_command_single(target_type target, const std::string& cmd) {
   return parse_command(target, cmd.c_str(), cmd.c_str() + cmd.size()).first;
 }
 
+// Parse one RPC command without executing it. Repeated evaluations can use
+// call_object on the result, which handles per-target argument expansion.
+torrent::Object        parse_command_object(const char* first, const char* last);
+
+inline torrent::Object parse_command_object(const std::string& cmd) {
+  return parse_command_object(cmd.c_str(), cmd.c_str() + cmd.size());
+}
+
+// Prepare a multicall's commands once, on the first target that uses them. This
+// keeps empty target lists from parsing commands that would never be evaluated.
+struct preparsed_commands : public std::vector<torrent::Object> {
+  explicit preparsed_commands(std::function<void(preparsed_commands&)> prepare)
+    : m_prepare(std::move(prepare)) {}
+
+  void prepare_if_needed() {
+    if (m_prepare) {
+      // Clear before invoking: the callback may inspect this vector, and a
+      // throwing callback must not be run again against partially added items.
+      auto prepare = std::move(m_prepare);
+      m_prepare = {};
+      prepare(*this);
+    }
+  }
+
+private:
+  std::function<void(preparsed_commands&)> m_prepare;
+};
+
 inline torrent::Object
 parse_command_multiple_std(const std::string& cmd, target_type target = rpc::make_target()) {
   return parse_command_multiple(target, cmd.c_str(), cmd.c_str() + cmd.size());

test/rpc/test_command.cc

@@ -2,7 +2,10 @@
 
 #include "test/rpc/test_command.h"
 
+#include <torrent/exceptions.h>
+
 #include "rpc/command.h"
+#include "rpc/parse_commands.h"
 
 CPPUNIT_TEST_SUITE_REGISTRATION(TestCommand);
 
@@ -83,3 +86,50 @@ TestCommand::test_stack_double() {
   rpc::command_base::pop_stack(&stack_first, last_stack_first);
   CPPUNIT_ASSERT(command_stack_all_empty());
 }
+
+void
+TestCommand::test_preparsed_commands() {
+  unsigned int prepare_count = 0;
+  rpc::preparsed_commands commands([&prepare_count](auto& prepared) {
+    ++prepare_count;
+    // Reentrant access must not invoke the same callback recursively.
+    prepared.prepare_if_needed();
+    prepared.push_back(rpc::parse_command_object("string.length=abc"));
+  });
+
+  CPPUNIT_ASSERT_EQUAL(0u, prepare_count);
+  CPPUNIT_ASSERT(commands.empty());
+
+  commands.prepare_if_needed();
+  CPPUNIT_ASSERT_EQUAL(1u, prepare_count);
+
+  size_t count = 0;
+  for (auto& itr : commands) {
+    CPPUNIT_ASSERT(itr.is_dict_key());
+    ++count;
+  }
+
+  CPPUNIT_ASSERT_EQUAL(size_t(1), count);
+  CPPUNIT_ASSERT_EQUAL(1u, prepare_count);
+
+  for (auto& itr : commands)
+    CPPUNIT_ASSERT(itr.is_dict_key());
+
+  CPPUNIT_ASSERT_EQUAL(1u, prepare_count);
+}
+
+void
+TestCommand::test_parse_command_object() {
+  auto command = rpc::parse_command_object("\tstring.length=abc  ");
+  CPPUNIT_ASSERT(command.is_dict_key());
+  CPPUNIT_ASSERT_EQUAL(std::string("string.length"), command.as_dict_key());
+
+  // This helper has no way to return the next-command pointer, so it must not
+  // silently accept a multipart command separated by ';'.
+  CPPUNIT_ASSERT_THROW(rpc::parse_command_object("string.length=abc;string.length=def"), torrent::input_error);
+
+  // Unlike parse_command (which parses command files), this helper handles one
+  // multicall command and must reject newline boundaries, including CRLF.
+  CPPUNIT_ASSERT_THROW(rpc::parse_command_object("string.length=abc\nstring.length=def"), torrent::input_error);
+  CPPUNIT_ASSERT_THROW(rpc::parse_command_object("string.length=abc\r\nstring.length=def"), torrent::input_error);
+}

test/rpc/test_command.h

@@ -5,10 +5,14 @@ class TestCommand : public test_fixture {
 
   CPPUNIT_TEST(test_stack);
   CPPUNIT_TEST(test_stack_double);
+  CPPUNIT_TEST(test_preparsed_commands);
+  CPPUNIT_TEST(test_parse_command_object);
 
   CPPUNIT_TEST_SUITE_END();
 
 public:
   void test_stack();
   void test_stack_double();
+  void test_preparsed_commands();
+  void test_parse_command_object();
 };