diff --git a/gtk/OptionsDialog.cc b/gtk/OptionsDialog.cc index 69c781d07..63463f014 100644 --- a/gtk/OptionsDialog.cc +++ b/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; } } diff --git a/gtk/Session.cc b/gtk/Session.cc index 60cee431d..76347365e 100644 --- a/gtk/Session.cc +++ b/gtk/Session.cc @@ -32,6 +32,7 @@ #include #include #include +#include #include #include @@ -51,6 +52,7 @@ #include #include #include +#include #include #include #include @@ -87,6 +89,8 @@ public: void add_torrent(Glib::RefPtr 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 const& response_func); void commit_prefs_change(tr_quark key); @@ -905,10 +909,51 @@ 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) +{ + if (auto const& [torrent, position] = find_torrent_by_id(id); torrent) { - /* remove from the gui */ - impl_->get_raw_model()->remove(position); + struct CallbackUserData + { + Glib::RefPtr session; + tr_torrent_id_t id; + }; + + // Callback to remove the torrent entry from the GUI if it was + // successfuly removed. This is called from the libtransmission thread. + auto handle_result = [](bool succeeded, void* user_data) + { + std::mutex wait_cb_thread; + wait_cb_thread.lock(); + + // Schedule the actual handler in the main thread: + Glib::signal_idle().connect_once( + [=, &wait_cb_thread]() + { + // Take ownership of the raw pointer, so it gets deleted when done. + auto ud = std::unique_ptr(static_cast(user_data)); + + if (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_); + } + } + + wait_cb_thread.unlock(); + }); + + // It is better to wait for the idle signal to be processed before + // returning, just to avoid the extremely improbable case of the + // tr_torrent_id_t being reused before the idle signal is processed. + std::lock_guard waiter{ wait_cb_thread }; + }; /* remove the torrent */ tr_torrentRemove( @@ -916,7 +961,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, + handle_result, + new CallbackUserData{ get_core_ptr(), id }); } } diff --git a/libtransmission/rpcimpl.cc b/libtransmission/rpcimpl.cc index ce7f547b9..7b61f2023 100644 --- a/libtransmission/rpcimpl.cc +++ b/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); } } diff --git a/libtransmission/torrent-files.cc b/libtransmission/torrent-files.cc index bc653afa8..47bec1467 100644 --- a/libtransmission/torrent-files.cc +++ b/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{ parent.sv() }; - for (tr_file_index_t idx = 0, n_files = file_count(); 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_files; + + auto const paths = std::array{ parent.sv() }; + for (tr_file_index_t idx = 0, n_files = file_count(); idx < n_files; ++idx) + { + if (auto const found = find(idx, std::data(paths), std::size(paths)); found) + { + auto const f = moved_file{ 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); + } } } diff --git a/libtransmission/torrent-files.h b/libtransmission/torrent-files.h index 9ebfecf02..9b3244402 100644 --- a/libtransmission/torrent-files.h +++ b/libtransmission/torrent-files.h @@ -117,7 +117,8 @@ public: tr_error* error = nullptr) const; using FileFunc = std::function; - 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 { diff --git a/libtransmission/torrent.cc b/libtransmission/torrent.cc index 42352cb70..1d9ee36f2 100644 --- a/libtransmission/torrent.cc +++ b/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; @@ -784,6 +758,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_result_notify_func 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 (notify_func != nullptr) + { + notify_func(ok, notify_user_data); + } + + if (ok) + { + tr_torrentFreeInSessionThread(tor); + } +} + void tr_torrentStop(tr_torrent* tor) { if (!tr_isTorrent(tor)) @@ -798,7 +825,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_result_notify_func notify_func, + void* notify_user_data) { using namespace start_stop_helpers; @@ -806,7 +839,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) diff --git a/libtransmission/torrent.h b/libtransmission/torrent.h index 280ddcfce..967b59cde 100644 --- a/libtransmission/torrent.h +++ b/libtransmission/torrent.h @@ -984,7 +984,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_result_notify_func 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_result_notify_func 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); diff --git a/libtransmission/transmission.h b/libtransmission/transmission.h index 2c3730cb6..f00205f49 100644 --- a/libtransmission/transmission.h +++ b/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_result_notify_func = 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_result_notify_func notify_func, + void* notify_user_data); /** @brief Start a torrent */ void tr_torrentStart(tr_torrent* torrent); diff --git a/libtransmission/utils.cc b/libtransmission/utils.cc index 8c8f232ac..e6680d3c8 100644 --- a/libtransmission/utils.cc +++ b/libtransmission/utils.cc @@ -569,11 +569,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) { @@ -594,7 +596,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)) { @@ -602,6 +604,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)) { diff --git a/libtransmission/utils.h b/libtransmission/utils.h index f04029b21..46e9bdbb7 100644 --- a/libtransmission/utils.h +++ b/libtransmission/utils.h @@ -63,6 +63,18 @@ std::optional tr_locale_set_global(std::locale const& locale) noexc bool tr_file_read(std::string_view filename, std::vector& 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); diff --git a/macosx/Torrent.mm b/macosx/Torrent.mm index 212d1f069..a8a752f22 100644 --- a/macosx/Torrent.mm +++ b/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 diff --git a/tests/libtransmission/move-test.cc b/tests/libtransmission/move-test.cc index 96839d632..f8625bc73 100644 --- a/tests/libtransmission/move-test.cc +++ b/tests/libtransmission/move-test.cc @@ -135,7 +135,7 @@ TEST_P(IncompleteDirTest, incompleteDir) } // cleanup - tr_torrentRemove(tor, true, nullptr, nullptr); + tr_torrentRemove(tor, true, nullptr, nullptr, nullptr, nullptr); } INSTANTIATE_TEST_SUITE_P( @@ -189,7 +189,7 @@ TEST_F(MoveTest, setLocation) } // cleanup - tr_torrentRemove(tor, true, nullptr, nullptr); + tr_torrentRemove(tor, true, nullptr, nullptr, nullptr, nullptr); } } // namespace libtransmission::test diff --git a/tests/libtransmission/rename-test.cc b/tests/libtransmission/rename-test.cc index 7756c4598..a3bf73b25 100644 --- a/tests/libtransmission/rename-test.cc +++ b/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; diff --git a/tests/libtransmission/rpc-test.cc b/tests/libtransmission/rpc-test.cc index ad87e96ed..41b4ea4c3 100644 --- a/tests/libtransmission/rpc-test.cc +++ b/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