respond to further review feedback

Source/Core/Core/HW/EXI/BBA/TAPServerBBA.cpp

@@ -3,21 +3,9 @@
 
 #include "Core/HW/EXI/EXI_DeviceEthernet.h"
 
-#ifdef _WIN32
+#include <cstring>
-#include <winsock2.h>
-#include <ws2ipdef.h>
-#else
-#include <fcntl.h>
-#include <sys/socket.h>
-#include <sys/types.h>
-#include <sys/un.h>
-#include <unistd.h>
-#endif
 
-#include "Common/CommonFuncs.h"
 #include "Common/Logging/Log.h"
-#include "Common/StringUtil.h"
-#include "Core/HW/EXI/EXI_Device.h"
 
 namespace ExpansionInterface
 {
@@ -64,7 +52,7 @@ void CEXIETHERNET::TAPServerNetworkInterface::RecvStop()
 
 bool CEXIETHERNET::TAPServerNetworkInterface::SendFrame(const u8* frame, u32 size)
 {
-  bool ret = m_tapserver_if.SendFrame(frame, size);
+  const bool ret = m_tapserver_if.SendFrame(frame, size);
   if (ret)
     m_eth_ref->SendComplete();
   return ret;
@@ -76,13 +64,12 @@ void CEXIETHERNET::TAPServerNetworkInterface::HandleReceivedFrame(std::string&&
   {
     ERROR_LOG_FMT(SP1, "Received BBA frame of size {}, which is larger than maximum size {}",
                   data.size(), BBA_RECV_SIZE);
+    return;
   }
-  else
+
-  {
+  std::memcpy(m_eth_ref->mRecvBuffer.get(), data.data(), data.size());
-    memcpy(m_eth_ref->mRecvBuffer.get(), data.data(), data.size());
+  m_eth_ref->mRecvBufferLength = static_cast<u32>(data.size());
-    m_eth_ref->mRecvBufferLength = static_cast<u32>(data.size());
+  m_eth_ref->RecvHandlePacket();
-    m_eth_ref->RecvHandlePacket();
-  }
 }
 
 }  // namespace ExpansionInterface

Source/Core/Core/HW/EXI/BBA/TAPServerConnection.cpp

@@ -14,6 +14,9 @@
 #include <unistd.h>
 #endif
 
+#include <cstdlib>
+#include <cstring>
+
 #include "Common/CommonFuncs.h"
 #include "Common/Logging/Log.h"
 #include "Common/StringUtil.h"
@@ -52,7 +55,7 @@ static int ConnectToDestination(const std::string& destination)
 
   int ss_size;
   sockaddr_storage ss;
-  memset(&ss, 0, sizeof(ss));
+  std::memset(&ss, 0, sizeof(ss));
   if (destination[0] != '/')
   {
     // IP address or hostname
@@ -64,10 +67,22 @@ static int ConnectToDestination(const std::string& destination)
     }
 
     sockaddr_in* sin = reinterpret_cast<sockaddr_in*>(&ss);
-    sin->sin_addr.s_addr = htonl(sf::IpAddress(destination.substr(0, colon_offset)).toInteger());
+    const sf::IpAddress dest_ip(destination.substr(0, colon_offset));
+    if (dest_ip == sf::IpAddress::None || dest_ip == sf::IpAddress::Any)
+    {
+      ERROR_LOG_FMT(SP1, "Destination IP address is not valid\n");
+      return -1;
+    }
+    sin->sin_addr.s_addr = htonl(dest_ip.toInteger());
     sin->sin_family = AF_INET;
-    std::string port_str = destination.substr(colon_offset + 1);
+    const std::string port_str = destination.substr(colon_offset + 1);
-    sin->sin_port = htons(atoi(port_str.c_str()));
+    const int dest_port = std::atoi(port_str.c_str());
+    if (dest_port < 1 || dest_port > 65535)
+    {
+      ERROR_LOG_FMT(SP1, "Destination port is not valid\n");
+      return -1;
+    }
+    sin->sin_port = htons(dest_port);
     ss_size = sizeof(*sin);
 #ifndef _WIN32
   }
