Fix #2081: Apply zstd compression level reliably across the documente…#2113
Conversation
Dimi1010
left a comment
Dimi1010
left a comment
There was a problem hiding this comment.
The added unit test seems to fail?
MerinoSheep
force-pushed
the
fix/issue-2081-zstd-compression-level
branch
3 times, most recently
from
309afec to
2b806ee
Compare
|
I made some tweak to the test and forgot to run it again ill fix it up. |
| int sizes[3] = { 0, 0, 0 }; | ||
| const int levels[3] = { 1, 5, 10 }; |
| PTF_ASSERT_GREATER_THAN(sizes[0], sizes[1]); | ||
| PTF_ASSERT_GREATER_THAN(sizes[1], sizes[2]); |
There was a problem hiding this comment.
CI is failing for almost all platforms, maybe this is not always true? 🤔
I asked Claude and it suggests checking the window size from the header. Compression levels 1, 5, and 10 should result with different windows sizes
There was a problem hiding this comment.
I took a look at .github/workflows/build_and_test.yml and it seems the only one that passes is the ubuntu container that has zstd configured on. From what I've seen if zstd is not available at run time the writer silently writes uncompressed files with the zstd extension.
There was a problem hiding this comment.
That appears to be because the compression context in LightPcapNG is NULL if its compiled without zst. It effectively performs a silent fallback to uncompressed, since NULL is also used if no compression is requested.
I suppose we could catch that in the constructor or open call of the PcapNg writer.
PR #1962 does add some methods to detect if zst is detected at compile time, tho that one is still open.
There was a problem hiding this comment.
Maybe the test can use the USE_Z_STD macro that is used in LighPcapNg, otherwise it can skip the test:
PTF_TEST_CASE(TestPcapNgZstdCompressionLevels)
{
#ifdef USE_Z_STD
// The test...
#else
PTF_SKIP_TEST("Zstd is not configured");
#endif
}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #2113 +/- ##
=======================================
Coverage 82.69% 82.69%
=======================================
Files 332 332
Lines 59688 59687 -1
Branches 12306 12287 -19
=======================================
+ Hits 49359 49360 +1
+ Misses 9005 8913 -92
- Partials 1324 1414 +90
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
The test compared output sizes for levels 1 vs 10 on a 20KB pcap; on inputs that small, zstd produces identical output for both levels, so the GREATER_THAN assertion always failed. Switch to a larger input (EXAMPLE2_PCAPNG_PATH, ~26KB) and widen the gap to levels 1 vs 22 (zstd's documented max), which produces a clear size delta. Also brace the single-statement while body per review nit.
MerinoSheep
force-pushed
the
fix/issue-2081-zstd-compression-level
branch
from
2744cfa to
6543a42
Compare
Dimi1010
left a comment
Dimi1010
left a comment
There was a problem hiding this comment.
LGTM. Thank you for the effort!
|
@MerinoSheep thank you so much for working on it, much appreciated! 🙏 |
3 participants
Description
Fixes #2081.
The zstd compression level passed to
PcapNgFileWriterDevicewas not beingapplied as documented. Two related defects in
light_zstd_compression.c:Root Cause
ZSTD_CCtx_setParameterwas wrapped inassert(), so the call (alongwith its side effect of actually setting the level) is stripped under
NDEBUG. In Release builds the requested level was never set; zstd fellback to its default (3).
PcapNgFileWriterDevicewas scaledinto the context (
* 2) but the original unscaled value was the onepassed to zstd, and the scaling did not span zstd's 1–22 range.
Proposed Fix
ZSTD_CCtx_setParameterinto a named local so its side effectsurvives
NDEBUG, and check the return value separately.1..10→ zstd3..21, preserve0as the documented"default compression" sentinel, and pass the same scaled value to zstd
that is stored on the context.
Test
Adds
TestPcapNgZstdCompressionLevels: writes the same input pcapng atlevels 1 and 10 and asserts the level-10 file is strictly smaller. With
either bug present the two files are the same size and the assertion
fires.