-
Notifications
You must be signed in to change notification settings - Fork 856
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
Fix --time-stamp-precision when used with -V #1078
base: master
Are you sure you want to change the base?
Conversation
c98114e
to
a478f28
Compare
Capture files were opened in two different bits of code. Only the first bit of code acquired support for --time-stamp-precision, so the 2nd and subsequent files opened with -V would always have the default timestamp precision. This fix unifies file opening into a single routine, ensuring consistent behavior for each opened file.
a478f28
to
87501b7
Compare
Thanks for setting up a rigorous CI system - it found a few issues in my original patch. I believe the remaining CI failures are not related to my payload. win32 failed with
which is pretty clearly a transient issue. netbsd-mips64 failed with
which also doesn't seem to be payload-related, although I can't really rule it out given my unfamiliarity with the codebase. Is this a known flaky CI leg? Could you try re-running it? (I don't have permissions to re-run.) As you might have noticed if you follow all the commit history here, I had to do some deeper edits to the test harness than I'd originally anticipated. Unfortunately, the existing harness was somewhat hostile to exercising the This ends up with some clumsy code for ultimately just one test case, so I'm not super thrilled with that. But it also is really the only way to exercise Let me know if you have any suggestions on how I can exercise Assuming the CI re-runs don't turn up any surprises, I'm ready for discussion & review of this proposed change. |
Yes. AppVeyor is prone to those sorts of transient issues; I restarted that build with "rebuild the failed build and continue from there". (I tried switching to Cirrus CI, but they don't have any pre-packaged containers complete with Visual Studio, and I wasn't going to have each build spend a long time downloading VS, so I gave up on that.)
Yeah, whatever program is writing those messages should at least use
Extremely unlikely to be related to anything done to tcpdump, as it's only constructing a source tarball there, not running tcpdump.
I think it's run out of disk space in the past.
I don't know what account I'd need to log in. @infrastation, could you poke this? |
It looks like the CI passed on re-run. Is there anything else needed on my side to move this PR forward? Thanks ;) |
tcpdump does the wrong thing if you combine
--time-stamp-precision
with-V
to open multiple input files. As a trivial example, observe that printing the exact same 1-line file twice gives different timestamps:The problem is that when
-V
was added, the code to open the 2nd and subsequent files was copy-pasted into the inductive part of a loop. When timestamp support was added, the patch was only applied to the first copy of the code, which is no doubt an oversight.I figured I'd fix this and prevent future similar problems by unifying the two code clones for opening files, so I created a new helper routine
open_pcap_file
to hold the common code.As a minor side effect, there is a behavior change around error handling, which may or not be desired. Previously, if the 2nd file had a different link type, tcpdump would bail out early with
"$filename: new dlt does not match original"
. Now we'll get a little further printing the file banner"reading from file $filename, link-type $type, snapshot length $length"
before giving up. This was always an error case, so I don't imagine it's a big deal if the sequence of messages is different.I didn't see a clean way to test this using the existing harness, so I added a minor hack to TESTrun: if
$options
appears to contain-V
, then use that instead of the usual-r
switch. I added a new test that exhibits the bug; after the fix, the test passes cleanly.I've attempted to match the local code philosophy and style, but if I've overlooked anything, please let me know; I'm happy to improve the patch.