-
Notifications
You must be signed in to change notification settings - Fork 721
Added const qualified match methods to BfpFilterWrapper. #1957
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
Added const qualified match methods to BfpFilterWrapper. #1957
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #1957 +/- ##
========================================
Coverage 83.45% 83.45%
========================================
Files 311 311
Lines 54923 54982 +59
Branches 11822 12121 +299
========================================
+ Hits 45836 45887 +51
+ Misses 7825 7824 -1
- Partials 1262 1271 +9
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:
|
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.
I left a few comments, but in general, many changes make this PR hard to review. Maybe we can break it up to multiple PRs?
/// @brief Match a raw packet against the filter. | ||
/// @param rawPacket The raw packet to match against. | ||
/// @return True if the filter matches (or if it's empty). False otherwise. | ||
bool matches(RawPacket const& rawPacket) const; |
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 an overload of matchPacketWithFilter
, no? Why not keep the same name?
Also - maybe add const
qualifier to the existing methods?
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.
I suppose it can be done, but I made a new method for two reasons.
For one, matchPacketWithFilter()
takes a pointer param, matches()
takes a const reference parameter.
The second one is that IMO matchPacketWithFilter
is a bit overly verbose for a method to a filter object? The object itself is already a filter, so there isn't much else to match with? The flow would then be filter -> matches -> rawPacket
, which IMO would read cleaner. I figured that since the parameter signature is changing, might as well do both changes at once.
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.
Having methods with different names that do more or less the same thing is confusing. We can deprecate the old methods and generate matches
methods instead, but I think it'd be easier to just stick with the same name
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.
I'm fine with deprecating the old ones. I think matches
makes for a more user friendly API since it is more concise.
While we are on the topic. I think the IPcapDevice::matchPacketWithFilter
should probably be deprecated too. It is a static method that is somewhat redundant. It requires the user to pass a filter object and a packet. If the user already has that, he can just call filter.match***(packet)
instead, which IMO would be more readable than IPcapDevice::matchPacketWithFilter(filter, packet)
.
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.
I'm fine with deprecating the old ones. I think
matches
makes for a more user friendly API since it is more concise.
I guess we can deprecate them? 🤔
While we are on the topic. I think the
IPcapDevice::matchPacketWithFilter
should probably be deprecated too. It is a static method that is somewhat redundant. It requires the user to pass a filter object and a packet. If the user already has that, he can just callfilter.match***(packet)
instead, which IMO would be more readable thanIPcapDevice::matchPacketWithFilter(filter, packet)
.
I agree, I think we can deprecate it. To be honest, I don't remember why it was added, there was probably a reason but I don't recall what it was
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.
@seladb Do you want to deprecate them here or in a new PR so it is cleaner? IMO, a clean PR afterwards would be better for finding the deprecations later.
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.
It's ok to open a new PR for the deprecations 👍
Pcap++/src/PcapFilter.cpp
Outdated
bool GeneralFilter::cacheFilterInternal() const | ||
{ | ||
std::string filterStr; | ||
parseToString(filterStr); | ||
if (!m_BpfWrapper.setFilter(filterStr)) | ||
return false; | ||
|
||
m_CachedFilter = true; | ||
return true; | ||
} |
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.
Why do we need this method? BpfFilterWrapper::setFilter()
already prevents resetting the filter if it hasn't changed
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.
The new code prevents the need to recompute the filter string, which was previously done on every call, to check if the filter needs to be reset. Now it is just a bool check.
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.
I don't think recomputing the filter string is a big deal since it is not done in real time
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.
Wdym not done in real time?
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.
Setting a filter (usually) isn't time-critical, meaning that spending a few more cycles on parsing the string shouldn't be an issue. As opposed to parsing a packet which is done in real-time so should be as fast as possible
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.
Got it. I dug deeper into it, and the complexity is about invalidating the cache when the filter changes - we need to call invalidateCache()
in every setter, and we might miss one (for example, you missed calling it in setDirection()
🙂 ).
I'm not sure how to solve it though...
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.
Fixed the invalidate miss. Thanks! b06fcef
I am not sure there is a better way to do it. With this optimization the derived classes need to signal the base implementation to regenerate the cache somehow, since they add new public mutators that the base doesn't know about.
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.
I asked ChatGPT, and it suggested using a Property<T>
Template Wrapper:
#include <iostream>
#include <functional>
template<typename T>
class Property {
T value;
std::function<void()> beforeSetter;
public:
Property(std::function<void()> beforeSetterFunc = nullptr)
: beforeSetter(beforeSetterFunc) {}
T get() const {
return value;
}
void set(const T& val) {
if (beforeSetter) beforeSetter();
value = val;
}
// Optional: Implicit conversion to T
operator T() const {
return value;
}
};
class Base {
protected:
void beforeSetter() {
std::cout << "[Base] beforeSetter called\n";
}
public:
Property<int> x;
Base() : x([this]() { beforeSetter(); }) {}
// Optional convenience
void setX(int val) { x.set(val); }
int getX() const { return x.get(); }
};
class DerivedA : public Base {
protected:
void beforeSetter() {
std::cout << "[DerivedA] beforeSetter called\n";
}
public:
Property<std::string> name;
DerivedA() : name([this]() { beforeSetter(); }) {}
void setName(const std::string& n) { name.set(n); }
std::string getName() const { return name.get(); }
};
Similarly, it suggested a PropertyVector<T>
for vectors:
#include <vector>
#include <algorithm>
template<typename T>
class PropertyVector {
std::vector<T> value;
std::function<void()> beforeSetter;
public:
PropertyVector(std::function<void()> beforeSetterFunc = nullptr)
: beforeSetter(beforeSetterFunc) {}
const std::vector<T>& get() const {
return value;
}
void set(const std::vector<T>& vec) {
if (beforeSetter) beforeSetter();
value = vec;
}
void add(const T& element) {
if (beforeSetter) beforeSetter();
value.push_back(element);
}
bool remove(const T& element) {
if (beforeSetter) beforeSetter();
auto it = std::find(value.begin(), value.end(), element);
if (it != value.end()) {
value.erase(it);
return true;
}
return false;
}
std::size_t size() const {
return value.size();
}
T& operator[](std::size_t idx) {
return value[idx];
}
const T& operator[](std::size_t idx) const {
return value[idx];
}
};
It's a nice pattern but I'm not sure if it's better than just calling invalidateCache()
, what do you think?
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.
Looks nice, but I think its not worth it in the current case. invalidateCache()
will most likely be inlined as it is a single bool operation. Going through std::function
removes that possibility as that is a call through function pointer, which the compiler doesn't know anything about at compile time.
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.
There is also the fact that this will also need to be setup in the derived classes, which makes it no different than simply calling invalidateCache()
.
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.
Pull Request Overview
Adds const-qualified match methods and caching to BpfFilterWrapper and GeneralFilter to enable matching packets on const instances, introduces move semantics, and refactors filter compilation and caching.
- Add const overloads: BpfFilterWrapper::matchPacketWithFilter and new matches(...) helpers
- Introduce filter program caching with on-demand recompilation when link type changes
- Make parseToString methods const across filters and add cache invalidation hooks in setters
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
File | Description |
---|---|
Pcap++/src/PcapFilter.cpp | Implements new const matching logic, filter compilation helper, cache handling, and updates parseToString definitions. |
Pcap++/header/PcapFilter.h | Declares new API (const matches methods, move ctor/assign), adds caching state to GeneralFilter and const parseToString signatures, and adds invalidateCache to mutators. |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
This PR updates
BpfFilterWrapper
to allow matching with a packet while the filter is marked asconst
.It also extends the wrapper with C++11 move constructors and a new constructor to create an instance with a set filter.
It additionally adds
const
qualified match methods toGenericFilter
.