[Qemu-devel] [PATCH 00/10] qcow2 error path fixes

[Qemu-devel] [PATCH 00/10] qcow2 error path fixes

[Kevin Wolf]

In qcow2 (and partly also in general block code), error handling doesn't quite
do what it should. In some places, errors are silently ignored. In other
places, the error code in not passed to the called but rather turned into
false, NULL or -EIO. Besides reporting the right error to the user, providing
the right error code is also important for werror=enospc (and possible some
more functions).

This patch series tries to fix the most obvious of them. Don't assume it's
complete and expect a second part some time in the future.

Kevin Wolf (10):
  qcow2: Fix error handling in qcow2_grow_l1_table
  qcow2: Fix error handling in qcow_save_vmstate
  qcow2: Return 0/-errno in get_cluster_table
  qcow2: Return 0/-errno in qcow2_alloc_cluster_offset
  block: Return original error codes in bdrv_pread/write
  qcow2: Fix error handling in grow_refcount_table
  qcow2: Improve error handling in update_refcount
  qcow2: Allow updating no refcounts
  qcow2: Don't ignore update_refcount return value
  qcow2: Don't ignore qcow2_alloc_clusters return value

 block.c                |   34 ++++++++++---------
 block/qcow2-cluster.c  |   84 ++++++++++++++++++++++++++++++++----------------
 block/qcow2-refcount.c |   82 ++++++++++++++++++++++++++++++++++++-----------
 block/qcow2-snapshot.c |   11 ++++++-
 block/qcow2.c          |   41 ++++++++++++-----------
 block/qcow2.h          |    4 +-
 6 files changed, 171 insertions(+), 85 deletions(-)

[Qemu-devel] [PATCH 01/10] qcow2: Fix error handling in qcow2_grow_l1_table

[Kevin Wolf]

Return the appropriate error value instead of always using EIO. Don't free the
L1 table on errors, we still need it.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qcow2-cluster.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index f88118c..adddf56 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -67,9 +67,10 @@ int qcow2_grow_l1_table(BlockDriverState *bs, int min_size)
     /* set new table */
     cpu_to_be32w((uint32_t*)data, new_l1_size);
     cpu_to_be64w((uint64_t*)(data + 4), new_l1_table_offset);
-    if (bdrv_pwrite(s->hd, offsetof(QCowHeader, l1_size), data,
-                sizeof(data)) != sizeof(data))
+    ret = bdrv_pwrite(s->hd, offsetof(QCowHeader, l1_size), data,sizeof(data));
+    if (ret != sizeof(data)) {
         goto fail;
+    }
     qemu_free(s->l1_table);
     qcow2_free_clusters(bs, s->l1_table_offset, s->l1_size * sizeof(uint64_t));
     s->l1_table_offset = new_l1_table_offset;
@@ -77,8 +78,7 @@ int qcow2_grow_l1_table(BlockDriverState *bs, int min_size)
     s->l1_size = new_l1_size;
     return 0;
  fail:
-    qemu_free(s->l1_table);
-    return -EIO;
+    return ret < 0 ? ret : -EIO;
 }
 
 void qcow2_l2_cache_reset(BlockDriverState *bs)
-- 
1.6.2.5

Re: [Qemu-devel] [PATCH 01/10] qcow2: Fix error handling in qcow2_grow_l1_table

[Christoph Hellwig]

On Mon, Jan 18, 2010 at 01:11:27PM +0100, Kevin Wolf wrote:
> Return the appropriate error value instead of always using EIO. Don't free the
> L1 table on errors, we still need it.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

Looks good correct, but shouldn't we free the clusters for the new l1
table if writing to it fails?  At least dependend on whether it's EIO
in which case we could assume the sectos on disk to be worn out.

Re: [Qemu-devel] [PATCH 01/10] qcow2: Fix error handling in qcow2_grow_l1_table

[Kevin Wolf]

Am 19.01.2010 11:58, schrieb Christoph Hellwig:
> On Mon, Jan 18, 2010 at 01:11:27PM +0100, Kevin Wolf wrote:
>> Return the appropriate error value instead of always using EIO. Don't free the
>> L1 table on errors, we still need it.
>>
>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> 
> Looks good correct, but shouldn't we free the clusters for the new l1
> table if writing to it fails?  At least dependend on whether it's EIO
> in which case we could assume the sectos on disk to be worn out.

Agreed, we should try to free them. At least in RHEV, the most likely
case for failure will be ENOSPC and we're going to succeed with the free
in that case.

This will become the first patch of the second series then. ;-)

Kevin

[Qemu-devel] [PATCH 02/10] qcow2: Fix error handling in qcow_save_vmstate

[Kevin Wolf]

