feat(stdout/tcp): default to relative tcp sequence numbers #354

mozillazg · 2025-10-06T13:36:57Z

Closes #156

Summary by CodeRabbit

New Features
- Added --absolute-tcp-sequence-numbers (-S) to show absolute TCP sequence numbers (relative is now default).
- Added --utc-time to display timestamps in UTC.
Documentation
- Updated README and README.zh-CN for new flags and behavior.
- Added/updated CHANGELOG entries.
Tests
- Expanded coverage with new fixtures and cases for sequence-number modes, timestamps, and formatting variants.
Chores
- Bumped vendored dependency version.

This change makes the default behavior to output relative TCP sequence numbers. The `-S` flag is introduced to output absolute TCP sequence numbers for backward compatibility. This makes the output more readable for humans. The test suite has been updated to reflect this change, and new tests with the `-S` flag have been added to ensure correctness.

coderabbitai · 2025-10-06T13:37:12Z

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Out of Scope Changes Check	⚠️ Warning	The PR includes additions unrelated to the “relative TCP sequence numbers” goal such as the new --utc-time flag, corresponding UTC timestamp support in the stdout writer, and documentation/test fixtures for timestamp formatting that are not part of issue #156’s scope.	These UTC timestamp features and related test coverage should be extracted into a separate PR or explicitly linked to a separate issue, leaving this PR focused solely on implementing default relative TCP sequence numbering.
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check	✅ Passed	The title concisely describes the primary feature addition—defaulting to relative TCP sequence numbering in the stdout TCP formatter—matching the main change in the PR.
Linked Issues Check	✅ Passed	The changes implement default relative TCP sequence number formatting with an override flag -S/--absolute-tcp-sequence-numbers, add the necessary state tracking and helper methods in the stdout writer and pktdump vendor code, and include test fixtures covering both default relative and absolute modes, satisfying the linked issue’s goal for relative numbering and suppression when no advancement occurs.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch support-relative-tcp-seq

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 3

🧹 Nitpick comments (5)

testdata/format/tcp.pcapng.-tttt.out.txt (1)

5-24: Relative TCP seq formatting and ACK-only suppression look correct.

Post-handshake uses relative seq/ack (e.g., ack 1, seq 1:87).

Pure ACK lines omit seq. Data/FIN math is consistent.

Consider adding fixtures for retransmissions, out-of-order segments, and 32-bit wraparound to harden relative mapping.

cmd/writer.go (1)

59-59: Add nil-check for formatter in StdoutWriter.Write
StdoutWriter.Init is idempotent; however, Write() will panic if Init() isn’t called (w.formatter == nil). Add a guard at the start of Write() to return a clear error when the formatter is uninitialized. No code paths create a StdoutWriter without Init().
internal/writer/stdout.go (1)
70-75: Guard against missing Init to avoid nil deref

If some code path forgets to call Init(), w.formatter/w.formatOpts will be nil. Add a lazy init to be safe.

