TimestampType::HostHighPrec changed meaning in libpcap 1.10.0, and they introduced HOST_HIPREC_UNSYNC

[BGR360]

The current documentation for TimestampType::HostHighPrec states:

The timestamp might or might not be synchronized with the system clock

This matches the documentation for PCAP_TSTAMP_HOST_HIPREC in libpcap's pcap-tstamp(7) man page from versions 1.5.3 through 1.9.1.

Starting in 1.10.0, the meaning of HOST_HIPREC changed to the following:

This is a high-precision time stamp, synchronized with the host operating system's clock.

And they added a new timestamp type, HOST_HIPREC_UNSYNCED, which means the following:

This is a high-precision time stamp, not synchronized with the host operating system's clock.

I personally think this was a bad move as far as an API goes. They should have called the new thing HOST_HIPREC_SYNCED so that the meaning of HOST_HIPREC wouldn't differ between two versions of libpcap. But, it is what it is.

So the question is, what to do in this crate? I see two options:

Option 1: mirror what libpcap did and document the difference.

enum TimestampType {
    /// A timestamp provided by the host machine that is high precision. It might be more expensive
    /// to fetch than [`TimestampType::HostLowPrec`].
    ///
    /// In libpcap versions >= 1.10.0, this is known to be synchronized with the system clock. Prior
    /// to that, it might or not be.
    HostHighPrec = 2,

    /// A timestamp provided by the host machine that is high precision. It might be more expensive
    /// to fetch than [`TimestampType::HostLowPrec`].
    ///
    /// The timestamp is not synchronized with the system clock.
    HostHighPrecUnsynced = 5,

In this case, you could potentially add a method to TimestampType with behavior gated on libpcap version:

fn is_synchronized_to_host(&self) -> bool {
    match self {
        Self::HostLowPrec | Self::Adapter => true,
        Self::Host | Self::AdapterUnsynced | Self::HostHighPrecUnsynced => false,
        Self::HostHighPrec => {
            #cfg[/* < 1.10.0 */]
            false
            #cfg[/* >= 1.10.0 */]
            true
        }
    }
}

Option 2: do what libpcap probably should have done

Add a new variant HostHighPrecSynced and map HOST_HIPREC to that variant on libpcap >= 1.10.0. Map HOST_HIPREC_UNSYNCED to the existing HostHighPrec, which advertises as not necessarily synced.

enum TimestampType {
    /// A timestamp provided by the host machine that is high precision. It might be more expensive
    /// to fetch.
    ///
    /// The timestamp might or might not be synchronized with the system clock.
    HostHighPrec = 2,

    /// A timestamp provided by the host machine that is high precision. It might be more expensive
    /// to fetch.
    ///
    /// The timestamp is synchronized with the system clock.
    HostHighPrecSynced = 5,

Downside there is now the enum discriminants don't 1-to-1 match with libpcap. But this is a higher-level library than libpcap so I think that's fine.

Either way, is this a breaking change as far as semver goes? Adding a variant to a public enum? I'm not well-versed on Rust crate semver rules.

Other stuff to do

The documentation of TimestampType::HostLowPrec can directly say that it's synchronized to the host rather than be wishy-washy about it. All of the libpcap docs I found say that it's synchronized, not maybe synchronized.

I think you also have to check the return value of raw::pcap_set_tstamp_type() inside of Capture::<Inactive>::tstamp_type(), because it can fail.

Finally, I think adding a link to https://www.tcpdump.org/manpages/pcap-tstamp.7.html in TimestampType's documentation would be helpful for users of this crate.

[Wojtek242]

Thanks for submitting the issue! It seems well researched and is super well explained. I greatly appreciate the effort you put into this!

Generally, I am in favour of being conservative with this crate and creating as few surprises for the users. Therefore, I'd go with Option 1. This is a higher-level crate, but we should only introduce higher-level convenience at programming-level, not in terms of content. Since advanced users must still refer to libpcap docs, deviating from theirs is not a good move I think. I'm not sure is_synchronized_to_host is needed, but if you find it helpful, you can always add it in a PR.

The documentation of TimestampType::HostLowPrec can directly say that it's synchronized to the host rather than be wishy-washy about it. All of the libpcap docs I found say that it's synchronized, not maybe synchronized.

Sounds good

I think you also have to check the return value of raw::pcap_set_tstamp_type() inside of Capture::::tstamp_type(), because it can fail.

Good idea. It would require a major version update, but I have a proposal for this at the end of this comment.

Finally, I think adding a link to https://www.tcpdump.org/manpages/pcap-tstamp.7.html in TimestampType's documentation would be helpful for users of this crate.

Sounds good

Either way, is this a breaking change as far as semver goes? Adding a variant to a public enum? I'm not well-versed on Rust crate semver rules.

I think so. The CI runs a semver check which you can also run locally if you wish (https://github.com/obi1kenobi/cargo-semver-checks). I believe that changing a public enum requires a major version update since it breaks compilation and requires new behaviour. Making a previously "infallible" method fallible is also breaking.

However, everything you propose is desirable so I propose the following:

1. Add a new feature called timestamp-compat or something like that
2. Put the breaking changes behind the timestamp-compat feature. This should include the new enum variant and the new version of tstamp_type that may fail.

By using a feature-gate we can more freely work on timestamp compatibility without worrying about semver. Then, once pcap is ready for 3.0.0, the feature gate is removed and the new behaviour becomes standard.