Skip to content
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

Consolidate shared DeviceList functionality under a common base class. #1488

Open
wants to merge 83 commits into
base: dev
Choose a base branch
from

Conversation

Dimi1010
Copy link
Collaborator

@Dimi1010 Dimi1010 commented Jul 7, 2024

Summary

This is a rework of #1431, parts 1 and 2.

The list classes now provide direct functions for iteration and fetching devices. The previous approach of retrieving another 'List' from a 'List' class was redundant and tied the internal implementation to it, making future changes more difficult.

Changes:

  • PointerVector:

    • Introduced a deleter extension point by changing it to PointerVector<T, Deleter>.
  • DeviceListBase:

    • Added a new base class DeviceListBase<DeviceType> to implement common functionality shared among device lists.
    • Common functionality includes:
      • A protected PointerVector<DeviceType> member variable.
      • Element accessor methods: at(), front(), back().
      • Iterator accessor methods: begin(), end(), cbegin(), cend().
      • Capacity accessors: size(), empty().
  • Refactoring:

    • Refactored all device list classes to inherit from DeviceListBase.
    • Removed the iterator API implementation from RemoteDeviceList, which is now superseded by the inherited iterator API from DeviceListBase.
    • Deprecated the getPcap*DevicesList methods, which have been superseded by the direct accessor methods provided by DeviceListBase.

Affected Lists:

  • PcapLiveDeviceList

  • PfRingDeviceList

  • DpdkDeviceList

  • Deprecated Methods:

    • Updated all usages of deprecated methods in the library to use the new, non-deprecated methods.

@Dimi1010 Dimi1010 marked this pull request as ready for review July 8, 2024 22:20
@Dimi1010 Dimi1010 requested a review from seladb as a code owner July 8, 2024 22:20
Copy link
Collaborator

@tigercosmos tigercosmos left a comment

Choose a reason for hiding this comment

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

LGTM

- Added iterator API
- Added iterator type aliases.
- Deprecated getPfRingDevicesList.
- Updated calls to getPfRingDevicesList.
@Dimi1010
Copy link
Collaborator Author

@tigercosmos added PfRingDeviceList to the refactor last minute. Sry for the re-review request.

Dimi1010 and others added 2 commits July 21, 2024 15:13
…-management

Conflicts:
	Pcap++/header/PcapRemoteDeviceList.h
	Pcap++/src/PcapRemoteDeviceList.cpp
@seladb
Copy link
Owner

seladb commented Jul 22, 2024

@Dimi1010 CI fails for some reason in generating the codecov report, and idea why? 🤔

@clementperon
Copy link
Collaborator

Owner

I have the same issue in my MR
#1370

I didn't touch to the code at all

@seladb
Copy link
Owner

seladb commented Jul 24, 2024

@clementperon gcovr complains it finds the same method pcpp::PcapLiveDeviceList::getInstance() in multiple lines: 51 ,63.
Apparently line 51 is the location of the method before the change, and line 63 is the location after the change:
image

gcovr also complains about a new file version when it compares the file to the file in .ccache. Maybe the issue is related to ccache we just added?

