Merge 299305e128 into 6c1cee5f79

gtk/OptionsDialog.cc

@@ -107,7 +107,7 @@ void OptionsDialog::Impl::removeOldTorrent()
     if (tor_ != nullptr)
     {
         file_list_->clear();
-        tr_torrentRemove(tor_, false, nullptr, nullptr);
+        tr_torrentRemove(tor_, false, nullptr, nullptr, nullptr, nullptr);
         tor_ = nullptr;
     }
 }

gtk/Session.cc

@@ -32,9 +32,12 @@
 #include <glibmm/i18n.h>
 #include <glibmm/main.h>
 #include <glibmm/miscutils.h>
+#include <glibmm/refptr.h>
 #include <glibmm/stringutils.h>
 #include <glibmm/variant.h>
 
+#include <glib.h>
+
 #if GTKMM_CHECK_VERSION(4, 0, 0)
 #include <gtkmm/sortlistmodel.h>
 #else
@@ -87,6 +90,8 @@ public:
     void add_torrent(Glib::RefPtr<Torrent> const& torrent, bool do_notify);
     bool add_from_url(Glib::ustring const& url);
 
+    void remove_torrent(tr_torrent_id_t id, bool delete_files);
+
     void send_rpc_request(tr_variant const& request, int64_t tag, std::function<void(tr_variant&)> const& response_func);
 
     void commit_prefs_change(tr_quark key);
@@ -905,10 +910,56 @@ void Session::torrent_changed(tr_torrent_id_t id)
 
 void Session::remove_torrent(tr_torrent_id_t id, bool delete_files)
 {
-    if (auto const& [torrent, position] = impl_->find_torrent_by_id(id); torrent)
+    impl_->remove_torrent(id, delete_files);
+}
+
+void Session::Impl::remove_torrent(tr_torrent_id_t id, bool delete_files)
+{
+    struct CallbackUserData
     {
-        /* remove from the gui */
-        impl_->get_raw_model()->remove(position);
+        Glib::RefPtr<Session> session;
+        tr_torrent_id_t id;
+        bool succeeded = false;
+    };
+
+    if (auto const& [torrent, position] = find_torrent_by_id(id); torrent)
+    {
+        // This is the callback called from the Gtk main thread. If successfull,
+        // it removes the torrent entry from the GUI.
+        auto callback_this_thread = [](gpointer user_data) -> gboolean
+        {
+            // Take ownership of the raw pointer, so it gets deleted when done.
+            auto ud = std::unique_ptr<CallbackUserData>(static_cast<CallbackUserData*>(user_data));
+            if (ud->succeeded)
+            {
+                auto const& impl = *ud->session->impl_;
+                if (auto const& [torrent_, position_] = impl.find_torrent_by_id(ud->id); torrent_)
+                {
+                    /* remove from the gui */
+                    impl.get_raw_model()->remove(position_);
+                }
+            }
+
+            return G_SOURCE_REMOVE;
+        };
+        // Convert it explicitly to a constexpr C function pointer so it can be
+        // used from inside the next lambda without capturing (so that itself
+        // can be used as a C function pointer).
+        constexpr GSourceFunc CALLBACK_THIS_THREAD_PTR = callback_this_thread;
+
+        // This is the callback called from the libtransmission thread, it
+        // stores the result and schedules the callback to be called from the
+        // main thread.
+        auto callback_other_thread = [](bool succeeded, void* user_data)
+        {
+            // Store the result for the Gtk thread callback to use:
+            auto* ud = static_cast<CallbackUserData*>(user_data);
+            ud->succeeded = succeeded;
+
+            // Unfortunatelly, there is no thread-safe C++ binding to
+            // this function, so we have to use the C API directly.
+            g_idle_add(CALLBACK_THIS_THREAD_PTR, user_data);
+        };
 
         /* remove the torrent */
         tr_torrentRemove(
@@ -916,7 +967,9 @@ void Session::remove_torrent(tr_torrent_id_t id, bool delete_files)
             delete_files,
             [](char const* filename, void* /*user_data*/, tr_error* error)
             { return gtr_file_trash_or_remove(filename, error); },
-            nullptr);
+            nullptr,
+            callback_other_thread,
+            new CallbackUserData{ get_core_ptr(), id });
     }
 }
 

