Skip to content
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

ptp: originTimeStamp field comma inconsistent between announce, sync #1260

Open
radeps opened this issue Dec 18, 2024 · 8 comments · May be fixed by #1277
Open

ptp: originTimeStamp field comma inconsistent between announce, sync #1260

radeps opened this issue Dec 18, 2024 · 8 comments · May be fixed by #1277

Comments

@radeps
Copy link

radeps commented Dec 18, 2024

Tested and present in two versions:

macOS Sonoma 14.6.1
tcpdump version 4.99.1 -- Apple version 137
libpcap version 1.10.1
LibreSSL 3.3.6

Rocky 9.5
tcpdump version 4.99.0
libpcap version 1.9.1

Inside an announce message:

originTimeStamp : 10791 seconds 967464480 nanoseconds

Inside a sync message:

originTimeStamp : 10792 seconds, 180 nanoseconds

I would quickly create a PR myself but I am unsure about the conventions that should be used here since other subfields (such as NS correction/sub NS correction) are broken up into two comma-separated titled fields.

Full comparison between two packets:

11:26:32.381032 IP 192.168.1.5.ptp-general > 224.0.1.129.ptp-general: PTPv2, v1 compat : no, msg type : announce msg, length : 64, domain : 127, reserved1 : 0, Flags [none], NS correction : 0, sub NS correction : 0, reserved2 : 0, clock identity : [redacted], port id : 1, seq id : 18704, control : 5 (Other), log message interval : 254, originTimeStamp : 10791 seconds 967464480 nanoseconds, origin cur utc :37, rsvd : 0, gm priority_1 : 126, gm clock class : 248, gm clock accuracy : 254, gm clock variance : 65535, gm priority_2 : 128, gm clock id : [redacted], steps removed : 0, time source : 0xa0
11:26:32.413549 IP 192.168.1.5.ptp-event > 224.0.1.129.ptp-event: PTPv2, v1 compat : no, msg type : sync msg, length : 44, domain : 127, reserved1 : 0, Flags [none], NS correction : 0, sub NS correction : 58654720, reserved2 : 0, clock identity : [redacted], port id : 1, seq id : 37410, control : 0 (sync msg), log message interval : 253, originTimeStamp : 10792 seconds, 180 nanoseconds
@infrastation
Copy link
Member

Since the existing code uses a comma to separate different data elements and calls the seconds + nanoseconds pair a timestamp element, it would make the most sense to me not to use a comma within the timestamp in both cases, i.e. to remove the comma from ptp_print_timestamp().

@fxlb
Copy link
Member

fxlb commented Dec 19, 2024

+1

@guyharris
Copy link
Member

Is there a compelling reason not to represent the first example as

originTimeStamp : 10791.967464480 seconds

and the second example as

originTimeStamp : 10792.000000180 seconds

@fxlb
Copy link
Member

fxlb commented Dec 19, 2024

No. We have to change the output and your proposal is in line with some %09u usages in p_ntp_time_fmt(), p_ntp_delta(), ts_frac_print(), etc.

@infrastation
Copy link
Member

One format or the other, it should indicate an invalid (> 999999999) value of ns as was done in commit 8db1e99 earlier.

@fxlb
Copy link
Member

fxlb commented Dec 20, 2024

We could also signal bogus packet timestamp with an exclamation mark, such as:
1 2004-06-14 10:13:29.4294643617! IP [...]
and use an exclamation mark for other such bogus date/time (>999999 for ms, > 999999999 for ns).

@infrastation
Copy link
Member

A word is a bit easier to notice on a busy screen.

@fxlb
Copy link
Member

fxlb commented Dec 21, 2024

A word is a bit easier to notice on a busy screen.

Yes.

infrastation added a commit to infrastation/tcpdump that referenced this issue Jan 9, 2025
Move nanosecond-specific constants from print-arista.c to netdissect.h.
In ptp_print_timestamp_identity() and ptp_print_announce_msg() use
ptp_print_timestamp() to deduplicate code.  In ptp_print_timestamp()
change the format to SECONDS.NANOSECONDS and a bogus nanoseconds value
indicator.  Update the tests.  This resolves GH issue the-tcpdump-group#1260.
@infrastation infrastation linked a pull request Jan 9, 2025 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

4 participants