Consider using different types for packet buffers at different stages of their lifetime.

[bzbarsky-apple]

Problem

I have now seen multiple mistakes made because people were confused about whether their packet buffer includes a PacketHeader/PayloadHeader or not. https://github.com/project-chip/connectedhomeip/issues/5903 is the latest iteration, but it's bitten us a few times.

Proposed Solution

Maybe we should consider have separate handle classes for:

1. Packet buffer pointing to message header (i.e. contains a packet header and maybe-encrypted payload)
2. Packet buffer pointing to message payload (aka exchange/payload header): without a packet header but with a maybe-encrypted payload.
3. Packet buffer pointing to guaranteed-decrypted payload: like 2, but after decryption.
4. Packet buffer pointing to application payload (i.e. after stripping off packet header, maybe-decrypting, stripping off payload header).

The header encode/decode methods could consume these different types and spit out the new types, though I'm a little worried that this would entail some extra codesize; I would have to test how it behaves in practice.

You could extract a raw PacketBufferHandle from any of the above types, so we should be able to start introducing these slowly in places that want to enforce what sort of packet buffer they are dealing with.

@kpschoedel @andy31415 @pan-apple Thoughts?

[kpschoedel]

I like this.

I think using empty subclasses should work and not add to code size, but I may be missing corners.

[bzbarsky-apple]

I think we can add the types without adding any codesize. It's the changes to the header encode/decode bits where it will need to write to a new handle (of the new type) instead of just mutating the handed-in one that may have some codesize impact.

I'll try to prototype this out next week, unless someone else gets to it first.

[bzbarsky-apple]

OK, so I tried this for PacketHeader::DecodeAndConsume. It adds about 30 bytes of codesize, which I think I can live with. I'll try doing this more widely and see how it goes!

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

[stale[bot]]

This stale issue has been automatically closed. Thank you for your contributions.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.