QuicFrame length and payload_length

[kixelated]

I recently implemented draft 2 and was very confused about the these two fields, found in some of the QuicFrame classes.

1. What does it mean when a frame has both payload_length and length (total frame size)? ex. PaddingFrame, PingFrame, AckFrame, ResetStreamFrame, StopSendingFrame

Does length include the frame type while payload_length is the rest? That doesn't seem true for PingFrame and PaddingFrame, as payload_length == length. Also the frame type must use the minimum size, so they're just going to be off by 1. Or does payload_length measure something else?

====

2. What about frames with both payload_length and length (not total frame size)? ex. CryptoFrame, StreamFrame

Do I need to make an exception for these frames? Is payload_length the aforementioned total length - 1, or is it the length of the STREAM/CRYPTO data?

The RFC says: > payload_length: length of the packet/frame payload, excluding AEAD tag. For many control frames, this will have a value of zero

That implies that payload_length == length for these frames, and payload_length == 0 for every other type of frame???

====

3. What about frames with no length or payload_length? ex. NewTokenFrame, MaxDataFrame, MaxStreamDataFrame, MaxStreamsFrame, ...

Is there no way to measure the size or these frames? The length of most of them don't matter, although you can make the same argument for ResetStreamFrame and StopSendingFrame. It seems like somebody got distracted while going down the list.

====

4. What about classes with raw_length? ex. UnknownFrame

Seems inconsistent!

[rmarx]

Hello Luke,

Thanks for opening this issue, because you're correct that there are some problems with this, as the design for how to do this has changed over time and there seem to be some leftovers/oversights. The person who "got distracted going down the list" was me ;)

The (currently) intended approach is to log actual lengths and payload lengths as part of an optional "raw" field, as described in https://tools.ietf.org/html/draft-marx-qlog-event-definitions-quic-h3-02#section-4.1. This is because many implementations won't log this info and it's only really useful for doing packetization analysis, which you don't always need, so it makes sense to split this up a bit.

As such, the payload_length and length fields on PingFrame, AckFrame, ResetStreamFrame, and StopSendingFrame and the payload_length field on PaddingFrame and CryptoFrame should be removed and instead raw.payload_length and raw.length should be used.

The length fields on Crypto and Stream remain and reflect the fields as defined in the QUIC docs, to make it easier to map them to qlog (this means that frame.length will be the same as raw.payload_length, but so be it...)

I'm not sure what to do with PaddingFrame. It doesn't have a length in the spec, but that would be useful to have in qlog. You could say implementations should then just log raw.payload_length, but not everyone has that type of flexible stack where that's easy to do if you don't log the raw field everywhere, so I'm in favor of keeping length on PaddingFrame as well.

Finally, for UnknownFrame, raw_length and raw should also be changed to just use the generic raw field.

Keeping this open to tackle for draft-03. Feel free to open a PR, otherwise I'll make the fixes over time.

---

As I said in the other issue you opened in qvis, it's great to have this type of feedback. It can be tough keeping every detail consistent in this type of document and I find I tend to overlook things like this. If you find anything else that seems strange, don't hesitate to open an issue.

[ptrd]

My 2 cents:

• IMHO Padding should contain a length field (in kwik i do the same optimisation of combining padding bytes into one frame; i guess more people are doing so ;-))
• for frames, having both length and payload_length wouldn't be very useful, would it? One can easily computed from the other (where "easily" is even an understatement ;-))

[LPardue]

Has this been addressed? If not, what needs doing?

[rmarx]

Coupling this to #243 as it seems that should be a (partial) fix if we properly use RawInfo, but should still double-check.

[rmarx]

AFAICT this was indeed fixed when moving to 'RawInfo everywhere" a while back. It still has some issues that @kixelated originally detailed (sometimes payload_length will be 0 for example), but I feel the more generalized approach is more important here (implementations can still choose to payload_length for those instances, as they're optional fields).

Closing for now; feel free to leave additional comments or open a new issue if this is still problematic.