From 326d9f3daff90c99a04eb40633fac460ee73e09a Mon Sep 17 00:00:00 2001 From: Charles Kerr Date: Fri, 23 Sep 2022 08:39:13 -0500 Subject: [PATCH] refactor: cppcoreguidelines-avoid-goto (#3841) * refactor: do not use goto in file-win32.cc * refactor: do not use goto in subprocess-posix.cc * refactor: do not use goto in peer-io.cc * build: add cppcoreguidelines-avoid-goto to libtransmission/.clang-tidy --- libtransmission/.clang-tidy | 1 + libtransmission/file-win32.cc | 79 ++++++++++---------- libtransmission/peer-io.cc | 79 +++++++------------- libtransmission/subprocess-posix.cc | 16 ++-- libtransmission/tr-udp.cc | 111 ++++++++++++---------------- 5 files changed, 118 insertions(+), 168 deletions(-) diff --git a/libtransmission/.clang-tidy b/libtransmission/.clang-tidy index 0d61f6bd8..eaac13183 100644 --- a/libtransmission/.clang-tidy +++ b/libtransmission/.clang-tidy @@ -10,6 +10,7 @@ Checks: > -cert-err58-cpp, clang-analyzer-*, cppcoreguidelines-avoid-const-or-ref-data-members, + cppcoreguidelines-avoid-goto, cppcoreguidelines-init-variables, cppcoreguidelines-interfaces-global-init, cppcoreguidelines-no-malloc, diff --git a/libtransmission/file-win32.cc b/libtransmission/file-win32.cc index 490f0e417..b6394731b 100644 --- a/libtransmission/file-win32.cc +++ b/libtransmission/file-win32.cc @@ -8,6 +8,7 @@ #include // for isalpha() #include #include // for std::back_inserter +#include #include #include @@ -454,58 +455,54 @@ bool tr_sys_path_is_relative(std::string_view path) return true; } +static std::optional get_file_info(char const* path, tr_error** error) +{ + auto const wpath = path_to_native_path(path); + if (std::empty(wpath)) + { + set_system_error_if_file_found(error, GetLastError()); + return {}; + } + + auto const handle = CreateFileW(wpath.c_str(), 0, 0, nullptr, OPEN_EXISTING, FILE_FLAG_BACKUP_SEMANTICS, nullptr); + if (handle == INVALID_HANDLE_VALUE) + { + set_system_error_if_file_found(error, GetLastError()); + return {}; + } + + // TODO: Use GetFileInformationByHandleEx on >= Server 2012 + auto info = BY_HANDLE_FILE_INFORMATION{}; + if (!GetFileInformationByHandle(handle, &info)) + { + set_system_error_if_file_found(error, GetLastError()); + CloseHandle(handle); + return {}; + } + + CloseHandle(handle); + return info; +} + bool tr_sys_path_is_same(char const* path1, char const* path2, tr_error** error) { TR_ASSERT(path1 != nullptr); TR_ASSERT(path2 != nullptr); - bool ret = false; - HANDLE handle1 = INVALID_HANDLE_VALUE; - HANDLE handle2 = INVALID_HANDLE_VALUE; - BY_HANDLE_FILE_INFORMATION fi1, fi2; - - auto const wide_path1 = path_to_native_path(path1); - auto const wide_path2 = path_to_native_path(path2); - - if (std::empty(wide_path1) || std::empty(wide_path2)) + auto const fi1 = get_file_info(path1, error); + if (!fi1) { - goto fail; + return false; } - handle1 = CreateFileW(wide_path1.c_str(), 0, 0, nullptr, OPEN_EXISTING, FILE_FLAG_BACKUP_SEMANTICS, nullptr); - - if (handle1 == INVALID_HANDLE_VALUE) + auto const fi2 = get_file_info(path2, error); + if (!fi2) { - goto fail; + return false; } - handle2 = CreateFileW(wide_path2.c_str(), 0, 0, nullptr, OPEN_EXISTING, FILE_FLAG_BACKUP_SEMANTICS, nullptr); - - if (handle2 == INVALID_HANDLE_VALUE) - { - goto fail; - } - - /* TODO: Use GetFileInformationByHandleEx on >= Server 2012 */ - - if (!GetFileInformationByHandle(handle1, &fi1) || !GetFileInformationByHandle(handle2, &fi2)) - { - goto fail; - } - - ret = fi1.dwVolumeSerialNumber == fi2.dwVolumeSerialNumber && fi1.nFileIndexHigh == fi2.nFileIndexHigh && - fi1.nFileIndexLow == fi2.nFileIndexLow; - - goto cleanup; - -fail: - set_system_error_if_file_found(error, GetLastError()); - -cleanup: - CloseHandle(handle2); - CloseHandle(handle1); - - return ret; + return fi1->dwVolumeSerialNumber == fi2->dwVolumeSerialNumber && fi1->nFileIndexHigh == fi2->nFileIndexHigh && + fi1->nFileIndexLow == fi2->nFileIndexLow; } std::string tr_sys_path_resolve(std::string_view path, tr_error** error) diff --git a/libtransmission/peer-io.cc b/libtransmission/peer-io.cc index 6186dc75d..d2efd95c3 100644 --- a/libtransmission/peer-io.cc +++ b/libtransmission/peer-io.cc @@ -261,73 +261,48 @@ static void event_write_cb(evutil_socket_t fd, short /*event*/, void* vio) TR_ASSERT(tr_isPeerIo(io)); TR_ASSERT(io->socket.type == TR_PEER_SOCKET_TYPE_TCP); - auto const dir = TR_UP; - auto res = int{ 0 }; - auto what = short{ BEV_EVENT_WRITING }; - io->pendingEvents &= ~EV_WRITE; tr_logAddTraceIo(io, "libevent says this peer is ready to write"); - /* Write as much as possible, since the socket is non-blocking, write() will - * return if it can't write any more data without blocking */ - size_t const howmuch = io->bandwidth().clamp(dir, evbuffer_get_length(io->outbuf.get())); + // Write as much as possible. Since the socket is non-blocking, write() + // will return if it can't write any more data without blocking. + auto constexpr Dir = TR_UP; + size_t const howmuch = io->bandwidth().clamp(Dir, evbuffer_get_length(io->outbuf.get())); - /* if we don't have any bandwidth left, stop writing */ + // if we don't have any bandwidth left, stop writing if (howmuch < 1) { - io->setEnabled(dir, false); + io->setEnabled(Dir, false); return; } EVUTIL_SET_SOCKET_ERROR(0); - res = tr_evbuffer_write(io, fd, howmuch); - int const e = EVUTIL_SOCKET_ERROR(); + auto const n_written = tr_evbuffer_write(io, fd, howmuch); // -1 on err, 0 on EOF + auto const err = EVUTIL_SOCKET_ERROR(); + auto const should_retry = n_written == -1 && (err == 0 || err == EAGAIN || err == EINTR || err == EINPROGRESS); - if (res == -1) + // schedule another write if we have more data to write & think future writes would succeed + if ((evbuffer_get_length(io->outbuf.get()) != 0) && (n_written > 0 || should_retry)) { - if (e == 0 || e == EAGAIN || e == EINTR || e == EINPROGRESS) + io->setEnabled(Dir, true); + } + + if (n_written > 0) + { + didWriteWrapper(io, n_written); + } + else + { + auto const what = n_written == -1 ? BEV_EVENT_WRITING | BEV_EVENT_ERROR : BEV_EVENT_WRITING | BEV_EVENT_EOF; + auto const errmsg = tr_net_strerror(err); + tr_logAddDebugIo( + io, + fmt::format("event_write_cb got an err. n_written:{}, what:{}, errno:{} ({})", n_written, what, err, errmsg)); + if (io->gotError != nullptr) { - goto RESCHEDULE; + io->gotError(io, what, io->userData); } - - /* error case */ - what |= BEV_EVENT_ERROR; - } - else if (res == 0) - { - /* eof case */ - what |= BEV_EVENT_EOF; - } - - if (res <= 0) - { - goto FAIL; - } - - if (evbuffer_get_length(io->outbuf.get()) != 0) - { - io->setEnabled(dir, true); - } - - didWriteWrapper(io, res); - return; - -RESCHEDULE: - if (evbuffer_get_length(io->outbuf.get()) != 0) - { - io->setEnabled(dir, true); - } - - return; - -FAIL: - auto const errmsg = tr_net_strerror(e); - tr_logAddDebugIo(io, fmt::format("event_write_cb got an err. res:{}, what:{}, errno:{} ({})", res, what, e, errmsg)); - - if (io->gotError != nullptr) - { - io->gotError(io, what, io->userData); } } diff --git a/libtransmission/subprocess-posix.cc b/libtransmission/subprocess-posix.cc index 7c4dac2a4..056f0a4b1 100644 --- a/libtransmission/subprocess-posix.cc +++ b/libtransmission/subprocess-posix.cc @@ -53,8 +53,7 @@ static void set_system_error(tr_error** error, int code, std::string_view what) static bool tr_spawn_async_in_child( char const* const* cmd, std::map const& env, - std::string_view work_dir, - int pipe_fd) + std::string_view work_dir) { auto key_sz = std::string{}; auto val_sz = std::string{}; @@ -66,25 +65,21 @@ static bool tr_spawn_async_in_child( if (setenv(key_sz.c_str(), val_sz.c_str(), 1) != 0) { - goto FAIL; + return false; } } if (!std::empty(work_dir) && chdir(tr_pathbuf{ work_dir }) == -1) { - goto FAIL; + return false; } if (execvp(cmd[0], const_cast(cmd)) == -1) { - goto FAIL; + return false; } return true; - -FAIL: - (void)write(pipe_fd, &errno, sizeof(errno)); - return false; } static bool tr_spawn_async_in_parent(int pipe_fd, tr_error** error) @@ -168,8 +163,9 @@ bool tr_spawn_async( { close(pipe_fds[0]); - if (!tr_spawn_async_in_child(cmd, env, work_dir, pipe_fds[1])) + if (!tr_spawn_async_in_child(cmd, env, work_dir)) { + (void)write(pipe_fds[1], &errno, sizeof(errno)); _exit(0); } } diff --git a/libtransmission/tr-udp.cc b/libtransmission/tr-udp.cc index 3bacc0a41..135188a25 100644 --- a/libtransmission/tr-udp.cc +++ b/libtransmission/tr-udp.cc @@ -104,15 +104,39 @@ void tr_session::tr_udp_core::set_socket_buffers() } } +static tr_socket_t rebind_ipv6_impl(in6_addr sin6_addr, tr_port port) +{ + auto const sock = socket(PF_INET6, SOCK_DGRAM, 0); + if (sock == TR_BAD_SOCKET) + { + return TR_BAD_SOCKET; + } + +#ifdef IPV6_V6ONLY + /* Since we always open an IPv4 socket on the same port, + * this shouldn't matter. But I'm superstitious. */ + int one = 1; + (void)setsockopt(sock, IPPROTO_IPV6, IPV6_V6ONLY, reinterpret_cast(&one), sizeof(one)); +#endif + + auto sin6 = sockaddr_in6{}; + sin6.sin6_family = AF_INET6; + sin6.sin6_addr = sin6_addr; + sin6.sin6_port = port.network(); + if (::bind(sock, reinterpret_cast(&sin6), sizeof(sin6)) == -1) + { + tr_netCloseSocket(sock); + return TR_BAD_SOCKET; + } + + return sock; +} + /* BEP-32 has a rather nice explanation of why we need to bind to one IPv6 address, if I may say so myself. */ -// TODO: remove goto, it prevents reducing scope of local variables void tr_session::tr_udp_core::rebind_ipv6(bool force) { - auto sin6 = sockaddr_in6{}; auto const ipv6 = tr_globalIPv6(&session_); - int rc = -1; - int one = 1; /* We currently have no way to enable or disable IPv6 after initialisation. No way to fix that without some surgery to the DHT code itself. */ @@ -127,74 +151,31 @@ void tr_session::tr_udp_core::rebind_ipv6(bool force) return; } - auto const s = socket(PF_INET6, SOCK_DGRAM, 0); - - if (s == TR_BAD_SOCKET) + auto const sock = rebind_ipv6_impl(*ipv6, udp_port_); + if (sock == TR_BAD_SOCKET) { - goto FAIL; + /* Something went wrong. It's difficult to recover, so let's + * simply set things up so that we try again next time. */ + auto const error_code = errno; + auto ipv6_readable = std::array{}; + evutil_inet_ntop(AF_INET6, &*ipv6, std::data(ipv6_readable), std::size(ipv6_readable)); + tr_logAddWarn(fmt::format( + _("Couldn't rebind IPv6 socket {address}: {error} ({error_code})"), + fmt::arg("address", std::data(ipv6_readable)), + fmt::arg("error", tr_strerror(error_code)), + fmt::arg("error_code", error_code))); + + udp6_bound_.reset(); + return; } -#ifdef IPV6_V6ONLY - /* Since we always open an IPv4 socket on the same port, this - shouldn't matter. But I'm superstitious. */ - (void)setsockopt(s, IPPROTO_IPV6, IPV6_V6ONLY, reinterpret_cast(&one), sizeof(one)); -#endif - - memset(&sin6, 0, sizeof(sin6)); - sin6.sin6_family = AF_INET6; - - if (ipv6) + if (udp6_socket_ != TR_BAD_SOCKET) { - sin6.sin6_addr = *ipv6; - } - - sin6.sin6_port = udp_port_.network(); - - rc = bind(s, (struct sockaddr*)&sin6, sizeof(sin6)); - - if (rc == -1) - { - goto FAIL; - } - - if (udp6_socket_ == TR_BAD_SOCKET) - { - udp6_socket_ = s; - } - else - { - /* FIXME: dup2 doesn't work for sockets on Windows */ - rc = dup2(s, udp6_socket_); - - if (rc == -1) - { - goto FAIL; - } - - tr_netCloseSocket(s); + tr_netCloseSocket(udp6_socket_); } + udp6_socket_ = sock; udp6_bound_ = ipv6; - return; - -FAIL: - /* Something went wrong. It's difficult to recover, so let's simply - set things up so that we try again next time. */ - auto const error_code = errno; - auto ipv6_readable = std::array{}; - evutil_inet_ntop(AF_INET6, &*ipv6, std::data(ipv6_readable), std::size(ipv6_readable)); - tr_logAddWarn(fmt::format( - _("Couldn't rebind IPv6 socket {address}: {error} ({error_code})"), - fmt::arg("address", std::data(ipv6_readable)), - fmt::arg("error", tr_strerror(error_code)), - fmt::arg("error_code", error_code))); - - if (s != TR_BAD_SOCKET) - { - tr_netCloseSocket(s); - } - - udp6_bound_.reset(); } static void event_callback(evutil_socket_t s, [[maybe_unused]] short type, void* vsession)