Re: [PATCH v3 7/7] tests: add blk_dflush() coverage

Hi Denis,

On 2026-07-16T22:33:54, None <[email protected]> wrote:
> tests: add blk_dflush() coverage
>
> Add minimal test invoking new blk_dflush() API.
>
> Signed-off-by: Denis Mukhin <[email protected]>
>
> arch/sandbox/cpu/os.c   | 5 +++++
>  drivers/block/sandbox.c | 9 +++++++++
>  include/os.h            | 8 ++++++++
>  test/dm/host.c          | 2 ++
>  test/dm/mmc.c           | 2 ++
>  5 files changed, 26 insertions(+)

> tests: add blk_dflush() coverage

The subject says tests, but this patch also adds os_fsync() and
implements flush support in the sandbox host block driver. Please can you
split the sandbox bits into its own sandbox: patch, before the tests?

> diff --git a/drivers/block/sandbox.c b/drivers/block/sandbox.c
> @@ -52,9 +52,18 @@ static unsigned long host_block_write(struct udevice *dev,
> +unsigned long host_block_flush(struct udevice *dev)
> +{

This should be static, to match host_block_read() and
host_block_write() - it is only referenced via the ops table.

> diff --git a/arch/sandbox/cpu/os.c b/arch/sandbox/cpu/os.c
> @@ -139,6 +139,11 @@ int os_close(int fd)
> +int os_fsync(int fd)
> +{
> +    return fsync(fd);
> +}

The function comment in os.h says this returns a negative error code,
but fsync() returns -1 with errno set. Since blk_flush() propagates
this value to callers, please can you return -errno on failure so the
caller gets a meaningful error?

> diff --git a/test/dm/host.c b/test/dm/host.c
> @@ -62,6 +62,8 @@ static int dm_test_host(struct unit_test_state *uts)
> +     ut_asserteq(0, blk_dflush(desc));
> +

This covers the direct blk_dflush() path, but nothing exercises the
partition-level disk_blk_flush() path added in patch 2, where flushing
a partition flushes the whole device. Would a test for that be
worthwhile?

Regards,
Simon