Kernel/Net: Cleanup the ICMP handler

[sdomi]

this may be a bit opinionated, but I think it improves readability by quite a lot + the cast is safer :)

---

This change fixes up handle_icmp to use a bit_cast instead of a reinterpret_cast, which got used due to legacy reasons.

A few dbgln's got turned into dbgln_if's, in accordance with the behavior throughout the rest of the file.

Furthermore, we revert a part of commit ad73ade which introduced inconsistency between struct names. There's no ICMPv4, only ICMP and ICMPv6. Thus, it makes more sense to revert the ICMPv4Type back to ICMPType.

We also remove the FIXME about the TTL - as per RFC1700, TTL of 64 is a recommended default, so the current behavior can stay.

[nico]

This bit_casts a pointer type if I read this correctly. How is that useful? I thought bit_cast() is for reinterpreting the bits of a literal (think int to float, or things like that).

Looking around a bit, eg https://stackoverflow.com/questions/67765898/aliasing-accesses-through-a-stdbit-casted-pointer seems to back this up.

[nico]

Commit desc: "cleanup" is a noun, "clean up" the verb

Otherwise seems fine.

For dbgln()s, I personally use the _if variant of it's only interesting to people working in that area and the unconditional form of it might be interesting to others (say, for "not yet implemented" and things like that). But I don't have a strong opinion on that.

It's really only the bit_cast change that feels Feng shui. The rest is fine.

[sdomi]

This bit_casts a pointer type if I read this correctly.

my decision to change that reinterpret_cast into bit_cast mostly stemmed from me reading about C++ cast types the other day and coming into the conclusion that "reinterpret_cast bad". but yes, you're not wrong that in this place, the outcome will likely always be the same.

Should I revert this change back?

[kleinesfilmroellchen]

This bit_casts a pointer type if I read this correctly. How is that useful? I thought bit_cast() is for reinterpreting the bits of a literal (think int to float, or things like that).

bit_cast is not just a reinterpreting of bits, that’s what reinterpret_cast is. bit_cast has “as-if” equivalence to memcpy (which makes its behavior, especially for padding, very much defined) and is usable at compile time. In this case this of course works out to identical behavior to reinterpret_cast.

The main point in this case is to guarantee that we’re actually handling pointers since bit_cast checks sizes.

As a rule (not yet fixed in documentation, but this is from what I have talked about with others, especially in the kernel space), we don’t want to use reinterpret_casts, since all of their use cases that don’t immediately invoke UB are covered by other casts, especially bit_cast. (I have been very vocal about this in reviews, and have not received significant pushback.) There’s a lot of legacy reinterpret_casts in the kernel, and as we’re moving through modernizing parts of it (like here), we can change them to the safer alternatives.