Re: [PATCH 3/3] block/vmdk: add eos marker for stream optimized disks

[Kevin Wolf <kwolf@redhat.com>]

Am 05.10.2026 um 15:33 hat Sanne Raymaekers geschrieben:
> The end of stream marker should be at the very end of a stream optimized
> disk. It consists of a regular marker struct with `val`, `size` and
> `type` set to 0. It is then padded with more 0s to fill out a
> sector. This effectively results in 512 zero bytes.[^1]
> 
> [^1] Virtual Disk Format 5.0 Technical Note.
> 
> Signed-off-by: Sanne Raymaekers <sraymaek@redhat.com>
> ---
>  block/vmdk.c | 29 +++++++++++++++++++++++------
>  1 file changed, 23 insertions(+), 6 deletions(-)
> 
> diff --git a/block/vmdk.c b/block/vmdk.c
> index 040f38cd1d..c10321146e 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -195,6 +195,13 @@ typedef struct VmdkGrainMarker {
>      uint8_t  data[];
>  } QEMU_PACKED VmdkGrainMarker;
>  
> +typedef struct EosMarker {
> +   uint64_t val;
> +   uint32_t size;
> +   uint32_t type;
> +   uint8_t pad[512 - 16];
> +} QEMU_PACKED EosMarker;
> +
>  enum {
>      MARKER_END_OF_STREAM    = 0,
>      MARKER_GRAIN_TABLE      = 1,
> @@ -987,12 +994,7 @@ vmdk_open_vmdk4(BlockDriverState *bs, BdrvChild *file, int flags,
>              VMDK4Header header;
>              uint8_t pad[512 - 4 - sizeof(VMDK4Header)];
>  
> -            struct {
> -                uint64_t val;
> -                uint32_t size;
> -                uint32_t type;
> -                uint8_t pad[512 - 16];
> -            } QEMU_PACKED eos_marker;
> +            EosMarker eos_marker;
>          } QEMU_PACKED footer;
>  
>          ret = bdrv_pread(file, bs->file->bs->total_sectors * 512 - 1536,
> @@ -2180,6 +2182,7 @@ vmdk_co_pwritev_compressed(BlockDriverState *bs, int64_t offset, int64_t bytes,
>          BDRVVmdkState *s = bs->opaque;
>          int i, ret;
>          int64_t length;
> +        EosMarker *eos_marker = NULL;
>  
>          for (i = 0; i < s->num_extents; i++) {
>              length = bdrv_co_getlength(s->extents[i].file->bs);
> @@ -2193,6 +2196,20 @@ vmdk_co_pwritev_compressed(BlockDriverState *bs, int64_t offset, int64_t bytes,
>                  return ret;
>              }
>          }

Is there any way s->num_extents could not be 1 for streamOptimized
images?

If not, I think you can delete the whole existing for loop (which will
execute only once). bdrv_co_truncate() extends the file size, but then
you write at a higher offset below, which will extend it a second time,
so the bdrv_co_truncate() call didn't do anything useful.

> +        /* Stream optimized disks end with an EOS marker, consisting of 512 null bytes. */
> +        if (strcmp(s->create_type, "streamOptimized") == 0) {

This is vmdk_co_pwritev_compressed(). If we're here, we already know
that this is a streamOptimized image.

> +            eos_marker = g_malloc(sizeof(EosMarker));
> +            memset(eos_marker, 0, sizeof(EosMarker));

This can be combined into g_malloc0().

In fact, using qemu_blockalign0() and declaring eos_marker as
QEMU_AUTO_VFREE would be even better because it makes sure that the
buffer has the required memory alignment; otherwise, a lower layer may
have to copy the data into a bounce buffer.

> +            length = bdrv_co_getlength(s->extents[s->num_extents - 1].file->bs);

Unless I'm mistaken above, this is always s->extents[0].

bdrv_co_getlength() errors must be handled, like in the for loop above.

> +            ret = bdrv_co_pwrite(s->extents[s->num_extents - 1].file, length,
> +                                 sizeof(EosMarker), eos_marker, 0);

When removing the truncate, length would become QEMU_ALIGN_UP(length,
512) here instead.

> +            g_free(eos_marker);
> +            if (ret < 0) {
> +                return ret;
> +            }
> +        }
> +
>          return 0;
>      }
>      return vmdk_co_pwritev(bs, offset, bytes, qiov, 0);

Kevin