1
0
Fork 0
mirror of https://github.com/transmission/transmission synced 2024-12-21 23:32:35 +00:00

Fails with an error if data removal was not possible (#6055)

* Do not remove torrent if trashing files failed.

Instead, stop the torrent and sets an error.

* Fixing GTK interface with new file removal behavior.

* C++17 compliant.

* Reverting unrelated change.

* Avoiding allocating unecessary objects.

* Easy review fixes.

* Fixing merge error.

* Adding result callback to tr_torrentRemove().

Using the new callback in Gtk GUI to decide when to remove it from the
interface.

* Reducing indentation level and making the function more readable.

* Using existing Session RefPtr.

* Notifying the client before freeing the torrent in the session.

* Addressing comments and synchronizing callback.

* Moving include.

* Fix constness issue reported by clang-tidy
This commit is contained in:
Lucas Clemente Vella 2024-08-13 05:26:09 +01:00 committed by GitHub
parent abfd39065c
commit 9fc9daf40d
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
14 changed files with 235 additions and 55 deletions

View file

@ -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;
}
}

View file

@ -32,6 +32,7 @@
#include <glibmm/i18n.h>
#include <glibmm/main.h>
#include <glibmm/miscutils.h>
#include <glibmm/refptr.h>
#include <glibmm/stringutils.h>
#include <glibmm/variant.h>
@ -51,6 +52,7 @@
#include <iostream>
#include <map>
#include <memory>
#include <mutex>
#include <optional>
#include <string>
#include <string_view>
@ -87,6 +89,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 +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> 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<CallbackUserData>(static_cast<CallbackUserData*>(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 });
}
}

View file

@ -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);
}
}

View file

@ -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 = 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_file> moved_files;
auto const paths = std::array<std::string_view, 1>{ 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);
}
}
}

View file

@ -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
{

View file

@ -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)

View file

@ -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);

View file

@ -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);

View file

@ -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))
{

View file

@ -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);

View file

@ -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

View file

@ -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

View file

@ -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;

View file

@ -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