Stderr of gcov was >>/__w/PcapPlusPlus/PcapPlusPlus/Examples/IcmpFileTransfer/Common.cpp:source file is newer than notes file '__w/PcapPlusPlus/PcapPlusPlus/.ccache/d/d/fac836f17d373ef9882523799d4b3d-1445515.gcno'
(the message is displayed only once per source file)
/__w/PcapPlusPlus/PcapPlusPlus/Packet++/header/Packet.h:source file is newer than notes file '__w/PcapPlusPlus/PcapPlusPlus/.ccache/d/d/fac836f17d373ef9882523799d4b3d-1445515.gcno'
/__w/PcapPlusPlus/PcapPlusPlus/Packet++/header/IcmpLayer.h:source file is newer than notes file '__w/PcapPlusPlus/PcapPlusPlus/.ccache/d/d/fac836f17d373ef9882523799d4b3d-1445515.gcno'
/__w/PcapPlusPlus/PcapPlusPlus/Packet++/header/IPv4Layer.h:source file is newer than notes file '__w/PcapPlusPlus/PcapPlusPlus/.ccache/d/d/fac836f17d373ef9882523799d4b3d-1445515.gcno'
/__w/PcapPlusPlus/PcapPlusPlus/Packet++/header/TLVData.h:source file is newer than notes file '__w/PcapPlusPlus/PcapPlusPlus/.ccache/d/d/fac836f17d373ef9882523799d4b3d-1445515.gcno'
/__w/PcapPlusPlus/PcapPlusPlus/Packet++/header/IPLayer.h:source file is newer than notes file '__w/PcapPlusPlus/PcapPlusPlus/.ccache/d/d/fac836f17d373ef9882523799d4b3d-1445515.gcno'
/__w/PcapPlusPlus/PcapPlusPlus/Packet++/header/EthLayer.h:source file is newer than notes file '__w/PcapPlusPlus/PcapPlusPlus/.ccache/d/d/fac836f17d373ef9882523799d4b3d-1445515.gcno'
/__w/PcapPlusPlus/PcapPlusPlus/Common++/header/PcapPlusPlusVersion.h:source file is newer than notes file '__w/PcapPlusPlus/PcapPlusPlus/.ccache/d/d/fac836f17d373ef9882523799d4b3d-1445515.gcno'
/__w/PcapPlusPlus/PcapPlusPlus/Pcap++/header/PcapLiveDeviceList.h:source file is newer than notes file '__w/PcapPlusPlus/PcapPlusPlus/.ccache/d/d/fac836f17d373ef9882523799d4b3d-1445515.gcno'
__w/PcapPlusPlus/PcapPlusPlus/.ccache/d/d/fac836f17d373ef9882523799d4b3d-1445515.gcda:cannot open data file, assuming not executed
<< End of stderr
Exception was >>Got function pcpp::PcapLiveDeviceList::getInstance() on multiple lines: 51, 63.
	You can run gcovr with --merge-mode-functions=MERGE_MODE.
	The available values for MERGE_MODE are described in the documentation.<< End of stderr

…-management

# Conflicts:
#	Tests/Pcap++Test/Tests/LiveDeviceTests.cpp
@seladb
Copy link
Owner

seladb commented Aug 26, 2024

@Dimi1010 although it's not a breaking change, it's a pretty big change that I'm not sure we should introduce just before the upcoming release. We already have a few deprecation and breaking changes...

What do you think about waiting with this PR until the release?

@Dimi1010
Copy link
Collaborator Author

@seladb Sure, no problem.

@tigercosmos
Copy link
Collaborator

@Dimi1010 although it's not a breaking change, it's a pretty big change that I'm not sure we should introduce just before the upcoming release. We already have a few deprecation and breaking changes...

What do you think about waiting with this PR until the release?

I'm also uncertain about the statement. Even if we don't include this in the upcoming release, the next one is likely to introduce even more breaking changes.

@seladb
Copy link
Owner

seladb commented Aug 26, 2024

@Dimi1010 although it's not a breaking change, it's a pretty big change that I'm not sure we should introduce just before the upcoming release. We already have a few deprecation and breaking changes...
What do you think about waiting with this PR until the release?

I'm also uncertain about the statement. Even if we don't include this in the upcoming release, the next one is likely to introduce even more breaking changes.

I hope that we'll release the version more frequently than the existing one, so hopefully each version will include fewer API changes and breaking changes

Pcap++/src/DpdkDeviceList.cpp Outdated Show resolved Hide resolved
Pcap++/src/PfRingDeviceList.cpp Outdated Show resolved Hide resolved
@Dimi1010
Copy link
Collaborator Author

@seladb can we revisit this one for merge?

@Dimi1010 Dimi1010 requested a review from seladb November 27, 2024 20:08
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