Skip to content

Add additional RoCE extended transport headers (such as DETH and ImmDt) #3408

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

haggaie
Copy link
Contributor

@haggaie haggaie commented Oct 16, 2021

Allow parsing unreliable datagram headers. The code adds guess_payload_class
functions to find the correct sub-header according to BTH.opcode, and also
automatic selection of the opcode when possible.

@codecov
Copy link

codecov bot commented Oct 16, 2021

Codecov Report

Attention: Patch coverage is 96.29630% with 2 lines in your changes missing coverage. Please review.

Project coverage is 81.62%. Comparing base (1346522) to head (e43e500).

Files with missing lines Patch % Lines
scapy/contrib/roce.py 96.29% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3408   +/-   ##
=======================================
  Coverage   81.62%   81.62%           
=======================================
  Files         359      359           
  Lines       86052    86092   +40     
=======================================
+ Hits        70237    70275   +38     
- Misses      15815    15817    +2     
Files with missing lines Coverage Δ
scapy/contrib/roce.py 95.12% <96.29%> (-0.06%) ⬇️

... and 7 files with indirect coverage changes

@gpotter2
Copy link
Member

gpotter2 commented Nov 8, 2021

Very sorry about the delay regarding this PR !

@haggaie
Copy link
Contributor Author

haggaie commented Nov 14, 2021

Very sorry about the delay regarding this PR !

Don't worry about it 😄

@haggaie haggaie changed the title Add RoCE DETH and Immdt sub-headers Add RoCE DETH and ImmDt sub-headers Nov 14, 2021
@albydnc
Copy link

albydnc commented Nov 23, 2023

is this PR going to be merged?

@gpotter2
Copy link
Member

Not as-is, no. A lot feedback has not been addressed.

@haggaie
Copy link
Contributor Author

haggaie commented Feb 4, 2024

Hi @gpotter2, I would be happy to fix this pull request (and continue with other parts of the RoCE spec), but I wanted to consult with you on how to best represent the RoCE protocol stack within scapy. I believe the main issue with the pull request was how the code examines upper layers in order to determine the next layer. With RoCE, a single opcode field within the BTH header can decide several subsequent layers. For example, an opcode of UD_SEND_ONLY_WITH_IMMEDIATE should tell the parser that the next header after BTH is the DETH header, and the one after that is the ImmDt. I'm not sure what's the best way to represent this in scapy. I can see three alternatives:

  1. Using logic in guess_payload and post_build to describe the layers that are expected (as this patch tried to do).
  2. Using conditional fields and storing the full set of alternative headers as one big layer. E.g., we could have all the DETH fields and ImmDt fields moved into BTH under conditions that test the opcode for UD_SEND_ONLY or UD_SEND_ONLY_WITH_IMMEDIATE.
  3. Creating a separate layer for every combination under BTH and use bind_layers to link each opcode with the appropriate layer. For example, we could have a DETHImmDt layer for the combination of DETH and ImmDt and bind it to BTH when the opcode is UD_SEND_ONLY_WITH_IMMEDIATE.

I can see there are pros and cons to each method, so I wonder if the scapy project has a preferred approach, or if you have any other alternative I haven't considered.

Thanks,
Haggai

@haggaie haggaie force-pushed the contrib-roce-deth branch 3 times, most recently from 617993b to c783a2d Compare May 20, 2024 15:50
@haggaie haggaie changed the title Add RoCE DETH and ImmDt sub-headers Add additional RoCE extended transport headers (such as DETH and ImmDt) May 20, 2024
haggaie and others added 2 commits January 1, 2025 10:17
Allow parsing additional subheaders. Use ConditionalFields to enable
subheaders according to the opcode.
@haggaie haggaie force-pushed the contrib-roce-deth branch from c783a2d to e43e500 Compare January 1, 2025 10:13
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.

3 participants