Skip to content

Add FrameBuilder for building frames with target packet length - #39

Draft
sippejw wants to merge 1 commit into
masterfrom
target_length
Draft

Add FrameBuilder for building frames with target packet length#39
sippejw wants to merge 1 commit into
masterfrom
target_length

Conversation

@sippejw

@sippejw sippejw commented May 7, 2024

Copy link
Copy Markdown
Member

Adds FrameBuilder that can adjust PADDING frame sizes to achieve a target length. Completes issue #38

@sippejw sippejw added the enhancement New feature or request label May 7, 2024
@sippejw
sippejw requested a review from gaukas May 7, 2024 17:15
@sippejw
sippejw marked this pull request as draft May 7, 2024 17:29
@sippejw

sippejw commented May 7, 2024

Copy link
Copy Markdown
Member Author

Marked as draft because this doesn't do exactly what I want yet. We want to be able to specify the length of the entire packet (payload and header) and then add padding accordingly. The FrameBuilder does not have access to the header length (I don't think the header has even been built at this point) so I don't yet see a clean way to do this.

@gaukas

gaukas commented May 7, 2024

Copy link
Copy Markdown
Contributor

I see what you meant there. Do we have any captured samples showing the variable length of Initial Packet header impacting the length of the frame section?

Actually we DO have the complete header ready at the point we build frame sections (see (*uPacketPacker).appendInitialPacket and (*uPacketPacker).MarshalInitialPacketPayload)

func (p *uPacketPacker) appendInitialPacket(buffer *packetBuffer, header *wire.ExtendedHeader, pl payload, encLevel protocol.EncryptionLevel, sealer sealer, v protocol.Version) (*longHeaderPacket, error) {

func (p *uPacketPacker) MarshalInitialPacketPayload(pl payload, v protocol.Version) ([]byte, error) {


I think originally quic-go does calculate the padding length at run-time. See

func (p *packetPacker) appendLongHeaderPacket(buffer *packetBuffer, header *wire.ExtendedHeader, pl payload, padding protocol.ByteCount, encLevel protocol.EncryptionLevel, sealer sealer, v protocol.Version) (*longHeaderPacket, error) {
from which I derived (*uPacketPacker).appendInitialPacket.

@gaukas gaukas left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good.

Can we move the definition and implementation of QUICPacket to after the end of `func (qfs QUICFrames) BuildFromFrames(frames []byte) (payload []byte, err error)? Just to keep each type definition and implementation together in one place.

Comment thread u_quic_frames.go
// and returns the byte representation of all frames as specified in
// the Frames slice. It then calculates the padding sizes needed to
// reach the specified Length and updates the PADDING frames accordingly.
func (qp QUICPacket) Build(cryptoData []byte) (payload []byte, err error) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
func (qp QUICPacket) Build(cryptoData []byte) (payload []byte, err error) {
func (qp *QUICPacket) Build(cryptoData []byte) (payload []byte, err error) {

Let's make *QUICPacket instead of QUICPacket the one interfacing QUICFrameBuilder, just to align with the style of *QUICRandomFrames. (QUICFrames is an exception as it is a slice instead of a ground-truth struct)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants