Skip to content

Conversation

@nibanks
Copy link
Collaborator

@nibanks nibanks commented Aug 22, 2023

Description

Cleans up a few things:

  1. Creates a clear abstraction between QUIC and platform layer.
  2. Renames QUIC layer objects to remove CXPLAT name.
  3. Refactors internals of datapaths to have similar structs/names to make it easier to understand (and maybe converge eventually).

Testing

Existing automation

Documentation

N/A

@nibanks nibanks requested a review from a team as a code owner August 22, 2023 18:17
@codecov
Copy link

codecov bot commented Aug 22, 2023

Codecov Report

Merging #3826 (9796ddd) into main (cd49fbb) will increase coverage by 0.00%.
The diff coverage is 86.48%.

@@           Coverage Diff           @@
##             main    #3826   +/-   ##
=======================================
  Coverage   86.57%   86.57%           
=======================================
  Files          56       56           
  Lines       16576    16581    +5     
=======================================
+ Hits        14350    14355    +5     
  Misses       2226     2226           
Files Changed Coverage Δ
src/core/binding.h 63.15% <ø> (ø)
src/core/datagram.c 90.31% <ø> (-1.17%) ⬇️
src/core/library.c 83.10% <ø> (-0.81%) ⬇️
src/core/packet.h 97.93% <ø> (ø)
src/core/stream.h 93.33% <ø> (ø)
src/core/stream_recv.c 87.77% <ø> (-0.63%) ⬇️
src/core/binding.c 87.55% <77.41%> (+0.14%) ⬆️
src/core/connection.c 81.50% <88.00%> (-0.07%) ⬇️
src/core/packet.c 76.81% <100.00%> (-0.25%) ⬇️
src/core/worker.c 89.45% <100.00%> (+1.70%) ⬆️

... and 7 files with indirect coverage changes

// Internal receive context.
//
typedef struct CXPLAT_DATAPATH_INTERNAL_RECV_BUFFER_CONTEXT {
typedef struct DECLSPEC_ALIGN(MEMORY_ALLOCATION_ALIGNMENT) DATAPATH_RX_PACKET {
Copy link
Collaborator

Choose a reason for hiding this comment

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

DECLSPEC_ALIGN(MEMORY_ALLOCATION_ALIGNMENT)

Why do we need this alignment requirement here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

If it's actually needed, we then need a comment similar to what we have in the XDP datapath.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure if it's strictly necessary. It was copy/paste.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It doesn't look like it's necessary. There is no SLIST operations in this file.

I will leave it up to you.

CXPLAT_RECV_DATA Data;

} CXPLAT_DATAPATH_INTERNAL_RECV_BUFFER_CONTEXT;
} DATAPATH_RX_PACKET;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd add a comment saying there's a client data following the last field.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There is already a comment at the end of CXPLAT_RECV_DATA to that effect. I'm going to leave that as is for now.

@nibanks nibanks merged commit 972e677 into main Aug 23, 2023
@nibanks nibanks deleted the nibanks/dal-rx-packet branch August 23, 2023 19:14
@wfurt wfurt mentioned this pull request Nov 14, 2023
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants