Hi Simon, Thank you for the review. All three points are addressed in v2, which I will post shortly.
On Thu, Aug 6, 2026 at 9:20 PM Simon Glass <[email protected]> wrote: > > Hi Aristo, > > On 2026-08-06T10:36:59, Aristo Chen <[email protected]> wrote: > > test: cmd: Add gzwrite chunk boundary regression test > > > > Add a deterministic regression test for the gzwrite() case where a > > decompression input chunk is exhausted at exactly the same time as > > the write buffer fills up. Build a gzip file by hand from two 1 KiB > > stored deflate blocks and pick a chunk size that covers exactly the > > first block header plus its payload, so that with a 1 KiB write > > buffer the first input chunk runs out precisely when the write buffer > > is full. > > > > Unlike the existing random data test, which only hits this corner > > case for rare byte patterns (about 1 percent of runs on sandbox64), > > this test fails 20 out of 20 runs without the preceding gunzip fix: > > > > Error: inflate() returned -5 > > > > and passed 100 out of 100 runs with it. > > > > Signed-off-by: Aristo Chen <[email protected]> > > > > test/cmd/unzip.c | 103 > > +++++++++++++++++++++++++++++++++++++++++++++++++++++-- > > 1 file changed, 101 insertions(+), 2 deletions(-) > > > diff --git a/test/cmd/unzip.c b/test/cmd/unzip.c > > @@ -132,3 +140,94 @@ static int dm_test_cmd_zip_gzwrite(struct > > unit_test_state *uts) > > + /* Input chunk: exactly one stored block header plus its payload */ > > + env_set_ulong("gzwrite_chunk", 5 + blk); > > + ut_assertok(run_commandf("gzwrite mmc 9 %lx %zx %zx", loadaddr, > > + gzlen, blk)); > > + ut_assert_skip_to_line("\t%zu bytes, crc 0x%08x", rawsize, crc); > > + env_set("gzwrite_chunk", NULL); > > We should have assrrts on the env_set...() functions too. > > If any earlier ut_assert*() fires between the env_set_ulong() and the > env_set(..., NULL), gzwrite_chunk leaks into subsequent tests. Ideally > we would put this code into its own function which does the core part, > then call ut_assertok() on a function with the test code. I'm not sure > how much this matters, though. Done in v2. The test body now lives in a helper and the outer test function owns the env_set_ulong()/env_set() pair, both asserted, so gzwrite_chunk is unset even when an assertion inside the helper fails. The pre-existing dm_test_cmd_zip_gzwrite has the same pattern, by the way: it sets gzwrite_chunk in a loop, does not assert on env_set_ulong(), and leaves the last value set when the test ends or an assertion fires. I left it alone to keep this series small; happy to send a follow-up cleaning that up the same way if you think it is worth it. > > > diff --git a/test/cmd/unzip.c b/test/cmd/unzip.c > > @@ -132,3 +140,94 @@ static int dm_test_cmd_zip_gzwrite(struct > > unit_test_state *uts) > > + const size_t blk = SZ_1K; > > + const size_t rawsize = 2 * blk; > > ... > > + u8 raw[2 * SZ_1K]; > > Please use rawsize (or sizeof(raw)) here rather than repeating 2 * > SZ_1K. Same for the 5 + blk chunk size - a named constant for the > stored-block header would make the intent clearer than the bare 5. Done in v2, as STORED_BLK_HDR_LEN and STORED_BLK_LEN, with rawsize derived from sizeof(raw). I kept the array bound as a macro expression rather than a const variable: in C a const size_t is not a constant expression, so raw[rawsize] would formally be a variable length array, which kernel style discourages and make W=3 flags via -Wvla. > > > diff --git a/test/cmd/unzip.c b/test/cmd/unzip.c > > @@ -132,3 +140,94 @@ static int dm_test_cmd_zip_gzwrite(struct > > unit_test_state *uts) > > + ut_assertok(run_commandf("mmc read %lx 0 %zx", decaddr, > > + rawsize / 512)); > > + ut_assert_nextline("MMC read: dev # 9, block # 0, count %zu ... %zu > > blocks read: OK", > > + rawsize / 512, rawsize / 512); > > Since you already have mmc_dev_desc, using mmc_dev_desc->blksz would > be more robust than the literal 512. Done in v2. > > Regards, > Simon Regards, Aristo
