Remove possibility of setFilter and clearFilter shadowing in derived devices.
#2005
Conversation
This prevents issues such as shadowing overloads when multiple overloads are available and only 1 is overloaded. For example: `setFilter` previously required `using::setFilter` declaration on every class that included an override it. The change consolidates all device specific filter update code in `doUpdateFilter` hook. The methods `setFiltter` (+overloads) / `clearFilter` are no longer virtual and can be overridden. Their implementation is now forwarded through `doUpdateFilter`.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #2005 +/- ##
==========================================
- Coverage 83.46% 83.46% -0.01%
==========================================
Files 311 311
Lines 54568 54566 -2
Branches 11808 11553 -255
==========================================
- Hits 45545 45541 -4
+ Misses 8313 7818 -495
- Partials 710 1207 +497
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:
|
| /// Clear the filter currently set on the device | ||
| /// @return True if filter was removed successfully or if no filter was set, false otherwise | ||
| virtual bool clearFilter() = 0; | ||
| bool clearFilter() |
There was a problem hiding this comment.
The problem as presented doesn't apply to clearFilter() because it only has one overload so nothing is shadowed. Why not keep it abstract?
Using "" as an indication to clear the filter seems a bit weird to me. I think clearFilter() is more explicit
There was a problem hiding this comment.
The problem as presented doesn't apply to
clearFilter()because it only has one overload so nothing is shadowed. Why not keep it abstract?
Simplification of the internal interface mostly. Having a single extension point for a certain functionality makes it harder to forget about part of it when overriding. This had happened in a couple of places already.
In the old code PcapNgFileReaderDevice and PcapNgFileWriterDevice only overrode setFilter to target m_BpfWrapper. Calling clearFilter would have had no effect on the filter, since that still used the IPcapDevice::clearFilter implementation that worked on m_PcapDescriptor.
My reasoning was that both setFilter and clearFilter can be conceptualized by the primitive action "update the filter", just with different parameters. This was also a factor when deciding to make the hook as doUpdateFilter and not for example doSetFilter.
The single extension point makes the above issue impossible, since to override the filter mechanism you have to update the entire hook. There is nothing to forget to override.
Afterwards clearFilter was made non-virtual to remove the possibility of overriding the method, thus potentially bypassing the base implementation, which is undesirable. Under the design derived classes should only modify the hook implementation.
Using
""as an indication to clear the filter seems a bit weird to me. I thinkclearFilter()is more explicit
For public API, I agree, clearFilter is more user friendly.
However, I primary made the the hook signature to fit the aforementioned design goals. It wasn't designed to be exposed to the public.
The choice of "" for "no filter" was due to PcapHandle and libpcap already treating it as match any packet, when passed to pcap_compile so I thought it might fit.
2 participants
Summary
The PR focuses on streamlining the logic for setting device filters, and removing possibility of errors. It overhauls the
IFilterableDeviceto utilize a Template Method Pattern, instead of direct pure virtual methods. The cause for the change is the problematic shadowing of methods when a derived class declares a override a virtual method. It fixes one such issue that was left unmitigated inPcapNGtype devices hidingsetFilter(GeneralFilter).Override methods and shadowing
The problem
The shadowing issue becomes relevant when a base class declares two or more overloads of a
virtualmethod. If a derived class overrides only one of the overloads, the C++ language spec hides all other overloads from the derived class's scope by default.For example:
Such issues are hard to spot due to their subtlety.
Such was the case in
PcapNGFileReaderDeviceandPcapNGFileWriterDevicewhich only declaredsetFilter(std::string), making the interfacesetFilter(GenericFilter const&)unavailable through the types.Solutions
There are three possible ways to fix this issue:
using Base::foo;declaration.Proposed solution
Previously, most of the devices utilized option 2, but that is brittle as it requires developer knowledge of the issue and scrutinous review as observed due to existing issues with PcapNG-type reader devices.
This PR refactors the code to utilize option 3 as an alternative. This was selected due to the overload
setFilter(GeneralFilter)cleanly delegating the majority of the work tosetFilter(std::string).The solution follows a Template Method Pattern by introducing a protected hook method
doUpdateFilter(std::string)and keeping the public API non-virtual.The change allows all calls from
setFilterandclearFilterto be delegated to the device hook, while keeping the public API unchanged. The following change has the following benefits:overrideoverloads, as they are no longervirtual. It is still possible to shadow a method by adding another overload in a derived class, but that situation is a lot more rare.overridenfor a specific device.Behaviour changes
clearFilternow mirrorssetFilterand requires the device to be in an open state due to reusing the same filter hook. This differs from the previous implementation whereclearFiltercould possibly succeed depending on the device's implementation of the method.