Add support for timestamp related options (#1656) #1657
Conversation
vcomito-apexai
force-pushed
the
1656-add-hardware-timestamp-config-support
branch
from
a06273f to
2ead59f
Compare
Pcap++/src/PcapLiveDevice.cpp
Outdated
| @@ -308,6 +340,34 @@ namespace pcpp | |||
| } | |||
| #endif | |||
|
|
|||
| if (config.timestampProvider) | |||
| { | |||
| ret = pcap_set_tstamp_type(pcap, timestampProviderMap(config.timestampProvider)); | |||
There was a problem hiding this comment.
According to libpcap docs we should first call pcap_list_tstamp_types to figure out which type are possible for this device. I'm not sure what happens if we set a type that isn't supported...
There was a problem hiding this comment.
@seladb the behavior is described here:
https://www.tcpdump.org/manpages/libpcap-1.10.5/pcap_set_tstamp_type.3pcap.html
if I read it correctly, this means that:
- the error will be printed
- the default timestamp type will be used.
Let me know if you prefer to add the check and e.g. throw an exception if the timestamp is not supported
There was a problem hiding this comment.
Thanks @vcomito-apexai ! I think it's ok to log an error
vcomito-apexai
force-pushed
the
1656-add-hardware-timestamp-config-support
branch
from
2ead59f to
2b27626
Compare
|
It seems that clang-tidy finds issues unrelated to this PR https://github.com/seladb/PcapPlusPlus/pull/1657/checks How may I address those? In a separate PR? |
vcomito-apexai
force-pushed
the
1656-add-hardware-timestamp-config-support
branch
from
2b27626 to
be0c2a8
Compare
vcomito-apexai
force-pushed
the
1656-add-hardware-timestamp-config-support
branch
from
be0c2a8 to
01957ac
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #1657 +/- ##
==========================================
- Coverage 83.16% 83.08% -0.09%
==========================================
Files 277 277
Lines 48193 48263 +70
Branches 9966 10222 +256
==========================================
+ Hits 40080 40097 +17
- Misses 7234 7267 +33
- Partials 879 899 +20
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
ah, appearently they are fixed after rebase 👌 |
Any more changes I should apply to the PR :) ? |
| else if (numSupportedTstampTypes == 1) | ||
| { | ||
| // If 1 is returned, then the only available typestamp is TimestampProvider::Host; | ||
| return timestampProvider == PcapLiveDevice::TimestampProvider::Host; |
There was a problem hiding this comment.
Do we not have to free supportedTstampTypes here?
| { | ||
| int tstampType = timestampProviderMap(timestampProvider); | ||
| int* supportedTstampTypes = nullptr; | ||
| const int numSupportedTstampTypes = pcap_list_tstamp_types(pcap, &supportedTstampTypes); |
There was a problem hiding this comment.
It might be useful to wrap supportedTstampTypes into a std::unique_ptr with a custom deleter, as that would handle the deallocation automatically when the ptr goes out of scope.
| static bool isTimestampProviderSupportedByDevice(pcap_t* pcap, | ||
| const PcapLiveDevice::TimestampProvider timestampProvider) | ||
| { | ||
| int tstampType = timestampProviderMap(timestampProvider); |
There was a problem hiding this comment.
nit: you can use auto instead of int
There was a problem hiding this comment.
It doesn't actually save you much in this case tho, does it? 🤔
As described by #1656
@seladb would it be possible to enable the CI?