Re: [PATCH 2/2] test: cmd: Add gzwrite chunk boundary regression test

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