PDU::serialize() is not const

[kyrofa]

This means I need to pass around non-const references to packets if I want to serialize them, which isn't ideal as it makes my API unclear (and somewhat unsafe). Is there a reason for this? If serializing actually mutates PDU data, perhaps that data should be mutable. Thoughts?

[mfontanini]

Several PDUs have fields which will have their value computed right before serialization. e.g. this happens on IP, TCP, UDP, IPv6, etc on their checksum field. Also on some other protocols that have padding, like RadioTap and Ethernet. Therefore, if the mutable data was made mutable, then most PDUs would have their entire header (header as in PDU header) mutable, which is not ideal. Plus, in this case serializing is actually changing the value of those fields, so using the keyword mutable is not really correct, as serializing the PDU is not a read-only operation over the packet.

You can always keep passing const and then const_cast them if you're sure they're not const. I personally pass around packets as non const as it's sometimes convenient to slice some PDU/attribute off in case you want to store something without making copies (e.g. moving the payload off a RawPDU), but that of course depends on what you're implementing.

[kyrofa]

You can always keep passing const and then const_cast them if you're sure they're not const

Except, as you point out, this really isn't a const operation, so I'd be violating my API for the same reason mutable isn't correct.

Is this essentially a lazy evaluation, then? e.g. don't calculate the checksum until you need it, but then store it once you've calculated it?

[mfontanini]

Is this essentially a lazy evaluation, then?

Yes, if it wasn't done this way then this should probably have to implement some observer pattern between PDUs in the same packet so that every change recalculates the checksum/other fields, but that would be a waste of CPU.

[ebertland]

In some cases, I want to serialize a packet without modifying it. For example, if I receive a bad packet, I want to send it to a different destination unmodified rather than dropping it or fixing it. The current implementation of serialize() fixes the IP header length and checksum. A const serialize() would just write out what was originally there.

Is there a(nother) way to get to the original bytes that were used to build the packet?

[aortez]

I recently ran into this problem and I think I have solved it by making a mutable copy of the const Packet and serializing that, leaving the const Packet unmutated.

Here's an example of what I mean, taken from my own codebase, with just the relevant bits shown:

void serialize(const Tins::Packet& packet, ...)
{
...
    Tins::Packet packetCopy(packet);
    const std::vector<uint8_t> buffer = packetCopy.pdu()->rfind_pdu<RawPDU>().serialize();
    serialize(buffer, ...);  /// <-- This is a serialize function internal to my codebase.
...
}

This is not the most efficient method (it makes a copy), but I think it is safer/simpler than trying to ensure conditions are right for using a const_cast.

@mfontanini, thank you for your excellent library and all of your time helping your users! :bow:
When you have some time, do you mind considering if this solution looks reasonable to you?

[mfontanini]

@aortez I think it really depends on what's your use case. What you're proposing is fine but ultimately the side effect of the serialization (e.g. computing checksums) is still happening, just on a copy of the original packet. Depending on what you're using with these packets, you might as well just serialize the original one and have the side effects change it rather than some volatile copy of it.

I think it's better not to try to hide the fact that serialization is a mutable action but rather live with it. This is something that could have been done in a different way, e.g. having some builder pattern such that once you build a packet it's immutable, but it's too late for that. Other libraries take the approach of having you explicitly computes the checksums before sending your packet, at the risk of having a bogus one if you don't, which to me feels worse that having both compacted into a mutable serialize that can't be misused.