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

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

gtk/FileList.cc

@@ -37,6 +37,7 @@
 #include <algorithm>
 #include <list>
 #include <memory>
+#include <optional>
 #include <queue>
 #include <stack>
 #include <string>
@@ -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<std::string> 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<std::string> 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)));
+        return;
     }
 
-            if (handled = !filename.empty(); handled)
+    auto const iter = store_->get_iter(path);
+    if (!iter)
     {
-                gtr_open_file(filename);
-            }
-        }
+        return;
     }
 
-    // return handled;
+    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)

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<bool>{};
+    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<bool>{};
+    while (!result)
     {
-        native_operation |= LOCK_SH;
+        if (flock(handle, native_operation) != -1)
+        {
+            result = true;
         }
-
-    if ((operation & TR_SYS_FILE_LOCK_EX) != 0)
+        else if (errno != EINTR)
         {
-        native_operation |= LOCK_EX;
+            result = false;
         }
-
-    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<bool>{ 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)

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<size_t>(n_read) == sizeof(child_errno));
         set_system_error(error, child_errno, "Child process setup");
         return false;
     }
 
+    // read failed (what to do?)
     return true;
 }
 } // namespace

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  }

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  }

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 = {};