Re: [Qemu-devel] [PATCH 2/4] qcow: Check failure of bdrv_getlength() and bdrv_truncate()

2017-08-08 Thread Eric Blake 
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 
>> Signed-off-by: Eric Blake 
>> ---

> 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.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 2/4] qcow: Check failure of bdrv_getlength() and bdrv_truncate()

2017-08-08 Thread Kevin Wolf 
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 
> Signed-off-by: Eric Blake 
> ---
>  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(>lock);
>  cluster_offset = get_cluster_offset(bs, sector_num << 9, 0, 0, 0, 0);
>  qemu_co_mutex_unlock(>lock);
> +if (cluster_offset < 0) {
> +return cluster_offset;
> +}
>  index_in_cluster =

Re: [Qemu-devel] [PATCH 2/4] qcow: Check failure of bdrv_getlength() and bdrv_truncate()

2017-08-07 Thread Philippe Mathieu-Daudé 

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 
Signed-off-by: Eric Blake 


Reviewed-by: Philippe Mathieu-Daudé 


---
  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(>lock);
  cluster_offset = get_cluster_offset(bs, sector_num << 9, 0, 0, 0, 0);
  qemu_co_mutex_unlock(>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

[Qemu-devel] [PATCH 2/4] qcow: Check failure of bdrv_getlength() and bdrv_truncate()

2017-08-07 Thread Eric Blake 
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 
Signed-off-by: Eric Blake 
---
 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(>lock);
 cluster_offset = get_cluster_offset(bs, sector_num << 9, 0, 0, 0, 0);
 qemu_co_mutex_unlock(>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,
-