-
Notifications
You must be signed in to change notification settings - Fork 703
Few fixes to support DPDK 23.11 and 24.11 #1774
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
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev #1774 +/- ##
==========================================
- Coverage 83.10% 83.10% -0.01%
==========================================
Files 283 283
Lines 48929 48929
Branches 10516 10516
==========================================
- Hits 40664 40661 -3
- Misses 7129 7130 +1
- Partials 1136 1138 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
m_Config.rssKey = nullptr; | ||
m_Config.rssKeyLength = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In DPDK 23.11/24.11 it won't let setting the RSS has function to RSS_NONE
but have something in rssKey
Pcap++/src/DpdkDevice.cpp
Outdated
@@ -346,7 +348,11 @@ namespace pcpp | |||
bool DpdkDevice::initQueues(uint8_t numOfRxQueuesToInit, uint8_t numOfTxQueuesToInit) | |||
{ | |||
rte_eth_dev_info devInfo; | |||
rte_eth_dev_info_get(m_Id, &devInfo); | |||
if (rte_eth_dev_info_get(m_Id, &devInfo) < 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In recent DPDK 24.11 they added __rte_warn_unused_result
to rte_eth_dev_info_get
and rte_eth_link_get
so we have to consider the return value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe add PCPP_LOG_ERROR no ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I agree, update in this commit: 6a44d08
RawPacket::setRawData(rte_pktmbuf_mtod(mBuf, const uint8_t*), rte_pktmbuf_pkt_len(mBuf), timestamp, | ||
LINKTYPE_ETHERNET); | ||
m_MBuf = mBuf; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was a bug that was introduced somewhere, not sure where: when calling RawPacket::setRawData()
it calls MBufRawPacket::clear()
which sets m_MBuf
to nullptr
, so we have to set the mbuf after calling RawPacket::setRawData()
, otherwise we get a segmentation fault
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a good explanation that should go in the commit log of "Fix a bug in MBufRawPacket::setMBuf"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We run "Squash and merge" so these commits go away after merging. That's why it's better to put them as comments, so anyone can go to the PR that made the change and look
@clementperon @egecetin @tigercosmos @Dimi1010 can one of you review this PR? |
The first PR to address issue #1770 (there'll be a follow-up PR).
I'm working on adding DPDK 23.11 and 24.11. These fixes are needed to support these versions. They should all be backward-compatible and work on older DPDK versions as well.
Once this PR is merged, I'll add Docker images for DPDK 23.11 and 24.11 (here is a draft PR for it: seladb/PcapPlusPlus-DockerImages#49) and open another PR to update our CI with the new images.