libtransmission/rpcimpl.cc

@@ -248,7 +248,7 @@ char const* torrentRemove(tr_session* session, tr_variant::Map const& args_in, t
     {
         if (auto const status = session->rpcNotify(type, tor); (status & TR_RPC_NOREMOVE) == 0)
         {
-            tr_torrentRemove(tor, delete_flag, nullptr, nullptr);
+            tr_torrentRemove(tor, delete_flag, nullptr, nullptr, nullptr, nullptr);
         }
     }
 

libtransmission/torrent-files.cc

@@ -233,7 +233,8 @@ bool tr_torrent_files::move(
  * 2. If there are nontorrent files, don't delete them...
  * 3. ...unless the other files are "junk", such as .DS_Store
  */
-void tr_torrent_files::remove(std::string_view parent_in, std::string_view tmpdir_prefix, FileFunc const& func) const
+void tr_torrent_files::remove(std::string_view parent_in, std::string_view tmpdir_prefix, FileFunc const& func, tr_error* error)
+    const
 {
     auto const parent = tr_pathbuf{ parent_in };
 
@@ -243,17 +244,37 @@ void tr_torrent_files::remove(std::string_view parent_in, std::string_view tmpdi
         return;
     }
 
-    // make a tmpdir
+    // try to make a tmpdir
     auto tmpdir = tr_pathbuf{ parent, '/', tmpdir_prefix, "__XXXXXX"sv };
-    tr_sys_dir_create_temp(std::data(tmpdir));
+    if (!tr_sys_dir_create_temp(std::data(tmpdir), error))
+    {
+        return;
+    }
 
     // move the local data to the tmpdir
-    auto const paths = std::array<std::string_view, 1>{ parent.sv() };
-    for (tr_file_index_t idx = 0, n_files = fileCount(); idx < n_files; ++idx)
     {
-        if (auto const found = find(idx, std::data(paths), std::size(paths)); found)
+        struct moved_file
         {
-            tr_file_move(found->filename(), tr_pathbuf{ tmpdir, '/', found->subpath() });
+            tr_pathbuf from;
+            tr_pathbuf to;
+        };
+        std::vector<moved_file> moved_files;
+
+        auto const paths = std::array<std::string_view, 1>{ parent.sv() };
+        for (tr_file_index_t idx = 0, n_files = fileCount(); idx < n_files; ++idx)
+        {
+            if (auto const found = find(idx, std::data(paths), std::size(paths)); found)
+            {
+                moved_file f{ found->filename(), tr_pathbuf{ tmpdir, '/', found->subpath() } };
+
+                // if moving a file fails, give up and let the error propagate
+                if (!tr_file_move_strict(f.from, f.to, error))
+                {
+                    return;
+                }
+
+                moved_files.push_back(f);
+            }
         }
     }
 

libtransmission/torrent-files.h

@@ -117,7 +117,8 @@ public:
         tr_error* error = nullptr) const;
 
     using FileFunc = std::function<void(char const* filename)>;
-    void remove(std::string_view parent_in, std::string_view tmpdir_prefix, FileFunc const& func) const;
+    void remove(std::string_view parent_in, std::string_view tmpdir_prefix, FileFunc const& func, tr_error* error = nullptr)
+        const;
 
     struct FoundFile : public tr_sys_path_info
     {

libtransmission/torrent.cc

@@ -607,32 +607,6 @@ bool removeTorrentFile(char const* filename, void* /*user_data*/, tr_error* erro
     return tr_sys_path_remove(filename, error);
 }
 
-void removeTorrentInSessionThread(tr_torrent* tor, bool delete_flag, tr_fileFunc delete_func, void* user_data)
-{
-    auto const lock = tor->unique_lock();
-
-    if (delete_flag && tor->has_metainfo())
-    {
-        // ensure the files are all closed and idle before moving
-        tor->session->close_torrent_files(tor->id());
-        tor->session->verify_remove(tor);
-
-        if (delete_func == nullptr)
-        {
-            delete_func = removeTorrentFile;
-        }
-
-        auto const delete_func_wrapper = [&delete_func, user_data](char const* filename)
-        {
-            delete_func(filename, user_data, nullptr);
-        };
-
-        tor->files().remove(tor->current_dir(), tor->name(), delete_func_wrapper);
-    }
-
-    tr_torrentFreeInSessionThread(tor);
-}
-
 void freeTorrent(tr_torrent* tor)
 {
     using namespace queue_helpers;
@@ -780,6 +754,59 @@ void tr_torrent::stop_now()
     set_is_queued(false);
 }
 
+void tr_torrentRemoveInSessionThread(
+    tr_torrent* tor,
+    bool delete_flag,
+    tr_fileFunc delete_func,
+    void* delete_user_data,
+    tr_successNotifyFunc notify_func,
+    void* notify_user_data)
+{
+    auto const lock = tor->unique_lock();
+
+    bool ok = true;
+    if (delete_flag && tor->has_metainfo())
+    {
+        // ensure the files are all closed and idle before moving
+        tor->session->close_torrent_files(tor->id());
+        tor->session->verify_remove(tor);
+
+        if (delete_func == nullptr)
+        {
+            delete_func = start_stop_helpers::removeTorrentFile;
+        }
+
+        auto const delete_func_wrapper = [&delete_func, delete_user_data](char const* filename)
+        {
+            delete_func(filename, delete_user_data, nullptr);
+        };
+
+        tr_error error;
+        tor->files().remove(tor->current_dir(), tor->name(), delete_func_wrapper, &error);
+        if (error)
+        {
+            ok = false;
+            tor->is_deleting_ = false;
+
+            tor->error().set_local_error(fmt::format(
+                _("Couldn't remove all torrent files: {error} ({error_code})"),
+                fmt::arg("error", error.message()),
+                fmt::arg("error_code", error.code())));
+            tr_torrentStop(tor);
+        }
+    }
+
+    if (ok)
+    {
+        tr_torrentFreeInSessionThread(tor);
+    }
+
+    if (notify_func != nullptr)
+    {
+        notify_func(ok, notify_user_data);
+    }
+}
+
 void tr_torrentStop(tr_torrent* tor)
 {
     if (!tr_isTorrent(tor))
@@ -794,7 +821,13 @@ void tr_torrentStop(tr_torrent* tor)
     tor->session->run_in_session_thread([tor]() { tor->stop_now(); });
 }
 
-void tr_torrentRemove(tr_torrent* tor, bool delete_flag, tr_fileFunc delete_func, void* user_data)
+void tr_torrentRemove(
+    tr_torrent* tor,
+    bool delete_flag,
+    tr_fileFunc delete_func,
+    void* delete_user_data,
+    tr_successNotifyFunc notify_func,
+    void* notify_user_data)
 {
     using namespace start_stop_helpers;
 
@@ -802,7 +835,14 @@ void tr_torrentRemove(tr_torrent* tor, bool delete_flag, tr_fileFunc delete_func
 
     tor->is_deleting_ = true;
 
-    tor->session->run_in_session_thread(removeTorrentInSessionThread, tor, delete_flag, delete_func, user_data);
+    tor->session->run_in_session_thread(
+        tr_torrentRemoveInSessionThread,
+        tor,
+        delete_flag,
+        delete_func,
+        delete_user_data,
+        notify_func,
+        notify_user_data);
 }
 
 void tr_torrentFreeInSessionThread(tr_torrent* tor)

libtransmission/torrent.h

@@ -977,7 +977,20 @@ private:
     friend tr_torrent* tr_torrentNew(tr_ctor* ctor, tr_torrent** setme_duplicate_of);
     friend uint64_t tr_torrentGetBytesLeftToAllocate(tr_torrent const* tor);
     friend void tr_torrentFreeInSessionThread(tr_torrent* tor);
-    friend void tr_torrentRemove(tr_torrent* tor, bool delete_flag, tr_fileFunc delete_func, void* user_data);
+    friend void tr_torrentRemoveInSessionThread(
+        tr_torrent* tor,
+        bool delete_flag,
+        tr_fileFunc delete_func,
+        void* delete_user_data,
+        tr_successNotifyFunc notify_func,
+        void* notify_user_data);
+    friend void tr_torrentRemove(
+        tr_torrent* tor,
+        bool delete_flag,
+        tr_fileFunc delete_func,
+        void* delete_user_data,
+        tr_successNotifyFunc notify_func,
+        void* notify_user_data);
     friend void tr_torrentSetDownloadDir(tr_torrent* tor, char const* path);
     friend void tr_torrentSetPriority(tr_torrent* tor, tr_priority_t priority);
     friend void tr_torrentStart(tr_torrent* tor);

libtransmission/transmission.h

@@ -829,8 +829,16 @@ tr_torrent* tr_torrentNew(tr_ctor* ctor, tr_torrent** setme_duplicate_of);
 
 using tr_fileFunc = bool (*)(char const* filename, void* user_data, tr_error* error);
 
+using tr_successNotifyFunc = void (*)(bool succeeded, void* user_data);
+
 /** @brief Removes our torrent and .resume files for this torrent */
-void tr_torrentRemove(tr_torrent* torrent, bool delete_flag, tr_fileFunc delete_func, void* user_data);
+void tr_torrentRemove(
+    tr_torrent* torrent,
+    bool delete_flag,
+    tr_fileFunc delete_func,
+    void* delete_user_data,
+    tr_successNotifyFunc notify_func,
+    void* notify_user_data);
 
 /** @brief Start a torrent */
 void tr_torrentStart(tr_torrent* torrent);

libtransmission/utils.cc

@@ -557,11 +557,13 @@ std::string tr_strratio(double ratio, char const* infinity)
 
 // ---
 
-bool tr_file_move(std::string_view oldpath_in, std::string_view newpath_in, tr_error* error)
+namespace
+{
+namespace tr_file_move_impl
 {
-    auto const oldpath = tr_pathbuf{ oldpath_in };
-    auto const newpath = tr_pathbuf{ newpath_in };
 
+bool check_paths(std::string_view const oldpath, std::string_view const newpath, tr_error* error)
+{
     auto local_error = tr_error{};
     if (error == nullptr)
     {
@@ -582,7 +584,7 @@ bool tr_file_move(std::string_view oldpath_in, std::string_view newpath_in, tr_e
     }
 
     // ensure the target directory exists
-    auto newdir = tr_pathbuf{ newpath.sv() };
+    auto newdir = tr_pathbuf{ newpath };
     newdir.popdir();
     if (!tr_sys_dir_create(newdir, TR_SYS_DIR_CREATE_PARENTS, 0777, error))
     {
@@ -590,6 +592,42 @@ bool tr_file_move(std::string_view oldpath_in, std::string_view newpath_in, tr_e
         return false;
     }
 
+    return true;
+}
+
+} // namespace tr_file_move_impl
+} // namespace
+
+bool tr_file_move_strict(std::string_view oldpath_in, std::string_view newpath_in, tr_error* error)
+{
+    if (!tr_file_move_impl::check_paths(oldpath_in, newpath_in, error))
+    {
+        return false;
+    }
+
+    auto const oldpath = tr_pathbuf{ oldpath_in };
+    auto const newpath = tr_pathbuf{ newpath_in };
+
+    /* do the actual moving */
+    if (!tr_sys_path_rename(oldpath, newpath, error))
+    {
+        error->prefix_message("Unable to move file: ");
+        return false;
+    }
+
+    return true;
+}
+
+bool tr_file_move(std::string_view oldpath_in, std::string_view newpath_in, tr_error* error)
+{
+    if (!tr_file_move_impl::check_paths(oldpath_in, newpath_in, error))
+    {
+        return false;
+    }
+
+    auto const oldpath = tr_pathbuf{ oldpath_in };
+    auto const newpath = tr_pathbuf{ newpath_in };
+
     /* they might be on the same filesystem... */
     if (tr_sys_path_rename(oldpath, newpath))
     {

libtransmission/utils.h

@@ -63,6 +63,18 @@ std::optional<std::locale> tr_locale_set_global(std::locale const& locale) noexc
 
 bool tr_file_read(std::string_view filename, std::vector<char>& contents, tr_error* error = nullptr);
 
+/**
+ * Tries to move a file by renaming, but never by copying.
+ *
+ * Creates the destination directory if it doesn't exist.
+ */
+bool tr_file_move_strict(std::string_view oldpath_in, std::string_view newpath_in, tr_error* error = nullptr);
+
+/**
+ * Tries to move a file by renaming, and if that fails, by copying.
+ *
+ * Creates the destination directory if it doesn't exist.
+ */
 bool tr_file_move(std::string_view oldpath, std::string_view newpath, tr_error* error = nullptr);
 
 bool tr_file_save(std::string_view filename, std::string_view contents, tr_error* error = nullptr);

macosx/Torrent.mm

@@ -192,7 +192,7 @@ bool trashDataFile(char const* filename, void* /*user_data*/, tr_error* error)
     //allow the file to be indexed by Time Machine
     [self setTimeMachineExclude:NO];
 
-    tr_torrentRemove(self.fHandle, trashFiles, trashDataFile, nullptr);
+    tr_torrentRemove(self.fHandle, trashFiles, trashDataFile, nullptr, nullptr, nullptr);
 }
 
 - (void)changeDownloadFolderBeforeUsing:(NSString*)folder determinationType:(TorrentDeterminationType)determinationType

tests/libtransmission/move-test.cc

@@ -134,7 +134,7 @@ TEST_P(IncompleteDirTest, incompleteDir)
     }
 
     // cleanup
-    tr_torrentRemove(tor, true, nullptr, nullptr);
+    tr_torrentRemove(tor, true, nullptr, nullptr, nullptr, nullptr);
 }
 
 INSTANTIATE_TEST_SUITE_P(
@@ -188,7 +188,7 @@ TEST_F(MoveTest, setLocation)
     }
 
     // cleanup
-    tr_torrentRemove(tor, true, nullptr, nullptr);
+    tr_torrentRemove(tor, true, nullptr, nullptr, nullptr, nullptr);
 }
 
 } // namespace libtransmission::test

tests/libtransmission/rename-test.cc

@@ -38,7 +38,7 @@ class RenameTest : public SessionTest
 protected:
     void torrentRemoveAndWait(tr_torrent* tor, size_t expected_torrent_count)
     {
-        tr_torrentRemove(tor, false, nullptr, nullptr);
+        tr_torrentRemove(tor, false, nullptr, nullptr, nullptr, nullptr);
         auto const test = [this, expected_torrent_count]()
         {
             return std::size(session_->torrents()) == expected_torrent_count;

tests/libtransmission/rpc-test.cc

@@ -120,7 +120,7 @@ TEST_F(RpcTest, tagAsync)
     EXPECT_EQ(*tag, 12345);
 
     // cleanup
-    tr_torrentRemove(tor, false, nullptr, nullptr);
+    tr_torrentRemove(tor, false, nullptr, nullptr, nullptr, nullptr);
 }
 
 TEST_F(RpcTest, tagNoHandler)
@@ -259,7 +259,7 @@ TEST_F(RpcTest, sessionGet)
     EXPECT_EQ(decltype(unexpected_keys){}, unexpected_keys);
 
     // cleanup
-    tr_torrentRemove(tor, false, nullptr, nullptr);
+    tr_torrentRemove(tor, false, nullptr, nullptr, nullptr, nullptr);
 }
 
 TEST_F(RpcTest, torrentGet)
@@ -298,7 +298,7 @@ TEST_F(RpcTest, torrentGet)
     EXPECT_EQ(1, first_torrent_id);
 
     // cleanup
-    tr_torrentRemove(tor, false, nullptr, nullptr);
+    tr_torrentRemove(tor, false, nullptr, nullptr, nullptr, nullptr);
 }
 
 } // namespace libtransmission::test