[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH 12/17] block: Convert bdrv_write() to BdrvChild
From: |
Max Reitz |
Subject: |
Re: [Qemu-block] [PATCH 12/17] block: Convert bdrv_write() to BdrvChild |
Date: |
Mon, 27 Jun 2016 15:44:21 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.1.1 |
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
- [Qemu-block] [PATCH 04/17] block: Convert bdrv_co_readv() to BdrvChild, (continued)
- [Qemu-block] [PATCH 04/17] block: Convert bdrv_co_readv() to BdrvChild, Kevin Wolf, 2016/06/21
- [Qemu-block] [PATCH 05/17] block: Convert bdrv_co_writev() to BdrvChild, Kevin Wolf, 2016/06/21
- [Qemu-block] [PATCH 08/17] block: Convert bdrv_co_do_readv/writev to BdrvChild, Kevin Wolf, 2016/06/21
- [Qemu-block] [PATCH 07/17] block: Convert bdrv_aio_writev() to BdrvChild, Kevin Wolf, 2016/06/21
- [Qemu-block] [PATCH 12/17] block: Convert bdrv_write() to BdrvChild, Kevin Wolf, 2016/06/21
- Re: [Qemu-block] [PATCH 12/17] block: Convert bdrv_write() to BdrvChild,
Max Reitz <=
- Re: [Qemu-block] [PATCH 12/17] block: Convert bdrv_write() to BdrvChild, Max Reitz, 2016/06/27
- [Qemu-block] [PATCH v2 12/17] block: Convert bdrv_write() to BdrvChild, Kevin Wolf, 2016/06/29
- Re: [Qemu-block] [PATCH v2 12/17] block: Convert bdrv_write() to BdrvChild, Max Reitz, 2016/06/29
- Re: [Qemu-block] [PATCH v2 12/17] block: Convert bdrv_write() to BdrvChild, Kevin Wolf, 2016/06/29
- Re: [Qemu-block] [PATCH v2 12/17] block: Convert bdrv_write() to BdrvChild, Max Reitz, 2016/06/29
[Qemu-block] [PATCH 10/17] block: Use BlockBackend for I/O in bdrv_commit(), Kevin Wolf, 2016/06/21
[Qemu-block] [PATCH 09/17] block: Move bdrv_commit() to block/commit.c, Kevin Wolf, 2016/06/21
[Qemu-block] [PATCH 06/17] block: Convert bdrv_aio_readv() to BdrvChild, Kevin Wolf, 2016/06/21