Correct timestamp handling for all fields and options #42
Conversation
Block::from_slice(), Block::write_to(), and all conversions from RawBlock to Block require a PcapNgState, because without state information the encoding is ambiguous. This was previously handled in an ad-hoc manner in the reader, writer and parser but is now centralised into the PcapNgState type. The previous functions for adjusting EPB timestamps are removed as they are no longer necessary. Timestamps are now converted as required in from_slice() and write_to(), according to the current PcapNgState.
This field follows the same rules as the EPB timestamp.
This field follows the same rules as the EPB timestamp.
This is necessary to return InvalidInterfaceId and TimestampTooBig.
| @@ -170,7 +126,7 @@ pub enum EnhancedPacketOption<'a> { | |||
| } | |||
|
|
|||
| impl<'a> PcapNgOption<'a> for EnhancedPacketOption<'a> { | |||
| fn from_slice<B: ByteOrder>(code: u16, length: u16, mut slice: &'a [u8]) -> Result<Self, PcapError> { | |||
| fn from_slice<B: ByteOrder>(_state: &PcapNgState, _interface_id: Option<u32>, code: u16, length: u16, mut slice: &'a [u8]) -> Result<Self, PcapError> { | |||
There was a problem hiding this comment.
I don't like that this function takes a state and interface_id, but doesn't use it. I see that it is needed because this is a trait. But, I don't see a better solution.
There was a problem hiding this comment.
It's pretty common to have unused parameters when implementing traits. The trait has to include all parameters that might be necessary, and in the general case, decoding an option correctly may require knowledge of the state as well as which interface is involved.
There was a problem hiding this comment.
Yes, I understand. In particular, the optional interface_id is a bit awkward in a context where there is no interface defined. As I said, I don't see a better solution.
My suggestion would be to review it commit-by-commit. I have already taken care to break it down into as many logical steps as possible, and the tests should pass at each commit.
It passes all the existing tests including those added in #37. But it would be good to have some more test cases as well, particularly some covering new features like the |
Up to release 2.0.0, the variable timestamp formats in PcapNg were not handled at all.
PR #37 added support for variable resolution for the timestamp field in
ExpandedPacketBlock, but this only addressed one aspect of the timestamping issues.InterfaceStatisticsBlock, theIsbStartTimeandIsbEndTimeoptions, and the obsolete but still supportedPacketBlock, all of which are defined to follow the same encoding/decoding rules.IfTsOffsetoption, which defines an offset in seconds that must be added or subtracted to relate timestamps to absolute time.The previous fix also introduced some new problems:
EnhancedPacketBlocktype became no longer constructable, because of the new privatewrite_ts_resolutionfield, as observed in v3.0.0-rc1 feedback #39. This complicates constructing new packets to save to a file.PcapError::TimestampTooBigvariant was added, but could never be returned, because the paths that lead to this failure were only able to returnstd::io::Error.This PR introduces a broader solution that addresses all of the above problems. The fundamental issue is that correctly encoding or decoding a block requires knowledge of the protocol state.
Currently, that state is maintained in the
section,interfacesandts_resolutionsfields onPcapNgParserandPcapNgWriter. The first step in this PR is to centralise that state into a commonPcapNgStatetype. All work necessary to update that state is centralised into itsupdate_from_blockmethod.All APIs that convert to and from
Blockare modified to require a reference to aPcapNgState. This allows encoding and decoding to be state-dependent. Timestamp encoding and decoding can then be correctly implemented wherever timestamps appear, by calling into helper methods onPcapNgStatein thefrom_sliceandwrite_tomethods of each block type. This eliminates the need to fix things up outside those methods.All timestamps which appear in blocks and options are now represented as a
Duration.This approach requires some one-off API changes, but provides a full solution, and also allows for correctly implementing any other state-dependent encoding/decoding of other blocks and options in the future.