Common/Network+BBA/Builtin: Cleanup

[sepalani]

In this PR, the code in Common/Network and BBA/Builtin is refactored a bit. A potential oversight regarding the IP header checksum is addressed. There should be no change behaviour beside that.

@schthack @nolrinale
Could you please give this PR a try to make sure it doesn't introduce any regression?

This PR is ready to be reviewed & tested.

[iwubcode]

I'm not super familiar with some of these Dolphin helper classes.

This seems odd to me; I'm probably missing something. I get the first equal is effectively creating the BitCastPtrType and the second equal is memcopy'ing 0 to that pointer. But why bother with that when the 3rd equal you set it to a different value?

Basically, why not do this:

auto ip_checksum_bitcast_ptr = Common::BitCastPtr<u16>(ip_ptr + offsetof(IPv4Header, header_checksum));
ip_checksum_bitcast_ptr = htons(Common::ComputeNetworkChecksum(ip_ptr, ip_header_size));

[schthack]

i will awnser for him, the crc calculation include the crc field, it need to be set to 0 for this process

[iwubcode]

Thank you but I don't get it. I'd understand if all the bytes were getting zero'd out and then writing just some bytes after. But here the value is being set to 0 and then set to whatever htons returns. Why are both required?

[AdmiralCurtiss]

ComputeNetworkChecksum() is doing the actual work here and it needs the field set to 0 to return the correct value, but the field when the packet is actually sent needs to be the result of ComputeNetworkChecksum(). Basically what is happening here is:

struct Packet {
 [arbitrary data here]
 u16 checksum
 [more arbitrary data here]
}

Packet p;
p.checksum = 0;
u16 c = CalculateChecksum(p);
p.checksum = c;

[AdmiralCurtiss]

See https://en.wikipedia.org/wiki/Transmission_Control_Protocol#Checksum_computation for reference. I don't know why the checksum is in the middle of the data either, but that's how it works.

[iwubcode]

Ah, I was so focused on the code flow / logic, I didn't stop to think what was going on. Has nothing to do with the memcpy here. We're literally wanting to set the checksum value to 0 to avoid including it in the checksum calculation. I should have seen that. Thanks @AdmiralCurtiss .

---

This isn't relevant to the PR really because I see this is just a cleanup.

But is there a reason to redo the checksum calculation? Yes, we flipped the bits to send over the wire but flipping the checksum should be adequate? Additionally, just personally I find something like this a little clearer than the pointer arithmetic:

std::vector<u8> TCPPacket::Build() const
{
  TCPPacket packet_result = *this;
  packet_result.tcp_header.properties = htons(packet_result.tcp_header.properties);
  packet_result.ip_header.total_len = htons(packet_result.ip_header.total_len);
  packet_result.ip_header.header_checksum = htons(packet_result.ip_header.header_checksum);
  packet_result.tcp_header.checksum = htons(packet_result.tcp_header.checksum);

  std::vector<u8> result;
  result.reserve(Size());  // Useful not to invalidate .data() pointers
  InsertObj(&result, packet_result);
  return result;
}

I guess it incurs the cost of the copy constructor, but I feel like that's worth the gain in readability.

Additionally, also curious why are we only handling some of the fields? Ex, why isn't source_port, identification, etc being handled?

[sepalani]

Build() is the method used to serialise the packet into bytes.

But is there a reason to redo the checksum calculation? Yes, we flipped the bits to send over the wire but flipping the checksum should be adequate?

Since this method is used for serialisation, before calling it we can change the parts we want of the structure, like updating members such as the TCP options, IP options, payload data and many fields of the IP and TCP headers. That's why we have to make sure the checksums and total_len are up-to-date and haven't changed.

Additionally, just personally I find something like this a little clearer than the pointer arithmetic:

std::vector<u8> TCPPacket::Build() const
{
  TCPPacket packet_result = *this;
  packet_result.tcp_header.properties = htons(packet_result.tcp_header.properties);
  packet_result.ip_header.total_len = htons(packet_result.ip_header.total_len);
  packet_result.ip_header.header_checksum = htons(packet_result.ip_header.header_checksum);
  packet_result.tcp_header.checksum = htons(packet_result.tcp_header.checksum);

Using a copy isn't the only issue, we need several objects to be contiguous in memory (IP header struct, IP options vector, TCP header struct, TCP options vector, data vector), these are needed to compute the checksum easily. That's why working on the vector directly is more straight forward. Otherwise, we have to compute several checksums separately, sum them and set them in the packet_result.

Additionally, also curious why are we only handling some of the fields? Ex, why isn't source_port, identification, etc being handled?

These fields are set in the packet constructor or when necessary by the code in the BBA/BuiltIn file.

[nolrinale]

I've tested this one as well and I dont see any problem or regression.

However I suggest you procure a copy of PSO Episode 1 & 2 Plus and try to connect to the schthack server.

Previously this wasn't necessary as schthack is the owner of the server and was able to set up test servers on his own + everyone in the community testing his builds constantly, now in this situation you will need to access the live servers like the rest of us and test while actually playing the game. That is, if you plan to heavily rewrite the whole thing and prefer a more through analysis of the PR performance beyond the feedback that we can provide you here.

[schthack]

tested on windows, all good. The part you toutched are for the tcp/udp structure themself, if they were broken no online play would be possible.

[AdmiralCurtiss]

Looks alright to me too.