Hi Denis,
On 2026-06-05T03:38:59, None <[email protected]> wrote:
> test: disk: coverage for {read,write}_disk_flags
>
> Add unit test coverage for GPT flags functions.
>
> To run the test:
> truncate -s 16M mmc1.img
> ./test/py/test.py --bd sandbox --build -k test_gpt_flags -v
>
> Signed-off-by: Denis Mukhin <[email protected]>
>
> disk/part_efi.c | 12 +++--
> test/dm/Makefile | 2 +-
> test/dm/gpt-flags.c | 148
> ++++++++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 156 insertions(+), 6 deletions(-)
> diff --git a/disk/part_efi.c b/disk/part_efi.c
> @@ -47,17 +47,19 @@ static inline u32 efi_crc32(const void *buf, u32 len)
> return crc32(0, buf, len);
> }
>
> +/* Public for unit tests. */
> +int is_gpt_valid(struct blk_desc *desc, u64 lba, gpt_header *pgpt_head,
> + gpt_entry **pgpt_pte);
> +int is_pte_valid(gpt_entry * pte);
> +
Please don't expose these by re-declaring the prototypes in two places
(here and in test/dm/gpt-flags.c). Either put the prototypes in a
private header that only the test pulls in, or rework the test so it
goes through the public API. The "Public for unit tests" comment plus
a duplicated extern declaration in the test file is fragile - if the
signature ever changes, the test silently drifts.
> diff --git a/test/dm/gpt-flags.c b/test/dm/gpt-flags.c
> @@ -0,0 +1,148 @@
> +static int mock_blk_desc(struct unit_test_state *uts, struct blk_desc **desc)
> +{
> + struct udevice *dev;
> + char str_disk_guid[UUID_STR_LEN + 1];
> + struct disk_partition parts[2] = {
> + {
> + .start = TEST_LBA_COUNT,
> + .size = 1,
> + .name = 'test1',
> + },
> + {
> + .start = 49,
> + .size = 1,
> + .name = 'test2',
> + },
> + };
> +
> + ut_assertok(uclass_get_device_by_name(UCLASS_BLK, 'mmc1.blk', &dev));
> + *desc = (struct blk_desc *)dev_get_uclass_plat(dev);
> +
> + if (CONFIG_IS_ENABLED(RANDOM_UUID)) {
> + gen_rand_uuid_str(parts[0].uuid, UUID_STR_FORMAT_STD);
> + gen_rand_uuid_str(parts[1].uuid, UUID_STR_FORMAT_STD);
> + gen_rand_uuid_str(str_disk_guid, UUID_STR_FORMAT_STD);
> + }
> + ut_assertok(gpt_restore(*desc, str_disk_guid, parts,
> + ARRAY_SIZE(parts)));
When CONFIG_RANDOM_UUID is not enabled, str_disk_guid is an
uninitialised stack buffer and parts[].uuid is empty, so gpt_restore()
calls uuid_str_to_bin() on garbage and fails. Please use a fixed GUID
string (or assert the dependency on RANDOM_UUID) - silently relying on
the sandbox defconfig is brittle.
> diff --git a/test/dm/gpt-flags.c b/test/dm/gpt-flags.c
> @@ -0,0 +1,148 @@
> +DM_TEST(test_gpt_flags_default, UTF_SCAN_FDT);
DM_TEST registers these under the dm suite (I am assuming DM is
necessary?), and the convention is to prefix function names with
dm_test_. Please rename to dm_test_gpt_flags_default etc - that is
also what 'ut dm gpt_flags_default' will look for. Please give
test_disk_flags_invalid_partnum the same prefix for consistency.
> diff --git a/test/dm/gpt-flags.c b/test/dm/gpt-flags.c
> @@ -0,0 +1,148 @@
> +static int test_gpt_flags_headers(struct unit_test_state *uts)
> +{
> + struct blk_desc *desc;
> + u16 flags_in = 0x0111;
> +
> + ut_assertok(mock_blk_desc(uts, &desc));
> + ut_assertok(write_disk_flags(desc, TEST_PARTNUM, flags_in));
> +
> + {
> + ALLOC_CACHE_ALIGN_BUFFER_PAD(gpt_header, gpt_h1, 1,
> desc->blksz);
> + ALLOC_CACHE_ALIGN_BUFFER_PAD(gpt_header, gpt_h2, 1,
> desc->blksz);
The nested { ... } block is awkward - please put the buffers and
locals to the top of the function.
> diff --git a/test/dm/gpt-flags.c b/test/dm/gpt-flags.c
> @@ -0,0 +1,148 @@
> + * To run the test:
> + * truncate -s 16M mmc1.img
> + * ./test/py/test.py --bd sandbox --build -k test_gpt_flags -v
This belongs as automatic setup in test/py/tests/test_ut.py (see
setup_fedora_image, setup_rauc_image, etc), not as manual instructions
in the commit message. Otherwise CI won't run it and people reading
the test years from now won't know where the image came from. Please
wire it into test_ut_dm_init_bootstd() so 'ut dm gpt_flags_*' just
works.
> diff --git a/test/dm/gpt-flags.c b/test/dm/gpt-flags.c
> @@ -0,0 +1,148 @@
> +static int test_disk_flags_invalid_partnum(struct unit_test_state *uts)
> +{
> + struct blk_desc *desc;
> + u16 flags;
> +
> + ut_assertok(mock_blk_desc(uts, &desc));
> + ut_asserteq(-EINVAL, read_disk_flags(desc, 0, &flags));
> + ut_asserteq(-EINVAL, write_disk_flags(desc, 0, 0));
> + ut_asserteq(-EINVAL, write_disk_flags(desc, 255, 0));
Missing a few cases: negative partnum, a partnum just past
num_partition_entries (which returns -EPERM from read_disk_flags() and
-EINVAL from write_disk_flags() = different return codes, worth a
separate comment on patch 2), and read_disk_flags() with the high
partnum. Also please test with a NULL flags pointer if
read_disk_flags() is meant to reject it.
Regards,
Simon
