Re: [Qemu-block] [PATCH v2] vmdk: align end of file to a sector boundary

[Fam Zheng]

On Thu, 09/13 15:47, yuchenlin wrote:
> On 2018-09-13 10:54, Fam Zheng wrote:
> > On Thu, 09/13 10:31, address@hidden wrote:
> > > From: yuchenlin <address@hidden>
> > > 
> > > There is a rare case which the size of last compressed cluster
> > > is larger than the cluster size, which will cause the file is
> > > not aligned at the sector boundary.
> > 
> > The code looks good to me. Can you also explain why it is important to
> > align
> > file size to sector boundary in the comment?
> > 
> > Fam
> > 
> 
> In my opinion, there are three reasons to do it. First, if vmdk doesn't
> align at the sector boundary, there may be many undefined behaviors, such as
> in vbox it will show VMDK: Compressed image is corrupted '88-disk1.vmdk'
> (VERR_ZIP_CORRUPTED) when we try to import an ova with unaligned vmdk.
> Second, all the cluster_sector is aligned
> to sector, the last one should be like this, too. Third, it ease reading
> with sector based I/Os.
> 
> What do you think?

Yes, it would be nice if you could add this information to the commit message or
the code comment.

Fam

> 
> yuchenlin
> 
> > > 
> > > Signed-off-by: yuchenlin <address@hidden>
> > > ---
> > > v1 -> v2:
> > > * Add more detail comment.
> > > * Add QEMU_ALIGN_UP to show the intention more clearly.
> > > * Add newline in the end of bdrv_truncate.
> > > 
> > > thanks
> > > 
> > >  block/vmdk.c | 21 +++++++++++++++++++++
> > >  1 file changed, 21 insertions(+)
> > > 
> > > diff --git a/block/vmdk.c b/block/vmdk.c
> > > index a9d0084e36..2c9e86d98f 100644
> > > --- a/block/vmdk.c
> > > +++ b/block/vmdk.c
> > > @@ -1698,6 +1698,27 @@ static int coroutine_fn
> > >  vmdk_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset,
> > >                             uint64_t bytes, QEMUIOVector *qiov)
> > >  {
> > > +    if (bytes == 0) {
> > > +        /* The caller will write bytes 0 to signal EOF.
> > > +         * When receive it, we align EOF to a sector boundary. */
> > > +        BDRVVmdkState *s = bs->opaque;
> > > +        int i, ret;
> > > +        int64_t length;
> > > +
> > > +        for (i = 0; i < s->num_extents; i++) {
> > > +            length = bdrv_getlength(s->extents[i].file->bs);
> > > +            if (length < 0) {
> > > +                return length;
> > > +            }
> > > +            length = QEMU_ALIGN_UP(length, BDRV_SECTOR_SIZE);
> > > +            ret = bdrv_truncate(s->extents[i].file, length,
> > > +                                PREALLOC_MODE_OFF, NULL);
> > > +            if (ret < 0) {
> > > +                return ret;
> > > +            }
> > > +        }
> > > +        return 0;
> > > +    }
> > >      return vmdk_co_pwritev(bs, offset, bytes, qiov, 0);
> > >  }
> > > 
> > > --
> > > 2.18.0
> > > 
>