qcow2: More error handling fixes
cover qcow2: More error handling fixes
Three more cases of ignored or mutated error codes.
Kevin Wolf (3):
qcow2: Allow get_refcount to return errors
qcow2: Allow alloc_clusters_noref to return errors
qcow2: Return real error code in load_refcount_block
block/qcow2-refcount.c | 70 +++++++++++++++++++++++++++++++++++++++++------- 1 files changed, 60 insertions(+), 10 deletions(-)
PATCH 1/3 qcow2: Allow get_refcount to return errors
get_refcount might need to load a refcount block from disk, so errors may happen. Return the error code instead of assuming a refcount of 1 and change the callers to respect error return values.
--- block/qcow2-refcount.c | 41 +++++++++++++++++++++++++++++++++++++---- 1 files changed, 37 insertions(+), 4 deletions(-)
block/qcow2-refcount.c +37 -4
@@ -105,11 +105,17 @@ static int load_refcount_block(BlockDriverState *bs,return 0 ; } +/* +* Returns the refcount of the cluster given by its index. Any non-negative +* return value is the refcount of the cluster, negative values are -errno +* and indicate an error. +*/ static int get_refcount (BlockDriverState *bs,int64_t cluster_index) {BDRVQcowState *s = bs->opaque;int refcount_table_index, block_index;int64_t refcount_block_offset; +int ret; refcount_table_index = cluster_index >> (s->cluster_bits -REFCOUNT_SHIFT );if (refcount_table_index >= s->refcount_table_size)
@@ -119,8 +125,10 @@ static int get_refcount(BlockDriverState *bs, int64_t cluster_index)return 0 ;if (refcount_block_offset != s->refcount_block_cache_offset) {/* better than nothing: return allocated if read error */ -if (load_refcount_block (bs, refcount_block_offset) <0 ) -return 1 ; + ret =load_refcount_block (bs, refcount_block_offset); +if (ret <0 ) { +return ret; + } } block_index = cluster_index & ((1 << (s->cluster_bits -REFCOUNT_SHIFT )) -1 );
@@ -538,7 +546,13 @@ fail:return ret; } -/* addend must be 1 or -1 */ +/* +* Increases or decreases the refcount of a given cluster by one. +* addend must be 1 or -1. +* +* If the return value is non-negative, it is the new refcount of the cluster. +* If it is negative, it is -errno and indicates an error. +*/ static int update_cluster_refcount (BlockDriverState *bs,int64_t cluster_index,int addend)
@@ -779,6 +793,10 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs, }else { refcount =get_refcount (bs, offset >> s->cluster_bits); } + +if (refcount <0 ) { + goto fail; + } }if (refcount ==1 ) {
@@ -801,7 +819,9 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs, }else { refcount =get_refcount (bs, l2_offset >> s->cluster_bits); } -if (refcount ==1 ) { +if (refcount <0 ) { + goto fail; + }else if (refcount ==1 ) { l2_offset |=QCOW_OFLAG_COPIED ; }if (l2_offset != old_l2_offset) {
@@ -934,6 +954,10 @@ static int check_refcounts_l2(BlockDriverState *bs,uint64_t entry = offset; offset &= ~QCOW_OFLAG_COPIED ; refcount =get_refcount (bs, offset >> s->cluster_bits); +if (refcount <0 ) { +fprintf (stderr,"Can't get refcount for offset %" + PRIx64": %s\n" , entry,strerror (-refcount)); + }if ((refcount ==1 ) != ((entry &QCOW_OFLAG_COPIED ) !=0 )) {fprintf (stderr,"ERROR OFLAG_COPIED: offset=%" PRIx64" refcount=%d\n" , entry, refcount);
@@ -1011,6 +1035,10 @@ static int check_refcounts_l1(BlockDriverState *bs,if (check_copied) { refcount =get_refcount (bs, (l2_offset & ~QCOW_OFLAG_COPIED ) >> s->cluster_bits); +if (refcount <0 ) { +fprintf (stderr,"Can't get refcount for l2_offset %" + PRIx64": %s\n" , l2_offset,strerror (-refcount)); + }if ((refcount ==1 ) != ((l2_offset &QCOW_OFLAG_COPIED ) !=0 )) {fprintf (stderr,"ERROR OFLAG_COPIED: l2_offset=%" PRIx64" refcount=%d\n" , l2_offset, refcount);
@@ -1118,6 +1146,11 @@ int qcow2_check_refcounts(BlockDriverState *bs)/* compare ref counts */ for (i =0 ; i < nb_clusters; i++) { refcount1 =get_refcount (bs, i); +if (refcount1 <0 ) { +fprintf (stderr,"Can't get refcount for cluster %d: %s\n" , + i,strerror (-refcount1)); + } + refcount2 = refcount_table[i];if (refcount1 != refcount2) { fprintf(stderr, "ERROR cluster %d refcount=%d reference=%d\n",
Signature · 1 line
1.6.6.1
PATCH 2/3 qcow2: Allow alloc_clusters_noref to return errors
In reply to Kevin WolfCurrently it would consider blocks for which get_refcount fails used. However, it's unlikely that get_refcount would succeed for the next cluster, so it's not really helpful. Return an error instead.
--- block/qcow2-refcount.c | 18 +++++++++++++++--- 1 files changed, 15 insertions(+), 3 deletions(-)
block/qcow2-refcount.c +15 -3
@@ -228,7 +228,10 @@ static int64_t alloc_refcount_block(BlockDriverState *bs, int64_t cluster_index) }/* Allocate the refcount block itself and mark it as used */ -uint64_t new_block =alloc_clusters_noref (bs, s->cluster_size); +int64_t new_block =alloc_clusters_noref (bs, s->cluster_size); +if (new_block <0 ) { +return new_block; + }#ifdef DEBUG_ALLOC2 fprintf(stderr, "qcow2: Allocate refcount block %d for %" PRIx64
@@ -579,14 +582,19 @@ static int update_cluster_refcount(BlockDriverState *bs,static int64_t alloc_clusters_noref (BlockDriverState *bs,int64_t size) {BDRVQcowState *s = bs->opaque; -int i, nb_clusters; +int i, nb_clusters, refcount; nb_clusters =size_to_clusters (s, size); retry:for (i =0 ; i < nb_clusters; i++) {int64_t next_cluster_index = s->free_cluster_index++; -if (get_refcount (bs, next_cluster_index) !=0 ) + refcount =get_refcount (bs, next_cluster_index); + +if (refcount <0 ) { +return refcount; + }else if (refcount !=0 ) { goto retry; + } }#ifdef DEBUG_ALLOC2 printf("alloc_clusters: size=%" PRId64 " -> %" PRId64 "\n",
@@ -603,6 +611,10 @@ int64_t qcow2_alloc_clusters(BlockDriverState *bs, int64_t size)BLKDBG_EVENT (bs->file,BLKDBG_CLUSTER_ALLOC ); offset =alloc_clusters_noref (bs, size); +if (offset <0 ) { +return offset; + } + ret =update_refcount (bs, offset, size,1 );if (ret <0 ) {return ret;
Signature · 1 line
1.6.6.1
PATCH 3/3 qcow2: Return real error code in load_refcount_block
In reply to Kevin WolfThis fixes load_refcount_block which completely ignored the return value of write_refcount_block and always returned -EIO for bdrv_pwrite failure.
--- block/qcow2-refcount.c | 11 ++++++++--- 1 files changed, 8 insertions(+), 3 deletions(-)
block/qcow2-refcount.c +8 -3
@@ -93,14 +93,19 @@ static int load_refcount_block(BlockDriverState *bs,int ret;if (cache_refcount_updates) { -write_refcount_block (bs); + ret =write_refcount_block (bs); +if (ret <0 ) { +return ret; + } }BLKDBG_EVENT (bs->file,BLKDBG_REFBLOCK_LOAD ); ret =bdrv_pread (bs->file, refcount_block_offset, s->refcount_block_cache, s->cluster_size); -if (ret != s->cluster_size) -return -EIO ; +if (ret <0 ) { +return ret; + } + s->refcount_block_cache_offset = refcount_block_offset;return 0 ; }
Signature · 1 line
1.6.6.1