On 27.06.2016 15:44, Max Reitz wrote:
> On 21.06.2016 11:21, Kevin Wolf wrote:
>> Signed-off-by: Kevin Wolf <kw...@redhat.com>
>> ---
>> 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.
[x] qcow_write_compressed() is not a coroutine_fn, resulting in:
qemu-img: ./util/qemu-coroutine-lock.c:143: qemu_co_mutex_unlock:
Assertion `qemu_in_coroutine()' failed.
>> 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].
[x] Same as in qcow.c, qcow2_write_compressed() is not a coroutine_fn
either.
> Max
>
>> if (ret < 0) {
>> goto fail;
>> }
>
signature.asc
Description: OpenPGP digital signature
