| Message ID | 20170807203007.19033-3-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 08/07/2017 05:30 PM, Eric Blake wrote: > This also requires changing the return type of get_cluster_offset() > and adjusting all callers. > > Use osdep.h macros instead of open-coded rounding while in the > area. > > Reported-by: Markus Armbruster <armbru@redhat.com> > Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > block/qcow.c | 64 ++++++++++++++++++++++++++++++++++++++++++------------------ > 1 file changed, 45 insertions(+), 19 deletions(-) > > diff --git a/block/qcow.c b/block/qcow.c > index c08cdc4a7b..937023d447 100644 > --- a/block/qcow.c > +++ b/block/qcow.c > @@ -347,16 +347,17 @@ static int qcow_reopen_prepare(BDRVReopenState *state, > * 'compressed_size'. 'compressed_size' must be > 0 and < > * cluster_size > * > - * return 0 if not allocated. > + * return 0 if not allocated, -errno on failure. > */ > -static uint64_t get_cluster_offset(BlockDriverState *bs, > - uint64_t offset, int allocate, > - int compressed_size, > - int n_start, int n_end) > +static int64_t get_cluster_offset(BlockDriverState *bs, > + uint64_t offset, int allocate, > + int compressed_size, > + int n_start, int n_end) > { > BDRVQcowState *s = bs->opaque; > int min_index, i, j, l1_index, l2_index; > - uint64_t l2_offset, *l2_table, cluster_offset, tmp; > + int64_t l2_offset, cluster_offset; > + uint64_t *l2_table, tmp; > uint32_t min_count; > int new_l2_table; > > @@ -368,8 +369,11 @@ static uint64_t get_cluster_offset(BlockDriverState *bs, > return 0; > /* allocate a new l2 entry */ > l2_offset = bdrv_getlength(bs->file->bs); > + if (l2_offset < 0) { > + return l2_offset; > + } > /* round to cluster size */ > - l2_offset = (l2_offset + s->cluster_size - 1) & ~(s->cluster_size - 1); > + l2_offset = QEMU_ALIGN_UP(l2_offset, s->cluster_size); > /* update the L1 entry */ > s->l1_table[l1_index] = l2_offset; > tmp = cpu_to_be64(l2_offset); > @@ -430,8 +434,10 @@ static uint64_t get_cluster_offset(BlockDriverState *bs, > if (decompress_cluster(bs, cluster_offset) < 0) > return 0; > cluster_offset = bdrv_getlength(bs->file->bs); > - cluster_offset = (cluster_offset + s->cluster_size - 1) & > - ~(s->cluster_size - 1); > + if (cluster_offset < 0) { > + return cluster_offset; > + } > + cluster_offset = QEMU_ALIGN_UP(cluster_offset, s->cluster_size); > /* write the cluster content */ > if (bdrv_pwrite(bs->file, cluster_offset, s->cluster_cache, > s->cluster_size) != > @@ -439,12 +445,19 @@ static uint64_t get_cluster_offset(BlockDriverState *bs, > return -1; > } else { > cluster_offset = bdrv_getlength(bs->file->bs); > + if (cluster_offset < 0) { > + return cluster_offset; > + } > if (allocate == 1) { > + int ret; > + > /* round to cluster size */ > - cluster_offset = (cluster_offset + s->cluster_size - 1) & > - ~(s->cluster_size - 1); > - bdrv_truncate(bs->file, cluster_offset + s->cluster_size, > - PREALLOC_MODE_OFF, NULL); > + cluster_offset = QEMU_ALIGN_UP(cluster_offset, s->cluster_size); > + ret = bdrv_truncate(bs->file, cluster_offset + s->cluster_size, > + PREALLOC_MODE_OFF, NULL); > + if (ret < 0) { > + return ret; > + } > /* if encrypted, we must initialize the cluster > content which won't be written */ > if (bs->encrypted && > @@ -491,11 +504,14 @@ static int64_t coroutine_fn qcow_co_get_block_status(BlockDriverState *bs, > { > BDRVQcowState *s = bs->opaque; > int index_in_cluster, n; > - uint64_t cluster_offset; > + int64_t cluster_offset; > > qemu_co_mutex_lock(&s->lock); > cluster_offset = get_cluster_offset(bs, sector_num << 9, 0, 0, 0, 0); > qemu_co_mutex_unlock(&s->lock); > + if (cluster_offset < 0) { > + return cluster_offset; > + } > index_in_cluster = sector_num & (s->cluster_sectors - 1); > n = s->cluster_sectors - index_in_cluster; > if (n > nb_sectors) > @@ -567,7 +583,7 @@ static coroutine_fn int qcow_co_readv(BlockDriverState *bs, int64_t sector_num, > BDRVQcowState *s = bs->opaque; > int index_in_cluster; > int ret = 0, n; > - uint64_t cluster_offset; > + int64_t cluster_offset; > struct iovec hd_iov; > QEMUIOVector hd_qiov; > uint8_t *buf; > @@ -588,8 +604,10 @@ static coroutine_fn int qcow_co_readv(BlockDriverState *bs, int64_t sector_num, > > while (nb_sectors != 0) { > /* prepare next request */ > - cluster_offset = get_cluster_offset(bs, sector_num << 9, > - 0, 0, 0, 0); > + cluster_offset = get_cluster_offset(bs, sector_num << 9, 0, 0, 0, 0); > + if (cluster_offset < 0) { > + return cluster_offset; > + } > index_in_cluster = sector_num & (s->cluster_sectors - 1); > n = s->cluster_sectors - index_in_cluster; > if (n > nb_sectors) { > @@ -670,7 +688,7 @@ static coroutine_fn int qcow_co_writev(BlockDriverState *bs, int64_t sector_num, > { > BDRVQcowState *s = bs->opaque; > int index_in_cluster; > - uint64_t cluster_offset; > + int64_t cluster_offset; > int ret = 0, n; > struct iovec hd_iov; > QEMUIOVector hd_qiov; > @@ -704,6 +722,10 @@ static coroutine_fn int qcow_co_writev(BlockDriverState *bs, int64_t sector_num, > cluster_offset = get_cluster_offset(bs, sector_num << 9, 1, 0, > index_in_cluster, > index_in_cluster + n); > + if (cluster_offset < 0) { > + ret = cluster_offset; > + break; > + } > if (!cluster_offset || (cluster_offset & 511) != 0) { > ret = -EIO; > break; > @@ -949,7 +971,7 @@ qcow_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset, > z_stream strm; > int ret, out_len; > uint8_t *buf, *out_buf; > - uint64_t cluster_offset; > + int64_t cluster_offset; > > buf = qemu_blockalign(bs, s->cluster_size); > if (bytes != s->cluster_size) { > @@ -1003,6 +1025,10 @@ qcow_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset, > qemu_co_mutex_lock(&s->lock); > cluster_offset = get_cluster_offset(bs, offset, 2, out_len, 0, 0); > qemu_co_mutex_unlock(&s->lock); > + if (cluster_offset < 0) { > + ret = cluster_offset; > + goto fail; > + } > if (cluster_offset == 0) { > ret = -EIO; > goto fail; >
On Mon, Aug 07, 2017 at 03:30:05PM -0500, Eric Blake wrote: > This also requires changing the return type of get_cluster_offset() > and adjusting all callers. > > Use osdep.h macros instead of open-coded rounding while in the > area. > > Reported-by: Markus Armbruster <armbru@redhat.com> > Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: Jeff Cody <jcody@redhat.com> > --- > block/qcow.c | 64 ++++++++++++++++++++++++++++++++++++++++++------------------ > 1 file changed, 45 insertions(+), 19 deletions(-) > > diff --git a/block/qcow.c b/block/qcow.c > index c08cdc4a7b..937023d447 100644 > --- a/block/qcow.c > +++ b/block/qcow.c > @@ -347,16 +347,17 @@ static int qcow_reopen_prepare(BDRVReopenState *state, > * 'compressed_size'. 'compressed_size' must be > 0 and < > * cluster_size > * > - * return 0 if not allocated. > + * return 0 if not allocated, -errno on failure. > */ > -static uint64_t get_cluster_offset(BlockDriverState *bs, > - uint64_t offset, int allocate, > - int compressed_size, > - int n_start, int n_end) > +static int64_t get_cluster_offset(BlockDriverState *bs, > + uint64_t offset, int allocate, > + int compressed_size, > + int n_start, int n_end) > { > BDRVQcowState *s = bs->opaque; > int min_index, i, j, l1_index, l2_index; > - uint64_t l2_offset, *l2_table, cluster_offset, tmp; > + int64_t l2_offset, cluster_offset; > + uint64_t *l2_table, tmp; > uint32_t min_count; > int new_l2_table; > > @@ -368,8 +369,11 @@ static uint64_t get_cluster_offset(BlockDriverState *bs, > return 0; > /* allocate a new l2 entry */ > l2_offset = bdrv_getlength(bs->file->bs); > + if (l2_offset < 0) { > + return l2_offset; > + } > /* round to cluster size */ > - l2_offset = (l2_offset + s->cluster_size - 1) & ~(s->cluster_size - 1); > + l2_offset = QEMU_ALIGN_UP(l2_offset, s->cluster_size); > /* update the L1 entry */ > s->l1_table[l1_index] = l2_offset; > tmp = cpu_to_be64(l2_offset); > @@ -430,8 +434,10 @@ static uint64_t get_cluster_offset(BlockDriverState *bs, > if (decompress_cluster(bs, cluster_offset) < 0) > return 0; > cluster_offset = bdrv_getlength(bs->file->bs); > - cluster_offset = (cluster_offset + s->cluster_size - 1) & > - ~(s->cluster_size - 1); > + if (cluster_offset < 0) { > + return cluster_offset; > + } > + cluster_offset = QEMU_ALIGN_UP(cluster_offset, s->cluster_size); > /* write the cluster content */ > if (bdrv_pwrite(bs->file, cluster_offset, s->cluster_cache, > s->cluster_size) != > @@ -439,12 +445,19 @@ static uint64_t get_cluster_offset(BlockDriverState *bs, > return -1; > } else { > cluster_offset = bdrv_getlength(bs->file->bs); > + if (cluster_offset < 0) { > + return cluster_offset; > + } > if (allocate == 1) { > + int ret; > + > /* round to cluster size */ > - cluster_offset = (cluster_offset + s->cluster_size - 1) & > - ~(s->cluster_size - 1); > - bdrv_truncate(bs->file, cluster_offset + s->cluster_size, > - PREALLOC_MODE_OFF, NULL); > + cluster_offset = QEMU_ALIGN_UP(cluster_offset, s->cluster_size); > + ret = bdrv_truncate(bs->file, cluster_offset + s->cluster_size, > + PREALLOC_MODE_OFF, NULL); > + if (ret < 0) { > + return ret; > + } > /* if encrypted, we must initialize the cluster > content which won't be written */ > if (bs->encrypted && > @@ -491,11 +504,14 @@ static int64_t coroutine_fn qcow_co_get_block_status(BlockDriverState *bs, > { > BDRVQcowState *s = bs->opaque; > int index_in_cluster, n; > - uint64_t cluster_offset; > + int64_t cluster_offset; > > qemu_co_mutex_lock(&s->lock); > cluster_offset = get_cluster_offset(bs, sector_num << 9, 0, 0, 0, 0); > qemu_co_mutex_unlock(&s->lock); > + if (cluster_offset < 0) { > + return cluster_offset; > + } > index_in_cluster = sector_num & (s->cluster_sectors - 1); > n = s->cluster_sectors - index_in_cluster; > if (n > nb_sectors) > @@ -567,7 +583,7 @@ static coroutine_fn int qcow_co_readv(BlockDriverState *bs, int64_t sector_num, > BDRVQcowState *s = bs->opaque; > int index_in_cluster; > int ret = 0, n; > - uint64_t cluster_offset; > + int64_t cluster_offset; > struct iovec hd_iov; > QEMUIOVector hd_qiov; > uint8_t *buf; > @@ -588,8 +604,10 @@ static coroutine_fn int qcow_co_readv(BlockDriverState *bs, int64_t sector_num, > > while (nb_sectors != 0) { > /* prepare next request */ > - cluster_offset = get_cluster_offset(bs, sector_num << 9, > - 0, 0, 0, 0); > + cluster_offset = get_cluster_offset(bs, sector_num << 9, 0, 0, 0, 0); > + if (cluster_offset < 0) { > + return cluster_offset; > + } > index_in_cluster = sector_num & (s->cluster_sectors - 1); > n = s->cluster_sectors - index_in_cluster; > if (n > nb_sectors) { > @@ -670,7 +688,7 @@ static coroutine_fn int qcow_co_writev(BlockDriverState *bs, int64_t sector_num, > { > BDRVQcowState *s = bs->opaque; > int index_in_cluster; > - uint64_t cluster_offset; > + int64_t cluster_offset; > int ret = 0, n; > struct iovec hd_iov; > QEMUIOVector hd_qiov; > @@ -704,6 +722,10 @@ static coroutine_fn int qcow_co_writev(BlockDriverState *bs, int64_t sector_num, > cluster_offset = get_cluster_offset(bs, sector_num << 9, 1, 0, > index_in_cluster, > index_in_cluster + n); > + if (cluster_offset < 0) { > + ret = cluster_offset; > + break; > + } > if (!cluster_offset || (cluster_offset & 511) != 0) { > ret = -EIO; > break; > @@ -949,7 +971,7 @@ qcow_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset, > z_stream strm; > int ret, out_len; > uint8_t *buf, *out_buf; > - uint64_t cluster_offset; > + int64_t cluster_offset; > > buf = qemu_blockalign(bs, s->cluster_size); > if (bytes != s->cluster_size) { > @@ -1003,6 +1025,10 @@ qcow_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset, > qemu_co_mutex_lock(&s->lock); > cluster_offset = get_cluster_offset(bs, offset, 2, out_len, 0, 0); > qemu_co_mutex_unlock(&s->lock); > + if (cluster_offset < 0) { > + ret = cluster_offset; > + goto fail; > + } > if (cluster_offset == 0) { > ret = -EIO; > goto fail; > -- > 2.13.4 > >
On Mon, Aug 07, 2017 at 03:30:05PM -0500, Eric Blake wrote: > This also requires changing the return type of get_cluster_offset() > and adjusting all callers. > > Use osdep.h macros instead of open-coded rounding while in the > area. > > Reported-by: Markus Armbruster <armbru@redhat.com> > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > block/qcow.c | 64 ++++++++++++++++++++++++++++++++++++++++++------------------ > 1 file changed, 45 insertions(+), 19 deletions(-) > > diff --git a/block/qcow.c b/block/qcow.c > index c08cdc4a7b..937023d447 100644 > --- a/block/qcow.c > +++ b/block/qcow.c > @@ -347,16 +347,17 @@ static int qcow_reopen_prepare(BDRVReopenState *state, > * 'compressed_size'. 'compressed_size' must be > 0 and < > * cluster_size > * > - * return 0 if not allocated. > + * return 0 if not allocated, -errno on failure. > */ > -static uint64_t get_cluster_offset(BlockDriverState *bs, > - uint64_t offset, int allocate, > - int compressed_size, > - int n_start, int n_end) > +static int64_t get_cluster_offset(BlockDriverState *bs, > + uint64_t offset, int allocate, > + int compressed_size, > + int n_start, int n_end) > { > BDRVQcowState *s = bs->opaque; > int min_index, i, j, l1_index, l2_index; > - uint64_t l2_offset, *l2_table, cluster_offset, tmp; > + int64_t l2_offset, cluster_offset; > + uint64_t *l2_table, tmp; > uint32_t min_count; > int new_l2_table; > > @@ -368,8 +369,11 @@ static uint64_t get_cluster_offset(BlockDriverState *bs, > return 0; > /* allocate a new l2 entry */ > l2_offset = bdrv_getlength(bs->file->bs); > + if (l2_offset < 0) { > + return l2_offset; > + } > /* round to cluster size */ > - l2_offset = (l2_offset + s->cluster_size - 1) & ~(s->cluster_size - 1); > + l2_offset = QEMU_ALIGN_UP(l2_offset, s->cluster_size); > /* update the L1 entry */ > s->l1_table[l1_index] = l2_offset; > tmp = cpu_to_be64(l2_offset); > @@ -430,8 +434,10 @@ static uint64_t get_cluster_offset(BlockDriverState *bs, > if (decompress_cluster(bs, cluster_offset) < 0) > return 0; > cluster_offset = bdrv_getlength(bs->file->bs); > - cluster_offset = (cluster_offset + s->cluster_size - 1) & > - ~(s->cluster_size - 1); > + if (cluster_offset < 0) { > + return cluster_offset; > + } > + cluster_offset = QEMU_ALIGN_UP(cluster_offset, s->cluster_size); > /* write the cluster content */ > if (bdrv_pwrite(bs->file, cluster_offset, s->cluster_cache, > s->cluster_size) != > @@ -439,12 +445,19 @@ static uint64_t get_cluster_offset(BlockDriverState *bs, > return -1; > } else { > cluster_offset = bdrv_getlength(bs->file->bs); > + if (cluster_offset < 0) { > + return cluster_offset; > + } > if (allocate == 1) { > + int ret; > + > /* round to cluster size */ > - cluster_offset = (cluster_offset + s->cluster_size - 1) & > - ~(s->cluster_size - 1); > - bdrv_truncate(bs->file, cluster_offset + s->cluster_size, > - PREALLOC_MODE_OFF, NULL); > + cluster_offset = QEMU_ALIGN_UP(cluster_offset, s->cluster_size); > + ret = bdrv_truncate(bs->file, cluster_offset + s->cluster_size, > + PREALLOC_MODE_OFF, NULL); My r-b stands, but do you think it is worth checking for (cluster_offset + s->cluster_size) > INT64_MAX? > + if (ret < 0) { > + return ret; > + } > /* if encrypted, we must initialize the cluster > content which won't be written */ > if (bs->encrypted && > @@ -491,11 +504,14 @@ static int64_t coroutine_fn qcow_co_get_block_status(BlockDriverState *bs, > { > BDRVQcowState *s = bs->opaque; > int index_in_cluster, n; > - uint64_t cluster_offset; > + int64_t cluster_offset; > > qemu_co_mutex_lock(&s->lock); > cluster_offset = get_cluster_offset(bs, sector_num << 9, 0, 0, 0, 0); > qemu_co_mutex_unlock(&s->lock); > + if (cluster_offset < 0) { > + return cluster_offset; > + } > index_in_cluster = sector_num & (s->cluster_sectors - 1); > n = s->cluster_sectors - index_in_cluster; > if (n > nb_sectors) > @@ -567,7 +583,7 @@ static coroutine_fn int qcow_co_readv(BlockDriverState *bs, int64_t sector_num, > BDRVQcowState *s = bs->opaque; > int index_in_cluster; > int ret = 0, n; > - uint64_t cluster_offset; > + int64_t cluster_offset; > struct iovec hd_iov; > QEMUIOVector hd_qiov; > uint8_t *buf; > @@ -588,8 +604,10 @@ static coroutine_fn int qcow_co_readv(BlockDriverState *bs, int64_t sector_num, > > while (nb_sectors != 0) { > /* prepare next request */ > - cluster_offset = get_cluster_offset(bs, sector_num << 9, > - 0, 0, 0, 0); > + cluster_offset = get_cluster_offset(bs, sector_num << 9, 0, 0, 0, 0); > + if (cluster_offset < 0) { > + return cluster_offset; > + } > index_in_cluster = sector_num & (s->cluster_sectors - 1); > n = s->cluster_sectors - index_in_cluster; > if (n > nb_sectors) { > @@ -670,7 +688,7 @@ static coroutine_fn int qcow_co_writev(BlockDriverState *bs, int64_t sector_num, > { > BDRVQcowState *s = bs->opaque; > int index_in_cluster; > - uint64_t cluster_offset; > + int64_t cluster_offset; > int ret = 0, n; > struct iovec hd_iov; > QEMUIOVector hd_qiov; > @@ -704,6 +722,10 @@ static coroutine_fn int qcow_co_writev(BlockDriverState *bs, int64_t sector_num, > cluster_offset = get_cluster_offset(bs, sector_num << 9, 1, 0, > index_in_cluster, > index_in_cluster + n); > + if (cluster_offset < 0) { > + ret = cluster_offset; > + break; > + } > if (!cluster_offset || (cluster_offset & 511) != 0) { > ret = -EIO; > break; > @@ -949,7 +971,7 @@ qcow_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset, > z_stream strm; > int ret, out_len; > uint8_t *buf, *out_buf; > - uint64_t cluster_offset; > + int64_t cluster_offset; > > buf = qemu_blockalign(bs, s->cluster_size); > if (bytes != s->cluster_size) { > @@ -1003,6 +1025,10 @@ qcow_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset, > qemu_co_mutex_lock(&s->lock); > cluster_offset = get_cluster_offset(bs, offset, 2, out_len, 0, 0); > qemu_co_mutex_unlock(&s->lock); > + if (cluster_offset < 0) { > + ret = cluster_offset; > + goto fail; > + } > if (cluster_offset == 0) { > ret = -EIO; > goto fail; > -- > 2.13.4 > >
Am 07.08.2017 um 22:30 hat Eric Blake geschrieben: > This also requires changing the return type of get_cluster_offset() > and adjusting all callers. > > Use osdep.h macros instead of open-coded rounding while in the > area. > > Reported-by: Markus Armbruster <armbru@redhat.com> > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > block/qcow.c | 64 ++++++++++++++++++++++++++++++++++++++++++------------------ > 1 file changed, 45 insertions(+), 19 deletions(-) > > diff --git a/block/qcow.c b/block/qcow.c > index c08cdc4a7b..937023d447 100644 > --- a/block/qcow.c > +++ b/block/qcow.c > @@ -347,16 +347,17 @@ static int qcow_reopen_prepare(BDRVReopenState *state, > * 'compressed_size'. 'compressed_size' must be > 0 and < > * cluster_size > * > - * return 0 if not allocated. > + * return 0 if not allocated, -errno on failure. > */ > -static uint64_t get_cluster_offset(BlockDriverState *bs, > - uint64_t offset, int allocate, > - int compressed_size, > - int n_start, int n_end) > +static int64_t get_cluster_offset(BlockDriverState *bs, > + uint64_t offset, int allocate, > + int compressed_size, > + int n_start, int n_end) qcow2 ended up with this prototype: int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t offset, unsigned int *bytes, uint64_t *cluster_offset) Separating a full uin64_t offset by-reference parameter and an 0/-errno status return code feels a little cleaner to me. And in fact, it's not only cleaner, but bit 63 is QCOW_OFLAG_COMPRESSED, so this patch breaks compressed images. > { > BDRVQcowState *s = bs->opaque; > int min_index, i, j, l1_index, l2_index; > - uint64_t l2_offset, *l2_table, cluster_offset, tmp; > + int64_t l2_offset, cluster_offset; > + uint64_t *l2_table, tmp; > uint32_t min_count; > int new_l2_table; > > @@ -368,8 +369,11 @@ static uint64_t get_cluster_offset(BlockDriverState *bs, > return 0; > /* allocate a new l2 entry */ > l2_offset = bdrv_getlength(bs->file->bs); > + if (l2_offset < 0) { > + return l2_offset; > + } > /* round to cluster size */ > - l2_offset = (l2_offset + s->cluster_size - 1) & ~(s->cluster_size - 1); > + l2_offset = QEMU_ALIGN_UP(l2_offset, s->cluster_size); > /* update the L1 entry */ > s->l1_table[l1_index] = l2_offset; > tmp = cpu_to_be64(l2_offset); > @@ -430,8 +434,10 @@ static uint64_t get_cluster_offset(BlockDriverState *bs, > if (decompress_cluster(bs, cluster_offset) < 0) > return 0; > cluster_offset = bdrv_getlength(bs->file->bs); > - cluster_offset = (cluster_offset + s->cluster_size - 1) & > - ~(s->cluster_size - 1); > + if (cluster_offset < 0) { > + return cluster_offset; > + } > + cluster_offset = QEMU_ALIGN_UP(cluster_offset, s->cluster_size); > /* write the cluster content */ > if (bdrv_pwrite(bs->file, cluster_offset, s->cluster_cache, > s->cluster_size) != > @@ -439,12 +445,19 @@ static uint64_t get_cluster_offset(BlockDriverState *bs, > return -1; > } else { > cluster_offset = bdrv_getlength(bs->file->bs); > + if (cluster_offset < 0) { > + return cluster_offset; > + } > if (allocate == 1) { > + int ret; > + > /* round to cluster size */ > - cluster_offset = (cluster_offset + s->cluster_size - 1) & > - ~(s->cluster_size - 1); > - bdrv_truncate(bs->file, cluster_offset + s->cluster_size, > - PREALLOC_MODE_OFF, NULL); > + cluster_offset = QEMU_ALIGN_UP(cluster_offset, s->cluster_size); > + ret = bdrv_truncate(bs->file, cluster_offset + s->cluster_size, > + PREALLOC_MODE_OFF, NULL); > + if (ret < 0) { > + return ret; > + } > /* if encrypted, we must initialize the cluster > content which won't be written */ > if (bs->encrypted && > @@ -491,11 +504,14 @@ static int64_t coroutine_fn qcow_co_get_block_status(BlockDriverState *bs, > { > BDRVQcowState *s = bs->opaque; > int index_in_cluster, n; > - uint64_t cluster_offset; > + int64_t cluster_offset; > > qemu_co_mutex_lock(&s->lock); > cluster_offset = get_cluster_offset(bs, sector_num << 9, 0, 0, 0, 0); > qemu_co_mutex_unlock(&s->lock); > + if (cluster_offset < 0) { > + return cluster_offset; > + } > index_in_cluster = sector_num & (s->cluster_sectors - 1); > n = s->cluster_sectors - index_in_cluster; > if (n > nb_sectors) > @@ -567,7 +583,7 @@ static coroutine_fn int qcow_co_readv(BlockDriverState *bs, int64_t sector_num, > BDRVQcowState *s = bs->opaque; > int index_in_cluster; > int ret = 0, n; > - uint64_t cluster_offset; > + int64_t cluster_offset; > struct iovec hd_iov; > QEMUIOVector hd_qiov; > uint8_t *buf; > @@ -588,8 +604,10 @@ static coroutine_fn int qcow_co_readv(BlockDriverState *bs, int64_t sector_num, > > while (nb_sectors != 0) { > /* prepare next request */ > - cluster_offset = get_cluster_offset(bs, sector_num << 9, > - 0, 0, 0, 0); > + cluster_offset = get_cluster_offset(bs, sector_num << 9, 0, 0, 0, 0); > + if (cluster_offset < 0) { > + return cluster_offset; > + } Oops, we're still holding s->lock. Combined with the bug that compressed clusters are treated as errors now, this makes compressed images result in an assertion failure: qemu-system-x86_64: util/qemu-coroutine.c:138: qemu_aio_coroutine_enter: Assertion `!co->locks_held' failed. > index_in_cluster = sector_num & (s->cluster_sectors - 1); > n = s->cluster_sectors - index_in_cluster; > if (n > nb_sectors) { Kevin
On 08/08/2017 03:28 AM, Kevin Wolf wrote: > Am 07.08.2017 um 22:30 hat Eric Blake geschrieben: >> This also requires changing the return type of get_cluster_offset() >> and adjusting all callers. >> >> Use osdep.h macros instead of open-coded rounding while in the >> area. >> >> Reported-by: Markus Armbruster <armbru@redhat.com> >> Signed-off-by: Eric Blake <eblake@redhat.com> >> --- > qcow2 ended up with this prototype: > > int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t offset, > unsigned int *bytes, uint64_t *cluster_offset) > > Separating a full uin64_t offset by-reference parameter and an 0/-errno > status return code feels a little cleaner to me. > > And in fact, it's not only cleaner, but bit 63 is QCOW_OFLAG_COMPRESSED, > so this patch breaks compressed images. Looks like I'll be sending a v2, then.
diff --git a/block/qcow.c b/block/qcow.c index c08cdc4a7b..937023d447 100644 --- a/block/qcow.c +++ b/block/qcow.c @@ -347,16 +347,17 @@ static int qcow_reopen_prepare(BDRVReopenState *state, * 'compressed_size'. 'compressed_size' must be > 0 and < * cluster_size * - * return 0 if not allocated. + * return 0 if not allocated, -errno on failure. */ -static uint64_t get_cluster_offset(BlockDriverState *bs, - uint64_t offset, int allocate, - int compressed_size, - int n_start, int n_end) +static int64_t get_cluster_offset(BlockDriverState *bs, + uint64_t offset, int allocate, + int compressed_size, + int n_start, int n_end) { BDRVQcowState *s = bs->opaque; int min_index, i, j, l1_index, l2_index; - uint64_t l2_offset, *l2_table, cluster_offset, tmp; + int64_t l2_offset, cluster_offset; + uint64_t *l2_table, tmp; uint32_t min_count; int new_l2_table; @@ -368,8 +369,11 @@ static uint64_t get_cluster_offset(BlockDriverState *bs, return 0; /* allocate a new l2 entry */ l2_offset = bdrv_getlength(bs->file->bs); + if (l2_offset < 0) { + return l2_offset; + } /* round to cluster size */ - l2_offset = (l2_offset + s->cluster_size - 1) & ~(s->cluster_size - 1); + l2_offset = QEMU_ALIGN_UP(l2_offset, s->cluster_size); /* update the L1 entry */ s->l1_table[l1_index] = l2_offset; tmp = cpu_to_be64(l2_offset); @@ -430,8 +434,10 @@ static uint64_t get_cluster_offset(BlockDriverState *bs, if (decompress_cluster(bs, cluster_offset) < 0) return 0; cluster_offset = bdrv_getlength(bs->file->bs); - cluster_offset = (cluster_offset + s->cluster_size - 1) & - ~(s->cluster_size - 1); + if (cluster_offset < 0) { + return cluster_offset; + } + cluster_offset = QEMU_ALIGN_UP(cluster_offset, s->cluster_size); /* write the cluster content */ if (bdrv_pwrite(bs->file, cluster_offset, s->cluster_cache, s->cluster_size) != @@ -439,12 +445,19 @@ static uint64_t get_cluster_offset(BlockDriverState *bs, return -1; } else { cluster_offset = bdrv_getlength(bs->file->bs); + if (cluster_offset < 0) { + return cluster_offset; + } if (allocate == 1) { + int ret; + /* round to cluster size */ - cluster_offset = (cluster_offset + s->cluster_size - 1) & - ~(s->cluster_size - 1); - bdrv_truncate(bs->file, cluster_offset + s->cluster_size, - PREALLOC_MODE_OFF, NULL); + cluster_offset = QEMU_ALIGN_UP(cluster_offset, s->cluster_size); + ret = bdrv_truncate(bs->file, cluster_offset + s->cluster_size, + PREALLOC_MODE_OFF, NULL); + if (ret < 0) { + return ret; + } /* if encrypted, we must initialize the cluster content which won't be written */ if (bs->encrypted && @@ -491,11 +504,14 @@ static int64_t coroutine_fn qcow_co_get_block_status(BlockDriverState *bs, { BDRVQcowState *s = bs->opaque; int index_in_cluster, n; - uint64_t cluster_offset; + int64_t cluster_offset; qemu_co_mutex_lock(&s->lock); cluster_offset = get_cluster_offset(bs, sector_num << 9, 0, 0, 0, 0); qemu_co_mutex_unlock(&s->lock); + if (cluster_offset < 0) { + return cluster_offset; + } index_in_cluster = sector_num & (s->cluster_sectors - 1); n = s->cluster_sectors - index_in_cluster; if (n > nb_sectors) @@ -567,7 +583,7 @@ static coroutine_fn int qcow_co_readv(BlockDriverState *bs, int64_t sector_num, BDRVQcowState *s = bs->opaque; int index_in_cluster; int ret = 0, n; - uint64_t cluster_offset; + int64_t cluster_offset; struct iovec hd_iov; QEMUIOVector hd_qiov; uint8_t *buf; @@ -588,8 +604,10 @@ static coroutine_fn int qcow_co_readv(BlockDriverState *bs, int64_t sector_num, while (nb_sectors != 0) { /* prepare next request */ - cluster_offset = get_cluster_offset(bs, sector_num << 9, - 0, 0, 0, 0); + cluster_offset = get_cluster_offset(bs, sector_num << 9, 0, 0, 0, 0); + if (cluster_offset < 0) { + return cluster_offset; + } index_in_cluster = sector_num & (s->cluster_sectors - 1); n = s->cluster_sectors - index_in_cluster; if (n > nb_sectors) { @@ -670,7 +688,7 @@ static coroutine_fn int qcow_co_writev(BlockDriverState *bs, int64_t sector_num, { BDRVQcowState *s = bs->opaque; int index_in_cluster; - uint64_t cluster_offset; + int64_t cluster_offset; int ret = 0, n; struct iovec hd_iov; QEMUIOVector hd_qiov; @@ -704,6 +722,10 @@ static coroutine_fn int qcow_co_writev(BlockDriverState *bs, int64_t sector_num, cluster_offset = get_cluster_offset(bs, sector_num << 9, 1, 0, index_in_cluster, index_in_cluster + n); + if (cluster_offset < 0) { + ret = cluster_offset; + break; + } if (!cluster_offset || (cluster_offset & 511) != 0) { ret = -EIO; break; @@ -949,7 +971,7 @@ qcow_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset, z_stream strm; int ret, out_len; uint8_t *buf, *out_buf; - uint64_t cluster_offset; + int64_t cluster_offset; buf = qemu_blockalign(bs, s->cluster_size); if (bytes != s->cluster_size) { @@ -1003,6 +1025,10 @@ qcow_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset, qemu_co_mutex_lock(&s->lock); cluster_offset = get_cluster_offset(bs, offset, 2, out_len, 0, 0); qemu_co_mutex_unlock(&s->lock); + if (cluster_offset < 0) { + ret = cluster_offset; + goto fail; + } if (cluster_offset == 0) { ret = -EIO; goto fail;
This also requires changing the return type of get_cluster_offset() and adjusting all callers. Use osdep.h macros instead of open-coded rounding while in the area. Reported-by: Markus Armbruster <armbru@redhat.com> Signed-off-by: Eric Blake <eblake@redhat.com> --- block/qcow.c | 64 ++++++++++++++++++++++++++++++++++++++++++------------------ 1 file changed, 45 insertions(+), 19 deletions(-)