Re: [Qemu-devel] [PATCH 12/17] block: Convert bdrv_write() to BdrvChild

[Max Reitz]

On 21.06.2016 11:21, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <address@hidden>
> ---
>  block/io.c             |  5 +++--
>  block/qcow.c           | 10 +++++++++-
>  block/qcow2-cluster.c  |  2 +-
>  block/qcow2-refcount.c |  2 +-
>  block/qcow2.c          | 10 +++++++++-
>  block/vdi.c            |  4 ++--
>  block/vvfat.c          |  5 ++---
>  include/block/block.h  |  2 +-
>  8 files changed, 28 insertions(+), 12 deletions(-)

Because I think this is better than saying "I won't give an R-b because
of (1), (2)..." at the bottom:

Review key:
[o] -- completely optional suggestion
[r] -- request, optional, but I'll wait with an R-b until I get feedback
[x] -- requires a fix

[...]

> diff --git a/block/qcow.c b/block/qcow.c
> index e09827b..c80df78 100644
> --- a/block/qcow.c
> +++ b/block/qcow.c
> @@ -969,7 +969,15 @@ static int qcow_write_compressed(BlockDriverState *bs, 
> int64_t sector_num,
>  
>      if (ret != Z_STREAM_END || out_len >= s->cluster_size) {
>          /* could not compress: write normal cluster */
> -        ret = bdrv_write(bs, sector_num, buf, s->cluster_sectors);
> +        QEMUIOVector qiov;
> +        struct iovec iov = {
> +            .iov_base   = (uint8_t*) buf,

[o] ERROR: "(foo*)" should be "(foo *)"

!!11!1elf

> +            .iov_len    = nb_sectors * BDRV_SECTOR_SIZE,
> +        };
> +        qemu_iovec_init_external(&qiov, &iov, 1);
> +
> +        ret = bs->drv->bdrv_co_writev(bs, sector_num, s->cluster_sectors,
> +                                      &qiov);

[r] I'd use nb_sectors instead of s->cluster_sectors (or use
s->cluster_sectors above), I think it should be the same variable in
both places.

[o] Also, but this is a much weaker proposal, I'd have preferred a
direct call to qcow_co_writev(). But maybe that's just me.

>          if (ret < 0) {
>              goto fail;
>          }

[...]

> diff --git a/block/qcow2.c b/block/qcow2.c
> index 4718f82..bc78af3 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -2597,7 +2597,15 @@ static int qcow2_write_compressed(BlockDriverState 
> *bs, int64_t sector_num,
>  
>      if (ret != Z_STREAM_END || out_len >= s->cluster_size) {
>          /* could not compress: write normal cluster */
> -        ret = bdrv_write(bs, sector_num, buf, s->cluster_sectors);
> +        QEMUIOVector qiov;
> +        struct iovec iov = {
> +            .iov_base   = (uint8_t*) buf,

[o] ERROR: "(foo*)" should be "(foo *)"

> +            .iov_len    = nb_sectors * BDRV_SECTOR_SIZE,
> +        };
> +        qemu_iovec_init_external(&qiov, &iov, 1);
> +
> +        ret = bs->drv->bdrv_co_writev(bs, sector_num, s->cluster_sectors,
> +                                      &qiov);

[x] I don't think qcow2 has a bdrv_co_writev().

($ dd if=/dev/urandom of=foo.raw bs=64k count=1
 $ ./qemu-img convert -c -O qcow2 foo.raw foo.qcow2
[1]    11808 segmentation fault (core dumped))

[r] Same as in qcow.c, I'd rather use nb_sectors instead of
s->cluster_sectors.

[o] Same as in qcow.c, I'd call qcow2_co_pwritev() directly. Doing so
would have prevented [x].

Max

>          if (ret < 0) {
>              goto fail;
>          }

signature.asc
Description: OpenPGP digital signature