Re: [Qemu-devel] [Qemu-block] [PATCH v2 1/3] block: pass bdrv_* methods to bs->file by default

[Stefan Hajnoczi <stefanha@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2973 bytes --]

On Thu, Jun 29, 2017 at 09:43:18PM +0300, Manos Pitsidianakis wrote:
> diff --git a/block.c b/block.c
> index 69439628..9e8d34ad 100644
> --- a/block.c
> +++ b/block.c
> @@ -494,6 +494,8 @@ int bdrv_probe_blocksizes(BlockDriverState *bs, BlockSizes *bsz)
>  
>      if (drv && drv->bdrv_probe_blocksizes) {
>          return drv->bdrv_probe_blocksizes(bs, bsz);
> +    } else if (drv && bs->file && bs->file->bs) {
> +        return bdrv_probe_blocksizes(bs->file->bs, bsz);
>      }
>  
>      return -ENOTSUP;

Currently only raw-format.c and file-posix.c implement
bdrv_probe_blocksizes().  qcow2 will start reporting bs->file's
blocksizes after this patch.

This can lead to a change in blocksizes when live migrating from an old
QEMU to a new QEMU.  Guest operating systems and applications can be
confused if the device suddenly changes beneath them.

On the other hand, it's already possible to hit that today by migrating
from a raw image to a qcow2 image.

I think this change makes sense - we should propagate blocksizes through
the graph - but it may introduce an incompatibility.

Kevin: Do you think this is safe?

> @@ -3406,11 +3410,15 @@ int bdrv_truncate(BdrvChild *child, int64_t offset, Error **errp)
>  
>      assert(child->perm & BLK_PERM_RESIZE);
>  
> +    /* if bs->drv == NULL, bs is closed, so there's nothing to do here */
>      if (!drv) {
>          error_setg(errp, "No medium inserted");
>          return -ENOMEDIUM;
>      }
>      if (!drv->bdrv_truncate) {
> +        if (bs->file && bs->file->bs) {
> +            return bdrv_truncate(bs->file, offset, errp);
> +        }
>          error_setg(errp, "Image format driver does not support resize");
>          return -ENOTSUP;
>      }

This is not safe because existing image formats (e.g. vmdk, dmg) do not
implement .bdrv_truncate().  If we begin truncating the underlying host
file ("foo.vmdk") the disk image will be corrupted.

It is only safe to forward .bdrv_truncate() in filter drivers.  I
suggest providing a default implementation instead:

int bdrv_truncate_file(...);

Then filters can do:

  .bdrv_truncate = bdrv_truncate_file,

> diff --git a/block/io.c b/block/io.c
> index c72d7015..a1aee01d 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -2401,6 +2401,11 @@ int bdrv_co_ioctl(BlockDriverState *bs, int req, void *buf)
>      };
>      BlockAIOCB *acb;
>  
> +    if (drv && !drv->bdrv_aio_ioctl && !drv->bdrv_co_ioctl &&
> +        bs->file && bs->file->bs) {
> +        return bdrv_co_ioctl(bs->file->bs, req, buf);
> +    }
> +
>      bdrv_inc_in_flight(bs);
>      if (!drv || (!drv->bdrv_aio_ioctl && !drv->bdrv_co_ioctl)) {
>          co.ret = -ENOTSUP;

This operation cannot be allowed by default for the same reason as
.bdrv_truncate().  It could change the host file in ways that the format
driver can't handle.  A separate function is needed here:
bdrv_co_ioctl_file().

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]