Don't assume success but pass the bdrv_pwrite return value on.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qcow2.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 6622eba..e06f4dd 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1163,12 +1163,13 @@ static int qcow_save_vmstate(BlockDriverState *bs, const uint8_t *buf,
 {
     BDRVQcowState *s = bs->opaque;
     int growable = bs->growable;
+    int ret;
 
     bs->growable = 1;
-    bdrv_pwrite(bs, qcow_vm_state_offset(s) + pos, buf, size);
+    ret = bdrv_pwrite(bs, qcow_vm_state_offset(s) + pos, buf, size);
     bs->growable = growable;
 
-    return size;
+    return ret;
 }
 
 static int qcow_load_vmstate(BlockDriverState *bs, uint8_t *buf,
-- 
1.6.2.5

Re: [Qemu-devel] [PATCH 02/10] qcow2: Fix error handling in qcow_save_vmstate

[Christoph Hellwig]

On Mon, Jan 18, 2010 at 01:11:28PM +0100, Kevin Wolf wrote:
> Don't assume success but pass the bdrv_pwrite return value on.

Looks good,


Reviewed-by: Christoph Hellwig <hch@lst.de>

[Qemu-devel] [PATCH 03/10] qcow2: Return 0/-errno in get_cluster_table

[Kevin Wolf]

Switching to 0/-errno allows it to distinguish different error cases.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qcow2-cluster.c |   30 ++++++++++++++++++------------
 1 files changed, 18 insertions(+), 12 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index adddf56..424bedd 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -479,8 +479,8 @@ out:
  * the l2 table offset in the qcow2 file and the cluster index
  * in the l2 table are given to the caller.
  *
+ * Returns 0 on success, -errno in failure case
  */
-
 static int get_cluster_table(BlockDriverState *bs, uint64_t offset,
                              uint64_t **new_l2_table,
                              uint64_t *new_l2_offset,
@@ -496,8 +496,9 @@ static int get_cluster_table(BlockDriverState *bs, uint64_t offset,
     l1_index = offset >> (s->l2_bits + s->cluster_bits);
     if (l1_index >= s->l1_size) {
         ret = qcow2_grow_l1_table(bs, l1_index + 1);
-        if (ret < 0)
-            return 0;
+        if (ret < 0) {
+            return ret;
+        }
     }
     l2_offset = s->l1_table[l1_index];
 
@@ -507,14 +508,16 @@ static int get_cluster_table(BlockDriverState *bs, uint64_t offset,
         /* load the l2 table in memory */
         l2_offset &= ~QCOW_OFLAG_COPIED;
         l2_table = l2_load(bs, l2_offset);
-        if (l2_table == NULL)
-            return 0;
+        if (l2_table == NULL) {
+            return -EIO;
+        }
     } else {
         if (l2_offset)
             qcow2_free_clusters(bs, l2_offset, s->l2_size * sizeof(uint64_t));
         l2_table = l2_allocate(bs, l1_index);
-        if (l2_table == NULL)
-            return 0;
+        if (l2_table == NULL) {
+            return -EIO;
+        }
         l2_offset = s->l1_table[l1_index] & ~QCOW_OFLAG_COPIED;
     }
 
@@ -526,7 +529,7 @@ static int get_cluster_table(BlockDriverState *bs, uint64_t offset,
     *new_l2_offset = l2_offset;
     *new_l2_index = l2_index;
 
-    return 1;
+    return 0;
 }
 
 /*
@@ -552,8 +555,9 @@ uint64_t qcow2_alloc_compressed_cluster_offset(BlockDriverState *bs,
     int nb_csectors;
 
     ret = get_cluster_table(bs, offset, &l2_table, &l2_offset, &l2_index);
-    if (ret == 0)
+    if (ret < 0) {
         return 0;
+    }
 
     cluster_offset = be64_to_cpu(l2_table[l2_index]);
     if (cluster_offset & QCOW_OFLAG_COPIED)
@@ -633,10 +637,11 @@ int qcow2_alloc_cluster_link_l2(BlockDriverState *bs, uint64_t cluster_offset,
             goto err;
     }
 
-    ret = -EIO;
     /* update L2 table */
-    if (!get_cluster_table(bs, m->offset, &l2_table, &l2_offset, &l2_index))
+    ret = get_cluster_table(bs, m->offset, &l2_table, &l2_offset, &l2_index);
+    if (ret < 0) {
         goto err;
+    }
 
     for (i = 0; i < m->nb_clusters; i++) {
         /* if two concurrent writes happen to the same unallocated cluster
@@ -692,8 +697,9 @@ uint64_t qcow2_alloc_cluster_offset(BlockDriverState *bs,
     QCowL2Meta *old_alloc;
 
     ret = get_cluster_table(bs, offset, &l2_table, &l2_offset, &l2_index);
-    if (ret == 0)
+    if (ret < 0) {
         return 0;
+    }
 
     nb_clusters = size_to_clusters(s, n_end << 9);
 
-- 
1.6.2.5

Re: [Qemu-devel] [PATCH 03/10] qcow2: Return 0/-errno in get_cluster_table

[Christoph Hellwig]

On Mon, Jan 18, 2010 at 01:11:29PM +0100, Kevin Wolf wrote:
> Switching to 0/-errno allows it to distinguish different error cases.

Looks good,


Reviewed-by: Christoph Hellwig <hch@lst.de>

[Qemu-devel] [PATCH 04/10] qcow2: Return 0/-errno in qcow2_alloc_cluster_offset

[Kevin Wolf]

Returning 0/-errno allows it to distingush different errors classes. The
cluster offset of newly allocated clusters is now returned in the QCowL2Meta
struct.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qcow2-cluster.c |   28 ++++++++++++++++++----------
 block/qcow2.c         |   36 +++++++++++++++++++-----------------
 block/qcow2.h         |    4 ++--
 3 files changed, 39 insertions(+), 29 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 424bedd..3e877a4 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -609,12 +609,12 @@ static int write_l2_entries(BDRVQcowState *s, uint64_t *l2_table,
     return 0;
 }
 
-int qcow2_alloc_cluster_link_l2(BlockDriverState *bs, uint64_t cluster_offset,
-    QCowL2Meta *m)
+int qcow2_alloc_cluster_link_l2(BlockDriverState *bs, QCowL2Meta *m)
 {
     BDRVQcowState *s = bs->opaque;
     int i, j = 0, l2_index, ret;
     uint64_t *old_cluster, start_sect, l2_offset, *l2_table;
+    uint64_t cluster_offset = m->cluster_offset;
 
     if (m->nb_clusters == 0)
         return 0;
@@ -675,16 +675,22 @@ err:
 /*
  * alloc_cluster_offset
  *
- * For a given offset of the disk image, return cluster offset in
- * qcow2 file.
- *
+ * For a given offset of the disk image, return cluster offset in qcow2 file.
  * If the offset is not found, allocate a new cluster.
  *
- * Return the cluster offset if successful,
- * Return 0, otherwise.
+ * If the cluster was already allocated, m->nb_clusters is set to 0,
+ * m->depends_on is set to NULL and the other fields in m are meaningless.
+ *
+ * If the cluster is newly allocated, m->nb_clusters is set to the number of
+ * contiguous clusters that have been allocated. This may be 0 if the request
+ * conflict with another write request in flight; in this case, m->depends_on
+ * is set and the remaining fields of m are meaningless.
  *
+ * If m->nb_clusters is non-zero, the other fields of m are valid and contain
+ * information about the first allocated cluster.
+ *
+ * Return 0 on success and -errno in error cases
  */
-
 uint64_t qcow2_alloc_cluster_offset(BlockDriverState *bs,
                                     uint64_t offset,
                                     int n_start, int n_end,
@@ -698,7 +704,7 @@ uint64_t qcow2_alloc_cluster_offset(BlockDriverState *bs,
 
     ret = get_cluster_table(bs, offset, &l2_table, &l2_offset, &l2_index);
     if (ret < 0) {
-        return 0;
+        return ret;
     }
 
     nb_clusters = size_to_clusters(s, n_end << 9);
@@ -715,6 +721,7 @@ uint64_t qcow2_alloc_cluster_offset(BlockDriverState *bs,
 
         cluster_offset &= ~QCOW_OFLAG_COPIED;
         m->nb_clusters = 0;
+        m->depends_on = NULL;
 
         goto out;
     }
@@ -793,10 +800,11 @@ uint64_t qcow2_alloc_cluster_offset(BlockDriverState *bs,
 
 out:
     m->nb_available = MIN(nb_clusters << (s->cluster_bits - 9), n_end);
+    m->cluster_offset = cluster_offset;
 
     *num = m->nb_available - n_start;
 
-    return cluster_offset;
+    return 0;
 }
 
 static int decompress_buffer(uint8_t *out_buf, int out_buf_size,
diff --git a/block/qcow2.c b/block/qcow2.c
index e06f4dd..773d381 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -561,7 +561,7 @@ static void qcow_aio_write_cb(void *opaque, int ret)
     acb->hd_aiocb = NULL;
 
     if (ret >= 0) {
-        ret = qcow2_alloc_cluster_link_l2(bs, acb->cluster_offset, &acb->l2meta);
+        ret = qcow2_alloc_cluster_link_l2(bs, &acb->l2meta);
     }
 
     run_dependent_requests(&acb->l2meta);
@@ -585,21 +585,23 @@ static void qcow_aio_write_cb(void *opaque, int ret)
         n_end > QCOW_MAX_CRYPT_CLUSTERS * s->cluster_sectors)
         n_end = QCOW_MAX_CRYPT_CLUSTERS * s->cluster_sectors;
 
-    acb->cluster_offset = qcow2_alloc_cluster_offset(bs, acb->sector_num << 9,
-                                          index_in_cluster,
-                                          n_end, &acb->n, &acb->l2meta);
+    ret = qcow2_alloc_cluster_offset(bs, acb->sector_num << 9,
+        index_in_cluster, n_end, &acb->n, &acb->l2meta);
+    if (ret < 0) {
+        goto done;
+    }
+
+    acb->cluster_offset = acb->l2meta.cluster_offset;
 
     /* Need to wait for another request? If so, we are done for now. */
-    if (!acb->cluster_offset && acb->l2meta.depends_on != NULL) {
+    if (acb->l2meta.nb_clusters == 0 && acb->l2meta.depends_on != NULL) {
         QLIST_INSERT_HEAD(&acb->l2meta.depends_on->dependent_requests,
             acb, next_depend);
         return;
     }
 
-    if (!acb->cluster_offset || (acb->cluster_offset & 511) != 0) {
-        ret = -EIO;
-        goto done;
-    }
+    assert((acb->cluster_offset & 511) == 0);
+
     if (s->crypt_method) {
         if (!acb->cluster_data) {
             acb->cluster_data = qemu_mallocz(QCOW_MAX_CRYPT_CLUSTERS *
@@ -782,27 +784,27 @@ static int get_bits_from_size(size_t size)
 static int preallocate(BlockDriverState *bs)
 {
     BDRVQcowState *s = bs->opaque;
-    uint64_t cluster_offset = 0;
     uint64_t nb_sectors;
     uint64_t offset;
     int num;
+    int ret;
     QCowL2Meta meta;
 
     nb_sectors = bdrv_getlength(bs) >> 9;
     offset = 0;
     QLIST_INIT(&meta.dependent_requests);
+    meta.cluster_offset = 0;
 
     while (nb_sectors) {
         num = MIN(nb_sectors, INT_MAX >> 9);
-        cluster_offset = qcow2_alloc_cluster_offset(bs, offset, 0, num, &num,
-            &meta);
+        ret = qcow2_alloc_cluster_offset(bs, offset, 0, num, &num, &meta);
 
-        if (cluster_offset == 0) {
+        if (ret < 0) {
             return -1;
         }
 
-        if (qcow2_alloc_cluster_link_l2(bs, cluster_offset, &meta) < 0) {
-            qcow2_free_any_clusters(bs, cluster_offset, meta.nb_clusters);
+        if (qcow2_alloc_cluster_link_l2(bs, &meta) < 0) {
+            qcow2_free_any_clusters(bs, meta.cluster_offset, meta.nb_clusters);
             return -1;
         }
 
@@ -821,10 +823,10 @@ static int preallocate(BlockDriverState *bs)
      * all of the allocated clusters (otherwise we get failing reads after
      * EOF). Extend the image to the last allocated sector.
      */
-    if (cluster_offset != 0) {
+    if (meta.cluster_offset != 0) {
         uint8_t buf[512];
         memset(buf, 0, 512);
-        bdrv_write(s->hd, (cluster_offset >> 9) + num - 1, buf, 1);
+        bdrv_write(s->hd, (meta.cluster_offset >> 9) + num - 1, buf, 1);
     }
 
     return 0;
diff --git a/block/qcow2.h b/block/qcow2.h
index 26ab5d9..d9ea6ab 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -135,6 +135,7 @@ struct QCowAIOCB;
 typedef struct QCowL2Meta
 {
     uint64_t offset;
+    uint64_t cluster_offset;
     int n_start;
     int nb_available;
     int nb_clusters;
@@ -199,8 +200,7 @@ uint64_t qcow2_alloc_compressed_cluster_offset(BlockDriverState *bs,
                                          uint64_t offset,
                                          int compressed_size);
 
-int qcow2_alloc_cluster_link_l2(BlockDriverState *bs, uint64_t cluster_offset,
-    QCowL2Meta *m);
+int qcow2_alloc_cluster_link_l2(BlockDriverState *bs, QCowL2Meta *m);
 
 /* qcow2-snapshot.c functions */
 int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info);
-- 
1.6.2.5

Re: [Qemu-devel] [PATCH 04/10] qcow2: Return 0/-errno in qcow2_alloc_cluster_offset

[Christoph Hellwig]

> @@ -715,6 +721,7 @@ uint64_t qcow2_alloc_cluster_offset(BlockDriverState *bs,
>  
>          cluster_offset &= ~QCOW_OFLAG_COPIED;
>          m->nb_clusters = 0;
> +        m->depends_on = NULL;

What does this have to do with the rest?

Otherwise looks good,


Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [Qemu-devel] [PATCH 04/10] qcow2: Return 0/-errno in qcow2_alloc_cluster_offset

[Kevin Wolf]

Am 19.01.2010 12:35, schrieb Christoph Hellwig:
>> @@ -715,6 +721,7 @@ uint64_t qcow2_alloc_cluster_offset(BlockDriverState *bs,
>>  
>>          cluster_offset &= ~QCOW_OFLAG_COPIED;
>>          m->nb_clusters = 0;
>> +        m->depends_on = NULL;
> 
> What does this have to do with the rest?

It's needed to be able to distinguish between the case where the
clusters are already allocated (0/NULL) and the case where the request
depends on another one (0/non-NULL). This check previously used the
return value (cluster_offset for success, 0 for failure) and I didn't
want to overload m->cluster_offset with such a meaning. This is the
change in the caller:

   /* Need to wait for another request? If so, we are done for now. */
-  if (!acb->cluster_offset && acb->l2meta.depends_on != NULL) {
+  if (acb->l2meta.nb_clusters == 0 && acb->l2meta.depends_on != NULL) {

The alternative would have been to keep using the return value and
hijack some errno value. This would possibly conflict with real
read/write errors though, so I decided to leave the return value alone.

Kevin

Re: [Qemu-devel] [PATCH 04/10] qcow2: Return 0/-errno in qcow2_alloc_cluster_offset

[Christoph Hellwig]

On Tue, Jan 19, 2010 at 12:57:35PM +0100, Kevin Wolf wrote:
> It's needed to be able to distinguish between the case where the
> clusters are already allocated (0/NULL) and the case where the request
> depends on another one (0/non-NULL). This check previously used the
> return value (cluster_offset for success, 0 for failure) and I didn't
> want to overload m->cluster_offset with such a meaning. This is the
> change in the caller:
> 
>    /* Need to wait for another request? If so, we are done for now. */
> -  if (!acb->cluster_offset && acb->l2meta.depends_on != NULL) {
> +  if (acb->l2meta.nb_clusters == 0 && acb->l2meta.depends_on != NULL) {
> 
> The alternative would have been to keep using the return value and
> hijack some errno value. This would possibly conflict with real
> read/write errors though, so I decided to leave the return value alone.

Ok, makes sense.

[Qemu-devel] [PATCH 05/10] block: Return original error codes in bdrv_pread/write

[Kevin Wolf]

Don't assume -EIO but return the real error.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c |   34 ++++++++++++++++++----------------
 1 files changed, 18 insertions(+), 16 deletions(-)

diff --git a/block.c b/block.c
index 115e591..a4c4953 100644
--- a/block.c
+++ b/block.c
@@ -717,6 +717,7 @@ int bdrv_pread(BlockDriverState *bs, int64_t offset,
     uint8_t tmp_buf[BDRV_SECTOR_SIZE];
     int len, nb_sectors, count;
     int64_t sector_num;
+    int ret;
 
     count = count1;
     /* first read to align to sector start */
@@ -725,8 +726,8 @@ int bdrv_pread(BlockDriverState *bs, int64_t offset,
         len = count;
     sector_num = offset >> BDRV_SECTOR_BITS;
     if (len > 0) {
-        if (bdrv_read(bs, sector_num, tmp_buf, 1) < 0)
-            return -EIO;
+        if ((ret = bdrv_read(bs, sector_num, tmp_buf, 1)) < 0)
+            return ret;
         memcpy(buf, tmp_buf + (offset & (BDRV_SECTOR_SIZE - 1)), len);
         count -= len;
         if (count == 0)
@@ -738,8 +739,8 @@ int bdrv_pread(BlockDriverState *bs, int64_t offset,
     /* read the sectors "in place" */
     nb_sectors = count >> BDRV_SECTOR_BITS;
     if (nb_sectors > 0) {
-        if (bdrv_read(bs, sector_num, buf, nb_sectors) < 0)
-            return -EIO;
+        if ((ret = bdrv_read(bs, sector_num, buf, nb_sectors)) < 0)
+            return ret;
         sector_num += nb_sectors;
         len = nb_sectors << BDRV_SECTOR_BITS;
         buf += len;
@@ -748,8 +749,8 @@ int bdrv_pread(BlockDriverState *bs, int64_t offset,
 
     /* add data from the last sector */
     if (count > 0) {
-        if (bdrv_read(bs, sector_num, tmp_buf, 1) < 0)
-            return -EIO;
+        if ((ret = bdrv_read(bs, sector_num, tmp_buf, 1)) < 0)
+            return ret;
         memcpy(buf, tmp_buf, count);
     }
     return count1;
@@ -761,6 +762,7 @@ int bdrv_pwrite(BlockDriverState *bs, int64_t offset,
     uint8_t tmp_buf[BDRV_SECTOR_SIZE];
     int len, nb_sectors, count;
     int64_t sector_num;
+    int ret;
 
     count = count1;
     /* first write to align to sector start */
@@ -769,11 +771,11 @@ int bdrv_pwrite(BlockDriverState *bs, int64_t offset,
         len = count;
     sector_num = offset >> BDRV_SECTOR_BITS;
     if (len > 0) {
-        if (bdrv_read(bs, sector_num, tmp_buf, 1) < 0)
-            return -EIO;
+        if ((ret = bdrv_read(bs, sector_num, tmp_buf, 1)) < 0)
+            return ret;
         memcpy(tmp_buf + (offset & (BDRV_SECTOR_SIZE - 1)), buf, len);
-        if (bdrv_write(bs, sector_num, tmp_buf, 1) < 0)
-            return -EIO;
+        if ((ret = bdrv_write(bs, sector_num, tmp_buf, 1)) < 0)
+            return ret;
         count -= len;
         if (count == 0)
             return count1;
@@ -784,8 +786,8 @@ int bdrv_pwrite(BlockDriverState *bs, int64_t offset,
     /* write the sectors "in place" */
     nb_sectors = count >> BDRV_SECTOR_BITS;
     if (nb_sectors > 0) {
-        if (bdrv_write(bs, sector_num, buf, nb_sectors) < 0)
-            return -EIO;
+        if ((ret = bdrv_write(bs, sector_num, buf, nb_sectors)) < 0)
+            return ret;
         sector_num += nb_sectors;
         len = nb_sectors << BDRV_SECTOR_BITS;
         buf += len;
@@ -794,11 +796,11 @@ int bdrv_pwrite(BlockDriverState *bs, int64_t offset,
 
     /* add data from the last sector */
     if (count > 0) {
-        if (bdrv_read(bs, sector_num, tmp_buf, 1) < 0)
-            return -EIO;
+        if ((ret = bdrv_read(bs, sector_num, tmp_buf, 1)) < 0)
+            return ret;
         memcpy(tmp_buf, buf, count);
-        if (bdrv_write(bs, sector_num, tmp_buf, 1) < 0)
-            return -EIO;
+        if ((ret = bdrv_write(bs, sector_num, tmp_buf, 1)) < 0)
+            return ret;
     }
     return count1;
 }
-- 
1.6.2.5

Re: [Qemu-devel] [PATCH 05/10] block: Return original error codes in bdrv_pread/write

[Christoph Hellwig]

On Mon, Jan 18, 2010 at 01:11:31PM +0100, Kevin Wolf wrote:
> Don't assume -EIO but return the real error.

While I'm not very found of assignment inside of if statements the patch
looks correct,


Reviewed-by: Christoph Hellwig <hch@lst.de>

[Qemu-devel] [PATCH 06/10] qcow2: Fix error handling in grow_refcount_table

[Kevin Wolf]

Return the appropriate error code instead of -EIO.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qcow2-refcount.c |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 3a2d44a..6f449c6 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -168,9 +168,12 @@ static int grow_refcount_table(BlockDriverState *bs, int min_size)
 
     cpu_to_be64w((uint64_t*)data, table_offset);
     cpu_to_be32w((uint32_t*)(data + 8), refcount_table_clusters);
-    if (bdrv_pwrite(s->hd, offsetof(QCowHeader, refcount_table_offset),
-                    data, sizeof(data)) != sizeof(data))
+    ret = bdrv_pwrite(s->hd, offsetof(QCowHeader, refcount_table_offset),
+                    data, sizeof(data));
+    if (ret != sizeof(data)) {
         goto fail;
+    }
+
     qemu_free(s->refcount_table);
     old_table_offset = s->refcount_table_offset;
     old_table_size = s->refcount_table_size;
@@ -183,7 +186,7 @@ static int grow_refcount_table(BlockDriverState *bs, int min_size)
     return 0;
  fail:
     qemu_free(new_table);
-    return -EIO;
+    return ret < 0 ? ret : -EIO;
 }
 
 
-- 
1.6.2.5

Re: [Qemu-devel] [PATCH 06/10] qcow2: Fix error handling in grow_refcount_table

[Christoph Hellwig]

On Mon, Jan 18, 2010 at 01:11:32PM +0100, Kevin Wolf wrote:
> Return the appropriate error code instead of -EIO.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

Looks good,


Reviewed-by: Christoph Hellwig <hch@lst.de>

[Qemu-devel] [PATCH 07/10] qcow2: Improve error handling in update_refcount

[Kevin Wolf]

If update_refcount fails, try to undo any changes made so far to avoid
inconsistencies in the image file.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qcow2-refcount.c |   32 +++++++++++++++++++++++++-------
 1 files changed, 25 insertions(+), 7 deletions(-)

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 6f449c6..a84620f 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -278,6 +278,7 @@ static int update_refcount(BlockDriverState *bs,
     int64_t refcount_block_offset = 0;
     int64_t table_index = -1, old_table_index;
     int first_index = -1, last_index = -1;
+    int ret;
 
 #ifdef DEBUG_ALLOC2
     printf("update_refcount: offset=%" PRId64 " size=%" PRId64 " addend=%d\n",
@@ -292,6 +293,7 @@ static int update_refcount(BlockDriverState *bs,
     {
         int block_index, refcount;
         int64_t cluster_index = cluster_offset >> s->cluster_bits;
+        int64_t new_block;
 
         /* Only write refcount block to disk when we are done with it */
         old_table_index = table_index;
@@ -309,10 +311,12 @@ static int update_refcount(BlockDriverState *bs,
         }
 
         /* Load the refcount block and allocate it if needed */
-        refcount_block_offset = alloc_refcount_block(bs, cluster_index);
-        if (refcount_block_offset < 0) {
-            return refcount_block_offset;
+        new_block = alloc_refcount_block(bs, cluster_index);
+        if (new_block < 0) {
+            ret = new_block;
+            goto fail;
         }
+        refcount_block_offset = new_block;
 
         /* we can update the count and save it */
         block_index = cluster_index &
@@ -326,24 +330,38 @@ static int update_refcount(BlockDriverState *bs,
 
         refcount = be16_to_cpu(s->refcount_block_cache[block_index]);
         refcount += addend;
-        if (refcount < 0 || refcount > 0xffff)
-            return -EINVAL;
+        if (refcount < 0 || refcount > 0xffff) {
+            ret = -EINVAL;
+            goto fail;
+        }
         if (refcount == 0 && cluster_index < s->free_cluster_index) {
             s->free_cluster_index = cluster_index;
         }
         s->refcount_block_cache[block_index] = cpu_to_be16(refcount);
     }
 
+    ret = 0;
+fail:
+
     /* Write last changed block to disk */
     if (refcount_block_offset != 0) {
         if (write_refcount_block_entries(s, refcount_block_offset,
             first_index, last_index) < 0)
         {
-            return -EIO;
+            return ret < 0 ? ret : -EIO;
         }
     }
 
-    return 0;
+    /*
+     * Try do undo any updates if an error is returned (This may succeed in
+     * some cases like ENOSPC for allocating a new refcount block)
+     */
+    if (ret < 0) {
+        int dummy;
+        dummy = update_refcount(bs, offset, cluster_offset - offset, -addend);
+    }
+
+    return ret;
 }
 
 /* addend must be 1 or -1 */
-- 
1.6.2.5

Re: [Qemu-devel] [PATCH 07/10] qcow2: Improve error handling in update_refcount

[Christoph Hellwig]

On Mon, Jan 18, 2010 at 01:11:33PM +0100, Kevin Wolf wrote:
> If update_refcount fails, try to undo any changes made so far to avoid
> inconsistencies in the image file.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/qcow2-refcount.c |   32 +++++++++++++++++++++++++-------
>  1 files changed, 25 insertions(+), 7 deletions(-)
> 

> +    /*
> +     * Try do undo any updates if an error is returned (This may succeed in
> +     * some cases like ENOSPC for allocating a new refcount block)
> +     */
> +    if (ret < 0) {
> +        int dummy;
> +        dummy = update_refcount(bs, offset, cluster_offset - offset, -addend);

So we recursively call into update_refcount here.  What happens an error
causes all updates to fail?

Re: [Qemu-devel] [PATCH 07/10] qcow2: Improve error handling in update_refcount

[Kevin Wolf]

Am 19.01.2010 19:51, schrieb Christoph Hellwig:
> On Mon, Jan 18, 2010 at 01:11:33PM +0100, Kevin Wolf wrote:
>> If update_refcount fails, try to undo any changes made so far to avoid
>> inconsistencies in the image file.
>>
>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>> ---
>>  block/qcow2-refcount.c |   32 +++++++++++++++++++++++++-------
>>  1 files changed, 25 insertions(+), 7 deletions(-)
>>
> 
>> +    /*
>> +     * Try do undo any updates if an error is returned (This may succeed in
>> +     * some cases like ENOSPC for allocating a new refcount block)
>> +     */
>> +    if (ret < 0) {
>> +        int dummy;
>> +        dummy = update_refcount(bs, offset, cluster_offset - offset, -addend);
> 
> So we recursively call into update_refcount here.  What happens an error
> causes all updates to fail?

We're only reverting writes that have succeeded. If reverting fails,
there are no successful writes and update_refcount will be called with a
length of 0 (which never fails). In the worst case we're only reverting
one cluster less with each recursive call - however, I don't think
that's a realistic scenario.

Kevin

[Qemu-devel] [PATCH 08/10] qcow2: Allow updating no refcounts

[Kevin Wolf]

There's absolutely no problem with updating the refcounts of 0 clusters.
At least snapshot code is doing this and would fail once the result of
update_refcount isn't ignored any more.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qcow2-refcount.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index a84620f..3dfadf1 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -284,8 +284,10 @@ static int update_refcount(BlockDriverState *bs,
     printf("update_refcount: offset=%" PRId64 " size=%" PRId64 " addend=%d\n",
            offset, length, addend);
 #endif
-    if (length <= 0)
+    if (length < 0) {
         return -EINVAL;
+    }
+
     start = offset & ~(s->cluster_size - 1);
     last = (offset + length - 1) & ~(s->cluster_size - 1);
     for(cluster_offset = start; cluster_offset <= last;
-- 
1.6.2.5

Re: [Qemu-devel] [PATCH 08/10] qcow2: Allow updating no refcounts

[Christoph Hellwig]

>  #endif
> -    if (length <= 0)
> +    if (length < 0) {
>          return -EINVAL;
> +    }
> +
>      start = offset & ~(s->cluster_size - 1);
>      last = (offset + length - 1) & ~(s->cluster_size - 1);
>      for(cluster_offset = start; cluster_offset <= last;

So for legnth = 0, last will equal start and we'll never go through
the loop.  But should we really bother with all the other work in the
function or just return 0 early on?

Re: [Qemu-devel] [PATCH 08/10] qcow2: Allow updating no refcounts

[Kevin Wolf]

Am 19.01.2010 19:53, schrieb Christoph Hellwig:
>>  #endif
>> -    if (length <= 0)
>> +    if (length < 0) {
>>          return -EINVAL;
>> +    }
>> +
>>      start = offset & ~(s->cluster_size - 1);
>>      last = (offset + length - 1) & ~(s->cluster_size - 1);
>>      for(cluster_offset = start; cluster_offset <= last;
> 
> So for legnth = 0, last will equal start and we'll never go through
> the loop.  But should we really bother with all the other work in the
> function or just return 0 early on?

I'm not a big fan of special-casing for no real reason ("all the other
work" basically is calculating start and last and skipping two ifs - and
length = 0 is an unusual case anyway), but if you really mind we can
change it.

Kevin

[Qemu-devel] [PATCH 09/10] qcow2: Don't ignore update_refcount return value

[Kevin Wolf]

update_refcount can return errors that need to be handled by the callers.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qcow2-refcount.c |   31 +++++++++++++++++++++++--------
 1 files changed, 23 insertions(+), 8 deletions(-)

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 3dfadf1..b9a825b 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -269,9 +269,8 @@ static int write_refcount_block_entries(BDRVQcowState *s,
 }
 
 /* XXX: cache several refcount block clusters ? */
-static int update_refcount(BlockDriverState *bs,
-                            int64_t offset, int64_t length,
-                            int addend)
+static int QEMU_WARN_UNUSED_RESULT update_refcount(BlockDriverState *bs,
+    int64_t offset, int64_t length, int addend)
 {
     BDRVQcowState *s = bs->opaque;
     int64_t start, last, cluster_offset;
@@ -413,9 +412,13 @@ retry:
 int64_t qcow2_alloc_clusters(BlockDriverState *bs, int64_t size)
 {
     int64_t offset;
+    int ret;
 
     offset = alloc_clusters_noref(bs, size);
-    update_refcount(bs, offset, size, 1);
+    ret = update_refcount(bs, offset, size, 1);
+    if (ret < 0) {
+        return ret;
+    }
     return offset;
 }
 
@@ -462,7 +465,13 @@ int64_t qcow2_alloc_bytes(BlockDriverState *bs, int size)
 void qcow2_free_clusters(BlockDriverState *bs,
                           int64_t offset, int64_t size)
 {
-    update_refcount(bs, offset, size, -1);
+    int ret;
+
+    ret = update_refcount(bs, offset, size, -1);
+    if (ret < 0) {
+        fprintf(stderr, "qcow2_free_clusters failed: %s\n", strerror(-ret));
+        abort();
+    }
 }
 
 /*
@@ -571,9 +580,15 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs,
                     if (offset & QCOW_OFLAG_COMPRESSED) {
                         nb_csectors = ((offset >> s->csize_shift) &
                                        s->csize_mask) + 1;
-                        if (addend != 0)
-                            update_refcount(bs, (offset & s->cluster_offset_mask) & ~511,
-                                            nb_csectors * 512, addend);
+                        if (addend != 0) {
+                            int ret;
+                            ret = update_refcount(bs,
+                                (offset & s->cluster_offset_mask) & ~511,
+                                nb_csectors * 512, addend);
+                            if (ret < 0) {
+                                goto fail;
+                            }
+                        }
                         /* compressed clusters are never modified */
                         refcount = 2;
                     } else {
-- 
1.6.2.5

Re: [Qemu-devel] [PATCH 09/10] qcow2: Don't ignore update_refcount return value

[Christoph Hellwig]

Looks good,


Reviewed-by: Christoph Hellwig <hch@lst.de>

[Qemu-devel] [PATCH 10/10] qcow2: Don't ignore qcow2_alloc_clusters return value

[Kevin Wolf]

Now that qcow2_alloc_clusters can return error codes, we must handle them in
the callers of qcow2_alloc_clusters.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qcow2-cluster.c  |   18 ++++++++++++++++--
 block/qcow2-refcount.c |    6 ++++++
 block/qcow2-snapshot.c |   11 ++++++++++-
 3 files changed, 32 insertions(+), 3 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 3e877a4..5a63918 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -33,7 +33,7 @@ int qcow2_grow_l1_table(BlockDriverState *bs, int min_size)
     BDRVQcowState *s = bs->opaque;
     int new_l1_size, new_l1_size2, ret, i;
     uint64_t *new_l1_table;
-    uint64_t new_l1_table_offset;
+    int64_t new_l1_table_offset;
     uint8_t data[12];
 
     new_l1_size = s->l1_size;
@@ -55,6 +55,9 @@ int qcow2_grow_l1_table(BlockDriverState *bs, int min_size)
 
     /* write new table (align to cluster) */
     new_l1_table_offset = qcow2_alloc_clusters(bs, new_l1_size2);
+    if (new_l1_table_offset < 0) {
+        return new_l1_table_offset;
+    }
 
     for(i = 0; i < s->l1_size; i++)
         new_l1_table[i] = cpu_to_be64(new_l1_table[i]);
@@ -220,6 +223,9 @@ static uint64_t *l2_allocate(BlockDriverState *bs, int l1_index)
     /* allocate a new l2 entry */
 
     l2_offset = qcow2_alloc_clusters(bs, s->l2_size * sizeof(uint64_t));
+    if (l2_offset < 0) {
+        return NULL;
+    }
 
     /* update the L1 entry */
 
@@ -567,6 +573,10 @@ uint64_t qcow2_alloc_compressed_cluster_offset(BlockDriverState *bs,
         qcow2_free_any_clusters(bs, cluster_offset, 1);
 
     cluster_offset = qcow2_alloc_bytes(bs, compressed_size);
+    if (cluster_offset < 0) {
+        return 0;
+    }
+
     nb_csectors = ((cluster_offset + compressed_size - 1) >> 9) -
                   (cluster_offset >> 9);
 
@@ -698,7 +708,8 @@ uint64_t qcow2_alloc_cluster_offset(BlockDriverState *bs,
 {
     BDRVQcowState *s = bs->opaque;
     int l2_index, ret;
-    uint64_t l2_offset, *l2_table, cluster_offset;
+    uint64_t l2_offset, *l2_table;
+    int64_t cluster_offset;
     unsigned int nb_clusters, i = 0;
     QCowL2Meta *old_alloc;
 
@@ -792,6 +803,9 @@ uint64_t qcow2_alloc_cluster_offset(BlockDriverState *bs,
     /* allocate a new cluster */
 
     cluster_offset = qcow2_alloc_clusters(bs, nb_clusters * s->cluster_size);
+    if (cluster_offset < 0) {
+        return cluster_offset;
+    }
 
     /* save info needed for meta data update */
     m->offset = offset;
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index b9a825b..890de7a 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -433,6 +433,9 @@ int64_t qcow2_alloc_bytes(BlockDriverState *bs, int size)
     assert(size > 0 && size <= s->cluster_size);
     if (s->free_byte_offset == 0) {
         s->free_byte_offset = qcow2_alloc_clusters(bs, s->cluster_size);
+        if (s->free_byte_offset < 0) {
+            return s->free_byte_offset;
+        }
     }
  redo:
     free_in_cluster = s->cluster_size -
@@ -448,6 +451,9 @@ int64_t qcow2_alloc_bytes(BlockDriverState *bs, int size)
             update_cluster_refcount(bs, offset >> s->cluster_bits, 1);
     } else {
         offset = qcow2_alloc_clusters(bs, s->cluster_size);
+        if (offset < 0) {
+            return offset;
+        }
         cluster_offset = s->free_byte_offset & ~(s->cluster_size - 1);
         if ((cluster_offset + s->cluster_size) == offset) {
             /* we are lucky: contiguous data */
diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index d63c7e1..8ddaea2 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -139,6 +139,9 @@ static int qcow_write_snapshots(BlockDriverState *bs)
 
     snapshots_offset = qcow2_alloc_clusters(bs, snapshots_size);
     offset = snapshots_offset;
+    if (offset < 0) {
+        return offset;
+    }
 
     for(i = 0; i < s->nb_snapshots; i++) {
         sn = s->snapshots + i;
@@ -235,6 +238,7 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
     QCowSnapshot *snapshots1, sn1, *sn = &sn1;
     int i, ret;
     uint64_t *l1_table = NULL;
+    int64_t l1_table_offset;
 
     memset(sn, 0, sizeof(*sn));
 
@@ -263,7 +267,12 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
         goto fail;
 
     /* create the L1 table of the snapshot */
-    sn->l1_table_offset = qcow2_alloc_clusters(bs, s->l1_size * sizeof(uint64_t));
+    l1_table_offset = qcow2_alloc_clusters(bs, s->l1_size * sizeof(uint64_t));
+    if (l1_table_offset < 0) {
+        goto fail;
+    }
+
+    sn->l1_table_offset = l1_table_offset;
     sn->l1_size = s->l1_size;
 
     if (s->l1_size != 0) {
-- 
1.6.2.5

Re: [Qemu-devel] [PATCH 10/10] qcow2: Don't ignore qcow2_alloc_clusters return value

[Christoph Hellwig]

On Mon, Jan 18, 2010 at 01:11:36PM +0100, Kevin Wolf wrote:
> @@ -55,6 +55,9 @@ int qcow2_grow_l1_table(BlockDriverState *bs, int min_size)
>  
>      /* write new table (align to cluster) */
>      new_l1_table_offset = qcow2_alloc_clusters(bs, new_l1_size2);
> +    if (new_l1_table_offset < 0) {
> +        return new_l1_table_offset;
> +    }

I think the error return needs to free new_l1_table first.

Re: [Qemu-devel] [PATCH 10/10] qcow2: Don't ignore qcow2_alloc_clusters return value

[Kevin Wolf]

Am 19.01.2010 19:58, schrieb Christoph Hellwig:
> On Mon, Jan 18, 2010 at 01:11:36PM +0100, Kevin Wolf wrote:
>> @@ -55,6 +55,9 @@ int qcow2_grow_l1_table(BlockDriverState *bs, int min_size)
>>  
>>      /* write new table (align to cluster) */
>>      new_l1_table_offset = qcow2_alloc_clusters(bs, new_l1_size2);
>> +    if (new_l1_table_offset < 0) {
>> +        return new_l1_table_offset;
>> +    }
> 
> I think the error return needs to free new_l1_table first.

Right. Actually, I have found this bug already and included the fix in
patch 11/10 which contains the fix to the problem you found with patch 1
(however, I wasn't aware any more that I introduced it myself). Maybe I
should better respin the series instead of posting patch 11 on top.

Kevin