Apply near the top of Write():
 func (w *StdoutWriter) Write(e *event.Packet) error {
   if w.DoNothing {
     return nil
   }
+  if w.formatter == nil || w.formatOpts == nil {
+    w.Init()
+  }
Ensure all writer creation paths invoke Init() (cmd/writer.go already does per PR summary).
vendor/github.com/x-way/pktdump/format.go (2)
490-496: Avoid re-entering defaultFormatter; call the receiver’s formatter to prevent deadlock/state corruption

These nested FormatWithStyle calls use the package-level defaultFormatter introduced above. If Format uses a lock, this can deadlock. If not, it can race on shared state. Call the receiver’s non-exported formatter instead.

Apply:
@@
-       case layers.EthernetTypeIPv4:
-           out += FormatWithStyle(gopacket.NewPacket(gre.LayerPayload(), layers.LayerTypeIPv4, gopacket.Default), f.opts.HeaderStyle)
+       case layers.EthernetTypeIPv4:
+           out += f.formatWithOptions(gopacket.NewPacket(gre.LayerPayload(), layers.LayerTypeIPv4, gopacket.Default))
-       case layers.EthernetTypeIPv6:
-           out += FormatWithStyle(gopacket.NewPacket(gre.LayerPayload(), layers.LayerTypeIPv6, gopacket.Default), f.opts.HeaderStyle)
+       case layers.EthernetTypeIPv6:
+           out += f.formatWithOptions(gopacket.NewPacket(gre.LayerPayload(), layers.LayerTypeIPv6, gopacket.Default))
@@
-   case layers.PPPTypeIPv4:
-       return FormatWithStyle(gopacket.NewPacket(ppp.LayerPayload(), layers.LayerTypeIPv4, gopacket.Default), f.opts.HeaderStyle)
+   case layers.PPPTypeIPv4:
+       return f.formatWithOptions(gopacket.NewPacket(ppp.LayerPayload(), layers.LayerTypeIPv4, gopacket.Default))
-   case layers.PPPTypeIPv6:
-       return FormatWithStyle(gopacket.NewPacket(ppp.LayerPayload(), layers.LayerTypeIPv6, gopacket.Default), f.opts.HeaderStyle)
+   case layers.PPPTypeIPv6:
+       return f.formatWithOptions(gopacket.NewPacket(ppp.LayerPayload(), layers.LayerTypeIPv6, gopacket.Default))
Also applies to: 531-534

59-81: Unreachable fallback for ACK printing

In the relative branch, ackPart is always set when tcp.ACK is true, so:

if ackPart == "" && tcp.ACK { out += fmt.Sprintf(", ack %d", tcp.Ack) }

never triggers. Remove to simplify.
-        if ackPart == "" && tcp.ACK {
-            out += fmt.Sprintf(", ack %d", tcp.Ack)
-        }

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 324649d and 9f18ef0.

⛔ Files ignored due to path filters (1)

go.sum is excluded by !**/*.sum

📒 Files selected for processing (71)

CHANGELOG.md (1 hunks)
README.md (3 hunks)
README.zh-CN.md (1 hunks)
cmd/options.go (3 hunks)
cmd/read.go (1 hunks)
cmd/read_test.go (23 hunks)
cmd/root.go (1 hunks)
cmd/writer.go (1 hunks)
go.mod (1 hunks)
internal/writer/stdout.go (5 hunks)
testdata/format/curl-thread.pcapng.-S.out.txt (1 hunks)
testdata/format/curl-thread.pcapng.-v.-S.out.txt (1 hunks)
testdata/format/curl-thread.pcapng.-v.out.txt (3 hunks)
testdata/format/curl-thread.pcapng.out.txt (1 hunks)
testdata/format/curl-user.pcapng.-S.out.txt (1 hunks)
testdata/format/curl-user.pcapng.-v.-S.out.txt (1 hunks)
testdata/format/curl-user.pcapng.-v.out.txt (1 hunks)
testdata/format/curl-user.pcapng.out.txt (1 hunks)
testdata/format/ipoptions.pcap.-S.out.txt (1 hunks)
testdata/format/ipoptions.pcap.-v.-S.out.txt (1 hunks)
testdata/format/ipoptions.pcap.-v.out.txt (1 hunks)
testdata/format/ipoptions.pcap.out.txt (1 hunks)
testdata/format/mptcp.pcap.-S.out.txt (1 hunks)
testdata/format/mptcp.pcap.out.txt (1 hunks)
testdata/format/tcp.pcapng.-A.-S.out.txt (1 hunks)
testdata/format/tcp.pcapng.-A.out.txt (5 hunks)
testdata/format/tcp.pcapng.-S.out.txt (1 hunks)
testdata/format/tcp.pcapng.-X.-S.out.txt (1 hunks)
testdata/format/tcp.pcapng.-X.out.txt (7 hunks)
testdata/format/tcp.pcapng.-XX.-S.out.txt (1 hunks)
testdata/format/tcp.pcapng.-XX.out.txt (7 hunks)
testdata/format/tcp.pcapng.-c.-S.out.txt (1 hunks)
testdata/format/tcp.pcapng.-q.-S.out.txt (1 hunks)
testdata/format/tcp.pcapng.-t.-S.out.txt (1 hunks)
testdata/format/tcp.pcapng.-t.out.txt (1 hunks)
testdata/format/tcp.pcapng.-tt.-S.out.txt (1 hunks)
testdata/format/tcp.pcapng.-tt.nano.-S.out.txt (1 hunks)
testdata/format/tcp.pcapng.-tt.nano.out.txt (1 hunks)
testdata/format/tcp.pcapng.-tt.out.txt (1 hunks)
testdata/format/tcp.pcapng.-ttt.-S.out.txt (1 hunks)
testdata/format/tcp.pcapng.-ttt.nano.-S.out.txt (1 hunks)
testdata/format/tcp.pcapng.-ttt.nano.out.txt (1 hunks)
testdata/format/tcp.pcapng.-ttt.out.txt (1 hunks)
testdata/format/tcp.pcapng.-tttt.-S.out.txt (1 hunks)
testdata/format/tcp.pcapng.-tttt.nano.-S.out.txt (1 hunks)
testdata/format/tcp.pcapng.-tttt.nano.out.txt (1 hunks)
testdata/format/tcp.pcapng.-tttt.out.txt (1 hunks)
testdata/format/tcp.pcapng.-ttttt.-S.out.txt (1 hunks)
testdata/format/tcp.pcapng.-ttttt.nano.-S.out.txt (1 hunks)
testdata/format/tcp.pcapng.-ttttt.nano.out.txt (1 hunks)
testdata/format/tcp.pcapng.-ttttt.out.txt (1 hunks)
testdata/format/tcp.pcapng.-v.--context.process.-S.out.txt (1 hunks)
testdata/format/tcp.pcapng.-v.--context.process.out.txt (5 hunks)
testdata/format/tcp.pcapng.-v.-S.out.txt (1 hunks)
testdata/format/tcp.pcapng.-v.out.txt (5 hunks)
testdata/format/tcp.pcapng.-x.-S.out.txt (1 hunks)
testdata/format/tcp.pcapng.-x.out.txt (7 hunks)
testdata/format/tcp.pcapng.-xx.-S.out.txt (1 hunks)
testdata/format/tcp.pcapng.-xx.out.txt (7 hunks)
testdata/format/tcp.pcapng.out.txt (1 hunks)
testdata/format/tfo.pcap.-S.out.txt (1 hunks)
testdata/format/tfo.pcap.out.txt (1 hunks)
testdata/format/tls.pcap.-S.out.txt (1 hunks)
testdata/format/tls.pcap.out.txt (1 hunks)
vendor/github.com/x-way/pktdump/.gitignore (1 hunks)
vendor/github.com/x-way/pktdump/format.go (5 hunks)
vendor/github.com/x-way/pktdump/formatter.go (1 hunks)
vendor/github.com/x-way/pktdump/options.go (2 hunks)
vendor/github.com/x-way/pktdump/tcp.go (1 hunks)
vendor/github.com/x-way/pktdump/tcp_sack.go (1 hunks)
vendor/modules.txt (2 hunks)

🧰 Additional context used

📓 Path-based instructions (6)

**/*.go