From 468de87076c3c1fc4cd46026051b2380488b6775 Mon Sep 17 00:00:00 2001 From: Charles Kerr Date: Sat, 20 Jan 2024 16:56:42 -0600 Subject: [PATCH] refactor: fix cppcoreguidelines-avoid-do-while warnings (#6527) * fix: avoid do-while in tr_sys_file_lock() * fix: avoid do-while in BitfieldTest * chore: set cppcoreguidelines-avoid-do-while.IgnoreMacros * fix: avoid do-while in FileList::Impl::onRowActivated() * fix: avoid do-while in tr_spawn_async_in_parent() * fix: avoid do-while in handle_sigchld() * fixup! fix: avoid do-while in tr_spawn_async_in_parent() * fixup! fix: avoid do-while in FileList::Impl::onRowActivated() * fixup! fix: avoid do-while in tr_spawn_async_in_parent() fix fd leak regression * fixup! fix: avoid do-while in tr_spawn_async_in_parent() --- gtk/.clang-tidy | 5 +- gtk/FileList.cc | 66 +++++++++++++++---------- libtransmission/file-posix.cc | 68 +++++++++++++------------- libtransmission/subprocess-posix.cc | 46 +++++++++-------- qt/.clang-tidy | 1 + tests/libtransmission/.clang-tidy | 1 + tests/libtransmission/bitfield-test.cc | 32 ++++-------- 7 files changed, 112 insertions(+), 107 deletions(-) diff --git a/gtk/.clang-tidy b/gtk/.clang-tidy index fafe6ed79..6ba3829d7 100644 --- a/gtk/.clang-tidy +++ b/gtk/.clang-tidy @@ -23,5 +23,6 @@ Checks: > -readability-redundant-access-specifiers CheckOptions: - misc-non-private-member-variables-in-classes.IgnoreClassesWithAllMemberVariablesBeingPublic: true - modernize-pass-by-value.ValuesOnly: true + - { key: cppcoreguidelines-avoid-do-while.IgnoreMacros, value: true } + - { key: misc-non-private-member-variables-in-classes.IgnoreClassesWithAllMemberVariablesBeingPublic, value: true } + - { key: modernize-pass-by-value.ValuesOnly, value: true } diff --git a/gtk/FileList.cc b/gtk/FileList.cc index 4e98f4770..e64beaeae 100644 --- a/gtk/FileList.cc +++ b/gtk/FileList.cc @@ -37,6 +37,7 @@ #include #include #include +#include #include #include #include @@ -658,7 +659,7 @@ void renderPriority(Gtk::CellRenderer* renderer, Gtk::TreeModel::const_iterator } /* build a filename from tr_torrentGetCurrentDir() + the model's FC_LABELs */ -std::string buildFilename(tr_torrent const* tor, Gtk::TreeModel::iterator const& iter) +std::string build_filename(tr_torrent const* tor, Gtk::TreeModel::iterator const& iter) { std::vector tokens; for (auto child = iter; child; child = child->parent()) @@ -671,37 +672,52 @@ std::string buildFilename(tr_torrent const* tor, Gtk::TreeModel::iterator const& return Glib::build_filename(tokens); } +std::optional get_filename_to_open(tr_torrent const* tor, Gtk::TreeModel::iterator const& iter) +{ + auto file = Gio::File::create_for_path(build_filename(tor, iter)); + + // if the selected file is complete, use it + if (iter->get_value(file_cols.prog) == 100 && file->query_exists()) + { + return file->get_path(); + } + + // use nearest existing ancestor instead + for (;;) + { + file = file->get_parent(); + + if (!file) + { + return {}; + } + + if (file->query_exists()) + { + return file->get_path(); + } + } +} } // namespace void FileList::Impl::onRowActivated(Gtk::TreeModel::Path const& path, Gtk::TreeViewColumn* /*col*/) { - bool handled = false; - - if (auto const* tor = core_->find_torrent(torrent_id_); tor != nullptr) + auto const* const tor = core_->find_torrent(torrent_id_); + if (tor == nullptr) { - if (auto const iter = store_->get_iter(path); iter) - { - auto filename = buildFilename(tor, iter); - auto const prog = iter->get_value(file_cols.prog); - - /* if the file's not done, walk up the directory tree until we find - * an ancestor that exists, and open that instead */ - if (!filename.empty() && (prog < 100 || !Glib::file_test(filename, TR_GLIB_FILE_TEST(EXISTS)))) - { - do - { - filename = Glib::path_get_dirname(filename); - } while (!filename.empty() && !Glib::file_test(filename, TR_GLIB_FILE_TEST(EXISTS))); - } - - if (handled = !filename.empty(); handled) - { - gtr_open_file(filename); - } - } + return; } - // return handled; + auto const iter = store_->get_iter(path); + if (!iter) + { + return; + } + + if (auto const filename = get_filename_to_open(tor, iter); filename) + { + gtr_open_file(*filename); + } } bool FileList::Impl::onViewPathToggled(Gtk::TreeViewColumn* col, Gtk::TreeModel::Path const& path) diff --git a/libtransmission/file-posix.cc b/libtransmission/file-posix.cc index 54d80935b..0468a43a0 100644 --- a/libtransmission/file-posix.cc +++ b/libtransmission/file-posix.cc @@ -977,8 +977,6 @@ bool tr_sys_file_lock([[maybe_unused]] tr_sys_file_t handle, [[maybe_unused]] in TR_ASSERT( !!(operation & TR_SYS_FILE_LOCK_SH) + !!(operation & TR_SYS_FILE_LOCK_EX) + !!(operation & TR_SYS_FILE_LOCK_UN) == 1); - bool ret = false; - #if defined(F_OFD_SETLK) struct flock fl = {}; @@ -1000,58 +998,60 @@ bool tr_sys_file_lock([[maybe_unused]] tr_sys_file_t handle, [[maybe_unused]] in fl.l_whence = SEEK_SET; - do - { - ret = fcntl(handle, (operation & TR_SYS_FILE_LOCK_NB) != 0 ? F_OFD_SETLK : F_OFD_SETLKW, &fl) != -1; - } while (!ret && errno == EINTR); + int const native_operation = (operation & TR_SYS_FILE_LOCK_NB) != 0 ? F_OFD_SETLK : F_OFD_SETLKW; - if (!ret && errno == EAGAIN) + auto result = std::optional{}; + while (!result) { - errno = EWOULDBLOCK; + if (fcntl(handle, native_operation, &fl) != -1) + { + result = true; + } + else if (errno != EINTR) + { + result = false; + } } #elif defined(HAVE_FLOCK) - int native_operation = 0; + int const native_operation = // + (((operation & TR_SYS_FILE_LOCK_SH) != 0) ? LOCK_SH : 0) | // + (((operation & TR_SYS_FILE_LOCK_EX) != 0) ? LOCK_EX : 0) | // + (((operation & TR_SYS_FILE_LOCK_NB) != 0) ? LOCK_NB : 0) | // + (((operation & TR_SYS_FILE_LOCK_UN) != 0) ? LOCK_UN : 0); - if ((operation & TR_SYS_FILE_LOCK_SH) != 0) + auto result = std::optional{}; + while (!result) { - native_operation |= LOCK_SH; + if (flock(handle, native_operation) != -1) + { + result = true; + } + else if (errno != EINTR) + { + result = false; + } } - if ((operation & TR_SYS_FILE_LOCK_EX) != 0) - { - native_operation |= LOCK_EX; - } - - if ((operation & TR_SYS_FILE_LOCK_NB) != 0) - { - native_operation |= LOCK_NB; - } - - if ((operation & TR_SYS_FILE_LOCK_UN) != 0) - { - native_operation |= LOCK_UN; - } - - do - { - ret = flock(handle, native_operation) != -1; - } while (!ret && errno == EINTR); - #else errno = ENOSYS; - ret = false; + auto const result = std::optional{ false }; #endif - if (error != nullptr && !ret) + if (!*result && errno == EAGAIN) + { + errno = EWOULDBLOCK; + } + + if (error != nullptr && !*result) { error->set_from_errno(errno); } - return ret; + return *result; } std::string tr_sys_dir_get_current(tr_error* error) diff --git a/libtransmission/subprocess-posix.cc b/libtransmission/subprocess-posix.cc index 0b407cd2e..8c9591f83 100644 --- a/libtransmission/subprocess-posix.cc +++ b/libtransmission/subprocess-posix.cc @@ -29,15 +29,18 @@ namespace { void handle_sigchld(int /*i*/) { - int rc = 0; - - do + for (;;) { - /* FIXME: Only check for our own PIDs */ - rc = waitpid(-1, nullptr, WNOHANG); - } while (rc > 0 || (rc == -1 && errno == EINTR)); + // FIXME: only check for our own PIDs + auto const res = waitpid(-1, nullptr, WNOHANG); - /* FIXME: Call old handler, if any */ + if ((res == 0) || (res == -1 && errno != EINTR)) + { + break; + } + } + + // FIXME: Call old handler, if any } void set_system_error(tr_error* error, int code, std::string_view what) @@ -82,34 +85,29 @@ void set_system_error(tr_error* error, int code, std::string_view what) [[nodiscard]] bool tr_spawn_async_in_parent(int pipe_fd, tr_error* error) { - int child_errno = 0; - ssize_t count = 0; - - static_assert(sizeof(child_errno) == sizeof(errno)); - - do + auto child_errno = int{}; + auto n_read = ssize_t{}; + for (auto done = false; !done;) { - count = read(pipe_fd, &child_errno, sizeof(child_errno)); - } while (count == -1 && errno == EINTR); + n_read = read(pipe_fd, &child_errno, sizeof(child_errno)); + done = n_read != -1 || errno != EINTR; + } close(pipe_fd); - if (count == -1) + if (n_read == 0) // child successfully exec'ed { - /* Read failed (what to do?) */ + return true; } - else if (count == 0) - { - /* Child successfully exec-ed */ - } - else - { - TR_ASSERT((size_t)count == sizeof(child_errno)); + if (n_read > 0) // child errno was set + { + TR_ASSERT(static_cast(n_read) == sizeof(child_errno)); set_system_error(error, child_errno, "Child process setup"); return false; } + // read failed (what to do?) return true; } } // namespace diff --git a/qt/.clang-tidy b/qt/.clang-tidy index 39b76cb64..05053fcf2 100644 --- a/qt/.clang-tidy +++ b/qt/.clang-tidy @@ -38,6 +38,7 @@ Checks: > -readability-redundant-access-specifiers, # keep: 'private' vs 'private slots' CheckOptions: + - { key: cppcoreguidelines-avoid-do-while.IgnoreMacros, value: true } - { key: readability-identifier-naming.ClassCase, value: CamelCase } - { key: readability-identifier-naming.ClassMethodCase, value: camelBack } - { key: readability-identifier-naming.ConstexprVariableCase, value: CamelCase } diff --git a/tests/libtransmission/.clang-tidy b/tests/libtransmission/.clang-tidy index 6f39896a9..ac0d561e1 100644 --- a/tests/libtransmission/.clang-tidy +++ b/tests/libtransmission/.clang-tidy @@ -34,6 +34,7 @@ Checks: > -readability-qualified-auto, CheckOptions: + - { key: cppcoreguidelines-avoid-do-while.IgnoreMacros, value: true } - { key: readability-identifier-naming.ClassCase, value: CamelCase } - { key: readability-identifier-naming.ClassMethodCase, value: camelBack } - { key: readability-identifier-naming.ConstexprVariableCase, value: CamelCase } diff --git a/tests/libtransmission/bitfield-test.cc b/tests/libtransmission/bitfield-test.cc index 83d62129a..7350fcf28 100644 --- a/tests/libtransmission/bitfield-test.cc +++ b/tests/libtransmission/bitfield-test.cc @@ -17,34 +17,22 @@ TEST(Bitfield, count) { - auto constexpr IterCount = int{ 10000 }; + auto constexpr IterCount = size_t{ 10000U }; - for (auto i = 0; i < IterCount; ++i) + for (size_t i = 0; i < IterCount; ++i) { - auto const bit_count = 100U + tr_rand_int(1000U); - // generate a random bitfield - tr_bitfield bf(bit_count); - - for (size_t j = 0, n = tr_rand_int(bit_count); j < n; ++j) + auto const bit_count = 100U + tr_rand_int(1000U); + auto bf = tr_bitfield{ bit_count }; + for (size_t idx = 0U; idx < bit_count; ++idx) { - bf.set(tr_rand_int(bit_count)); + bf.set(idx, tr_rand_int(2U) != 0U); } - int begin = tr_rand_int(bit_count); - int end = 0; - do - { - end = tr_rand_int(bit_count); - } while (end == begin); - - // ensure end <= begin - if (end < begin) - { - int const tmp = begin; - begin = end; - end = tmp; - } + // pick arbitrary endpoints in the 1st and 2nd half of the bitfield + auto const midpt = bit_count / 2U; + auto const begin = tr_rand_int(midpt); + auto const end = midpt + tr_rand_int(midpt); // test the bitfield unsigned long count1 = {};