@@ -81,7 +96,7 @@ static int ConnectToDestination(const std::string& destination)
       return -1;
     }
     sun->sun_family = AF_UNIX;
-    strcpy(sun->sun_path, destination.c_str());
+    std::strcpy(sun->sun_path, destination.c_str());
     ss_size = sizeof(*sun);
 #else
   }
@@ -107,8 +122,8 @@ static int ConnectToDestination(const std::string& destination)
 
   if (connect(fd, reinterpret_cast<sockaddr*>(&ss), ss_size) == -1)
   {
-    std::string s = Common::StrNetworkError();
+    INFO_LOG_FMT(SP1, "Couldn't connect socket ({}), unable to create tapserver connection\n",
-    INFO_LOG_FMT(SP1, "Couldn't connect socket ({}), unable to create tapserver connection\n", s);
+                 Common::StrNetworkError());
     closesocket(fd);
     return -1;
   }
@@ -162,31 +177,31 @@ void TAPServerConnection::RecvStop()
   m_read_enabled.Clear();
 }
 
-bool TAPServerConnection::SendAndRemoveAllHDLCFrames(std::string& send_buf)
+bool TAPServerConnection::SendAndRemoveAllHDLCFrames(std::string* send_buf)
 {
-  while (!send_buf.empty())
+  while (!send_buf->empty())
   {
-    std::size_t start_offset = send_buf.find(0x7E);
+    const std::size_t start_offset = send_buf->find(0x7E);
     if (start_offset == std::string::npos)
     {
       break;
     }
-    std::size_t end_sentinel_offset = send_buf.find(0x7E, start_offset + 1);
+    const std::size_t end_sentinel_offset = send_buf->find(0x7E, start_offset + 1);
     if (end_sentinel_offset == std::string::npos)
     {
       break;
     }
-    std::size_t end_offset = end_sentinel_offset + 1;
+    const std::size_t end_offset = end_sentinel_offset + 1;
-    std::size_t size = end_offset - start_offset;
+    const std::size_t size = end_offset - start_offset;
 
-    u8 size_bytes[2] = {static_cast<u8>(size), static_cast<u8>(size >> 8)};
+    const u8 size_bytes[2] = {static_cast<u8>(size), static_cast<u8>(size >> 8)};
     if (send(m_fd, reinterpret_cast<const char*>(size_bytes), 2, SEND_FLAGS) != 2)
     {
       ERROR_LOG_FMT(SP1, "SendAndRemoveAllHDLCFrames(): could not write size field");
       return false;
     }
     const int written_bytes =
-        send(m_fd, send_buf.data() + start_offset, static_cast<int>(size), SEND_FLAGS);
+        send(m_fd, send_buf->data() + start_offset, static_cast<int>(size), SEND_FLAGS);
     if (u32(written_bytes) != size)
     {
       ERROR_LOG_FMT(SP1,
@@ -194,31 +209,25 @@ bool TAPServerConnection::SendAndRemoveAllHDLCFrames(std::string& send_buf)
                     size, written_bytes);
       return false;
     }
-    else
+    *send_buf = send_buf->substr(end_offset);
-    {
-      send_buf = send_buf.substr(end_offset);
-    }
   }
   return true;
 }
 
 bool TAPServerConnection::SendFrame(const u8* frame, u32 size)
 {
-  {
+  INFO_LOG_FMT(SP1, "SendFrame {}\n{}", size, ArrayToString(frame, size, 0x10));
-    const std::string s = ArrayToString(frame, size, 0x10);
-    INFO_LOG_FMT(SP1, "SendFrame {}\n{}", size, s);
-  }
 
   // On Windows, the data pointer is of type const char*; on other systems it is
   // of type const void*. This is the reason for the reinterpret_cast here and
   // in the other send/recv calls in this file.
-  u8 size_bytes[2] = {static_cast<u8>(size), static_cast<u8>(size >> 8)};
+  const u8 size_bytes[2] = {static_cast<u8>(size), static_cast<u8>(size >> 8)};
   if (send(m_fd, reinterpret_cast<const char*>(size_bytes), 2, SEND_FLAGS) != 2)
   {
     ERROR_LOG_FMT(SP1, "SendFrame(): could not write size field");
     return false;
   }
-  int written_bytes =
+  const int written_bytes =
       send(m_fd, reinterpret_cast<const char*>(frame), static_cast<ws_ssize_t>(size), SEND_FLAGS);
   if (u32(written_bytes) != size)
   {
@@ -263,7 +272,7 @@ void TAPServerConnection::ReadThreadHandler()
     case ReadState::SIZE:
     {
       u8 size_bytes[2];
-      ws_ssize_t bytes_read = recv(m_fd, reinterpret_cast<char*>(size_bytes), 2, 0);
+      const ws_ssize_t bytes_read = recv(m_fd, reinterpret_cast<char*>(size_bytes), 2, 0);
       if (bytes_read == 1)
       {
         read_state = ReadState::SIZE_HIGH;
@@ -295,25 +304,23 @@ void TAPServerConnection::ReadThreadHandler()
       // This handles the annoying case where only one byte of the size field
       // was available earlier.
       u8 size_high = 0;
-      ws_ssize_t bytes_read = recv(m_fd, reinterpret_cast<char*>(&size_high), 1, 0);
+      const ws_ssize_t bytes_read = recv(m_fd, reinterpret_cast<char*>(&size_high), 1, 0);
-      if (bytes_read == 1)
+      if (bytes_read != 1)
-      {
-        frame_bytes_expected |= (size_high << 8);
-        frame_data.resize(frame_bytes_expected, '\0');
-        if (frame_bytes_expected > m_max_frame_size)
-        {
-          ERROR_LOG_FMT(SP1, "Packet is too large ({} bytes); dropping it", frame_bytes_expected);
-          read_state = ReadState::SKIP;
-        }
-        else
-        {
-          read_state = ReadState::DATA;
-        }
-      }
-      else
       {
         ERROR_LOG_FMT(SP1, "Failed to read split size field from destination: {}",
                       Common::StrNetworkError());
+        break;
+      }
+      frame_bytes_expected |= (size_high << 8);
+      frame_data.resize(frame_bytes_expected, '\0');
+      if (frame_bytes_expected > m_max_frame_size)
+      {
+        ERROR_LOG_FMT(SP1, "Packet is too large ({} bytes); dropping it", frame_bytes_expected);
+        read_state = ReadState::SKIP;
+      }
+      else
+      {
+        read_state = ReadState::DATA;
       }
       break;
     }
@@ -321,26 +328,24 @@ void TAPServerConnection::ReadThreadHandler()
     case ReadState::SKIP:
     {
       const std::size_t bytes_to_read = frame_data.size() - frame_bytes_received;
-      ws_ssize_t bytes_read = recv(m_fd, frame_data.data() + frame_bytes_received,
+      const ws_ssize_t bytes_read = recv(m_fd, frame_data.data() + frame_bytes_received,
-                                   static_cast<ws_ssize_t>(bytes_to_read), 0);
+                                         static_cast<ws_ssize_t>(bytes_to_read), 0);
       if (bytes_read <= 0)
       {
         ERROR_LOG_FMT(SP1, "Failed to read data from destination: {}", Common::StrNetworkError());
+        break;
       }
-      else
+      frame_bytes_received += bytes_read;
+      if (frame_bytes_received == frame_bytes_expected)
       {
-        frame_bytes_received += bytes_read;
+        if (read_state == ReadState::DATA)
-        if (frame_bytes_received == frame_bytes_expected)
         {
-          if (read_state == ReadState::DATA)
+          m_recv_cb(std::move(frame_data));
-          {
-            m_recv_cb(std::move(frame_data));
-          }
-          frame_data.clear();
-          frame_bytes_received = 0;
-          frame_bytes_expected = 0;
-          read_state = ReadState::SIZE;
         }
+        frame_data.clear();
+        frame_bytes_received = 0;
+        frame_bytes_expected = 0;
+        read_state = ReadState::SIZE;
       }
       break;
     }

Source/Core/Core/HW/EXI/BBA/TAPServerConnection.h

@@ -3,24 +3,12 @@
 
 #pragma once
 
-#ifdef _WIN32
-#include <winsock2.h>
-#include <ws2ipdef.h>
-#else
-#include <fcntl.h>
-#include <sys/socket.h>
-#include <sys/types.h>
-#include <sys/un.h>
-#include <unistd.h>
-#endif
-
 #include <functional>
+#include <string>
 #include <thread>
 
-#include "Common/CommonFuncs.h"
+#include "Common/Flag.h"
-#include "Common/Logging/Log.h"
 #include "Common/SocketContext.h"
-#include "Common/StringUtil.h"
 
 namespace ExpansionInterface
 {
@@ -28,7 +16,9 @@ namespace ExpansionInterface
 class TAPServerConnection
 {
 public:
-  TAPServerConnection(const std::string& destination, std::function<void(std::string&&)> recv_cb,
+  using RecvCallback = std::function<void(std::string&&)>;
+
+  TAPServerConnection(const std::string& destination, RecvCallback recv_cb,
                       std::size_t max_frame_size);
 
   bool Activate();
@@ -37,7 +27,7 @@ public:
   bool RecvInit();
   void RecvStart();
   void RecvStop();
-  bool SendAndRemoveAllHDLCFrames(std::string& send_buf);
+  bool SendAndRemoveAllHDLCFrames(std::string* send_buf);
   bool SendFrame(const u8* frame, u32 size);
 
 private:
@@ -49,9 +39,9 @@ private:
     Skip,
   };
 
-  std::string m_destination;
+  const std::string m_destination;
-  std::function<void(std::string&&)> m_recv_cb;
+  const std::function<void(std::string&&)> m_recv_cb;
-  std::size_t m_max_frame_size;
+  const std::size_t m_max_frame_size;
   Common::SocketContext m_socket_context;
 
   int m_fd = -1;

Source/Core/Core/HW/EXI/EXI_DeviceModem.cpp

@@ -6,6 +6,7 @@
 #include <inttypes.h>
 
 #include <algorithm>
+#include <cstring>
 #include <memory>
 #include <optional>
 #include <string>
@@ -87,12 +88,8 @@ void CEXIModem::ImmWrite(u32 data, u32 size)
   {  // Write device register
     u8 reg_num = static_cast<uint8_t>((m_transfer_descriptor >> 24) & 0x1F);
     bool should_update_interrupts = false;
-    for (; size; size--)
+    for (; size && reg_num < m_regs.size(); size--)
     {
-      if (reg_num >= m_regs.size())
-      {
-        break;
-      }
       should_update_interrupts |=
           ((reg_num == Register::INTERRUPT_MASK) || (reg_num == Register::PENDING_INTERRUPT_MASK));
       m_regs[reg_num++] = (data >> 24);
@@ -153,8 +150,9 @@ u32 CEXIModem::ImmRead(u32 size)
     return ntohl(be_data);
   }
   else
-  {  // Read device register
+  {
-    u8 reg_num = static_cast<uint8_t>((m_transfer_descriptor >> 24) & 0x1F);
+    // Read device register
+    const u8 reg_num = static_cast<uint8_t>((m_transfer_descriptor >> 24) & 0x1F);
     if (reg_num == 0)
     {
       return 0x02020000;  // Device ID (modem)
@@ -214,7 +212,7 @@ void CEXIModem::HandleReadModemTransfer(void* data, u32 size)
   {
     // AT command buffer
     const std::size_t bytes_to_copy = std::min<std::size_t>(size, m_at_reply_data.size());
-    memcpy(data, m_at_reply_data.data(), bytes_to_copy);
+    std::memcpy(data, m_at_reply_data.data(), bytes_to_copy);
     m_at_reply_data = m_at_reply_data.substr(bytes_to_copy);
     m_regs[Register::AT_REPLY_SIZE] = static_cast<u8>(m_at_reply_data.size());
     SetInterruptFlag(Interrupt::AT_REPLY_DATA_AVAILABLE, !m_at_reply_data.empty(), true);
@@ -224,7 +222,7 @@ void CEXIModem::HandleReadModemTransfer(void* data, u32 size)
     // Packet receive buffer
     std::lock_guard<std::mutex> g(m_receive_buffer_lock);
     const std::size_t bytes_to_copy = std::min<std::size_t>(size, m_receive_buffer.size());
-    memcpy(data, m_receive_buffer.data(), bytes_to_copy);
+    std::memcpy(data, m_receive_buffer.data(), bytes_to_copy);
     m_receive_buffer = m_receive_buffer.substr(bytes_to_copy);
     OnReceiveBufferSizeChangedLocked(true);
   }
@@ -264,7 +262,7 @@ void CEXIModem::HandleWriteModemTransfer(const void* data, u32 size)
     // from the emulated program's perspective, so we always tell it the send
     // FIFO is empty.
     SetInterruptFlag(Interrupt::SEND_BUFFER_BELOW_THRESHOLD, true, true);
-    m_network_interface->SendAndRemoveAllHDLCFrames(m_send_buffer);
+    m_network_interface->SendAndRemoveAllHDLCFrames(&m_send_buffer);
   }
   else
   {

Source/Core/Core/HW/EXI/EXI_DeviceModem.h

@@ -17,7 +17,7 @@ class PointerWrap;
 namespace ExpansionInterface
 {
 
-#define MODEM_RECV_SIZE 0x800
+static constexpr std::size_t MODEM_RECV_SIZE = 0x800;
 
 enum
 {
@@ -44,13 +44,20 @@ public:
   void DoState(PointerWrap& p) override;
 
 private:
+  // Note: The names in these enums are based on reverse-engineering of PSO and
+  // not on any documentation of the GC modem or its chipset. If official
+  // documentation is found, any names in there probably will not match these
+  // names.
+
   enum Interrupt
-  {  // Used for Register::INTERRUPT_MASK and Register::PENDING_INTERRUPT_MASK
+  {
+    // Used for Register::INTERRUPT_MASK and Register::PENDING_INTERRUPT_MASK
     AT_REPLY_DATA_AVAILABLE = 0x02,
     SEND_BUFFER_BELOW_THRESHOLD = 0x10,
     RECEIVE_BUFFER_ABOVE_THRESHOLD = 0x20,
     RECEIVE_BUFFER_NOT_EMPTY = 0x40,
   };
+
   enum Register
   {
     DEVICE_TYPE = 0x00,
@@ -90,18 +97,22 @@ private:
   {
     return transfer_descriptor == 0x80000000;
   }
+
   static inline bool IsWriteTransfer(u32 transfer_descriptor)
   {
     return transfer_descriptor & 0x40000000;
   }
+
   static inline bool IsModemTransfer(u32 transfer_descriptor)
   {
     return transfer_descriptor & 0x20000000;
   }
+
   static inline u16 GetModemTransferSize(u32 transfer_descriptor)
   {
     return (transfer_descriptor >> 8) & 0xFFFF;
   }
+
   static inline u32 SetModemTransferSize(u32 transfer_descriptor, u16 new_size)
   {
     return (transfer_descriptor & 0xFF000000) | (new_size << 8);
@@ -117,7 +128,7 @@ private:
     virtual bool Activate() { return false; }
     virtual void Deactivate() {}
     virtual bool IsActivated() { return false; }
-    virtual bool SendAndRemoveAllHDLCFrames(std::string&) { return false; }
+    virtual bool SendAndRemoveAllHDLCFrames(std::string*) { return false; }
     virtual bool RecvInit() { return false; }
     virtual void RecvStart() {}
     virtual void RecvStop() {}
@@ -134,7 +145,7 @@ private:
     virtual bool Activate() override;
     virtual void Deactivate() override;
     virtual bool IsActivated() override;
-    virtual bool SendAndRemoveAllHDLCFrames(std::string& send_buffer) override;
+    virtual bool SendAndRemoveAllHDLCFrames(std::string* send_buffer) override;
     virtual bool RecvInit() override;
     virtual void RecvStart() override;
     virtual void RecvStop() override;

Source/Core/Core/HW/EXI/Modem/TAPServerModem.cpp

@@ -3,21 +3,7 @@
 
 #include "Core/HW/EXI/EXI_DeviceModem.h"
 
-#ifdef _WIN32
-#include <winsock2.h>
-#include <ws2ipdef.h>
-#else
-#include <fcntl.h>
-#include <sys/socket.h>
-#include <sys/types.h>
-#include <sys/un.h>
-#include <unistd.h>
-#endif
-
-#include "Common/CommonFuncs.h"
 #include "Common/Logging/Log.h"
-#include "Common/StringUtil.h"
-#include "Core/HW/EXI/EXI_Device.h"
 
 namespace ExpansionInterface
 {
@@ -47,11 +33,11 @@ bool CEXIModem::TAPServerNetworkInterface::IsActivated()
   return m_tapserver_if.IsActivated();
 }
 
-bool CEXIModem::TAPServerNetworkInterface::SendAndRemoveAllHDLCFrames(std::string& send_buffer)
+bool CEXIModem::TAPServerNetworkInterface::SendAndRemoveAllHDLCFrames(std::string* send_buffer)
 {
-  std::size_t orig_size = send_buffer.size();
+  const std::size_t orig_size = send_buffer->size();
-  bool send_succeeded = m_tapserver_if.SendAndRemoveAllHDLCFrames(send_buffer);
+  const bool send_succeeded = m_tapserver_if.SendAndRemoveAllHDLCFrames(send_buffer);
-  if (send_succeeded && (send_buffer.size() < orig_size))
+  if (send_succeeded && (send_buffer->size() < orig_size))
     m_modem_ref->SendComplete();
   return send_succeeded;
 }

Source/Core/DolphinQt/Settings/BroadbandAdapterSettingsDialog.cpp

@@ -51,7 +51,7 @@ void BroadbandAdapterSettingsDialog::InitControls()
   case Type::TapServer:
   case Type::ModemTapServer:
   {
-    bool is_modem = (m_bba_type == Type::ModemTapServer);
+    const bool is_modem = (m_bba_type == Type::ModemTapServer);
     current_address =
         QString::fromStdString(Config::Get(is_modem ? Config::MAIN_MODEM_TAPSERVER_DESTINATION :
                                                       Config::MAIN_BBA_TAPSERVER_DESTINATION));
@@ -62,21 +62,13 @@ void BroadbandAdapterSettingsDialog::InitControls()
         tr("Enter the IP address and port of the tapserver instance you want to connect to."));
 #else
     address_label = new QLabel(tr("Destination (UNIX socket path or address:port):"));
-    address_placeholder = QStringLiteral("/tmp/dolphin-tap");
+    address_placeholder =
-    if (is_modem)
+        is_modem ? QStringLiteral(u"/tmp/dolphin-modem-tap") : QStringLiteral(u"/tmp/dolphin-tap");
-    {
+    description =
-      description = new QLabel(
+        new QLabel(tr("The default value \"%1\" will work with a local tapserver and newserv."
-          tr("The default value \"/tmp/dolphin-modem-tap\" will work with a local tapserver and "
+                      " You can also enter a network location (address:port) to connect to a "
-             "newserv. You "
+                      "remote tapserver.")
-             "can also enter a network location (address:port) to connect to a remote tapserver."));
+                       .arg(address_placeholder));
-    }
-    else
-    {
-      description = new QLabel(
-          tr("The default value \"/tmp/dolphin-tap\" will work with a local tapserver and newserv. "
-             "You "
-             "can also enter a network location (address:port) to connect to a remote tapserver."));
-    }
 #endif
     window_title = tr("BBA destination address");
     break;