Skip to content

Attempt to flatten the BgpLayer inheritance chain. #1820

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

Draft
wants to merge 15 commits into
base: dev
Choose a base branch
from

Conversation

Dimi1010
Copy link
Collaborator

@Dimi1010 Dimi1010 commented May 18, 2025

Overview

This PR is a draft attempt to flatten the inheritance chain of BgpLayer and separate the protocol layer parsing logic from the message payload parsing logic.

In the current implementation each BGP message has a dedicated derived BGP layer class. This introduces limitations where the message payload cannot be switched to a different type without destroying the layer object and creating a new one, requiring the layer to be extracted from the stack and then readded to it.

This PR aims to simplify the following operations by providing the following objects:

  • "MessageView" proxies - thin wrappers over a BgpLayer that parse the payload as a specific message.
    They exist in two flavors "View" and "ConstView" with the former allowing in-place data modifications and the latter being read-only.
  • "Message" objects - lightweight struct objects that can be passed to the BgpLayer's constructor or a dedicated method to completely replace the underlying message with a message of a different type. Similar to the ArpLayer's helper structs.

The "View" object

The view object is a thin wrapper over a BgpLayer that holds a reference to the layer it has been created from. It provides accessors and mutators for reading and writing message specific data directly to the underlying buffer. Each view is intended to be passed by value and should be no larger than a pointer (the reference to the BgpLayer), if possible.

Each view should maintain message integrity. During construction the View should validate a minimally viable message of the type can be parsed from the buffer. Full validation can be deferred until use of accessors and/or mutators of the view object.

BGP Layer Enhancements

Improved support of full length update message path attributes.

The previous version supported path attributes with data up to 32 bytes. The new PathAttribute class supports the full spectrum of variable data lengths a path attribute block can contain specified by RFC 4271 Section 4.3.
The attribute can support a data buffer of:

  • standard length attribute - up to 255 bytes
  • extended length attribute - unlimited, restricted by total message length of 4096 bytes.

Other enhancements

Addition of utility functions for enum class flags.

This PR adds the header EnumFlagUtils.h to the Common++ package. The header provides operators for bitwise operators and helper functions of enum class objects when they satisfy the trait EnableBitMaskOperators.

In addition the header defines the following macros:

  • PCPP_DECLARE_ENUM_FLAG - to enable the operators to be used for a class.
  • PCPP_USING_ENUM_FLAG_OPERATORS - to declare aliases to all templated operators in the scope this macro is used to satisfy ADL.

Copy link

codecov bot commented May 18, 2025

Codecov Report

Attention: Patch coverage is 0% with 682 lines in your changes missing coverage. Please review.

Project coverage is 81.89%. Comparing base (59d0b64) to head (7be7843).
Report is 5 commits behind head on dev.

Files with missing lines Patch % Lines
Packet++/src/BgpLayer.cpp 0.00% 613 Missing and 3 partials ⚠️
Packet++/header/BgpLayer.h 0.00% 38 Missing and 2 partials ⚠️
Common++/src/IpAddress.cpp 0.00% 17 Missing ⚠️
Common++/header/EnumFlagUtils.h 0.00% 9 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #1820      +/-   ##
==========================================
- Coverage   84.10%   81.89%   -2.21%     
==========================================
  Files         272      285      +13     
  Lines       47578    49691    +2113     
  Branches    10344    10548     +204     
==========================================
+ Hits        40015    40695     +680     
- Misses       6518     8209    +1691     
+ Partials     1045      787     -258     
Flag Coverage Δ
alpine320 73.79% <0.00%> (-2.56%) ⬇️
fedora42 73.89% <0.00%> (-2.54%) ⬇️
macos-13 79.50% <0.00%> (-2.15%) ⬇️
macos-14 79.50% <0.00%> (-2.15%) ⬇️
macos-15 79.52% <0.00%> (-2.11%) ⬇️
mingw32 68.63% <0.00%> (-2.10%) ⬇️
mingw64 68.61% <0.00%> (-2.08%) ⬇️
npcap 82.69% <0.00%> (-2.40%) ⬇️
rhel94 73.63% <0.00%> (-2.58%) ⬇️
ubuntu2004 57.68% <0.00%> (-1.59%) ⬇️
ubuntu2004-zstd 57.81% <0.00%> (-1.46%) ⬇️
ubuntu2204 73.56% <0.00%> (-2.58%) ⬇️
ubuntu2204-icpx 59.92% <0.00%> (-1.58%) ⬇️
ubuntu2404 73.84% <0.00%> (-2.63%) ⬇️
ubuntu2404-arm64 73.82% <0.00%> (-2.57%) ⬇️
unittest 81.89% <0.00%> (-2.21%) ⬇️
windows-2019 82.72% <0.00%> (-2.38%) ⬇️
windows-2022 82.72% <0.00%> (-2.39%) ⬇️
winpcap 82.86% <0.00%> (-2.39%) ⬇️
xdp 49.71% <0.00%> (-2.78%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Dimi1010 added 5 commits May 18, 2025 15:04
…r for quick instantiation.

- Added internal::nocheck_t tag and a nocheck helper value.
- Replaced duplicated code from mutable views with instantiation of a const view and call to the respective function.
@Dimi1010
Copy link
Collaborator Author

@seladb @tigercosmos @clementperon @egecetin Can I get some first impressions on the idea of this draft?

@seladb
Copy link
Owner

seladb commented May 21, 2025

@Dimi1010 is it worth it to rewrite BgpLayer? 🤔

@Dimi1010
Copy link
Collaborator Author

Dimi1010 commented May 21, 2025

@Dimi1010 is it worth it to rewrite BgpLayer? 🤔

Dunno yet. I have been mostly using it as a test bed for expanding the idea I had in the #1813 about ways it can allow direct read/write of fields of different payloads without having to decode and copy the entire buffer out.

@tigercosmos
Copy link
Collaborator

tigercosmos commented May 22, 2025

@Dimi1010 Is it possible to split this PR? It seems the PR is too large to review.

@Dimi1010
Copy link
Collaborator Author

@Dimi1010 Is it possible to split this PR? It seems the PR is too large to review.

Possibly, but this is still a WIP.

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