Am 04.02.2015 um 16:04 hat Max Reitz geschrieben: > On 2015-02-04 at 06:40, Kevin Wolf wrote: > >Am 15.12.2014 um 13:50 hat Max Reitz geschrieben: > >>qcow2_update_cluster_refcount() may fail, and qcow2_alloc_bytes() should > >>mind that case. > >> > >>Signed-off-by: Max Reitz <mre...@redhat.com> > >>Reviewed-by: Eric Blake <ebl...@redhat.com> > >>Reviewed-by: Stefan Hajnoczi <stefa...@redhat.com> > >>--- > >> block/qcow2-refcount.c | 33 +++++++++++++++++++++------------ > >> 1 file changed, 21 insertions(+), 12 deletions(-) > >> > >>diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c > >>index 0308a7e..db81647 100644 > >>--- a/block/qcow2-refcount.c > >>+++ b/block/qcow2-refcount.c > >>@@ -778,8 +778,8 @@ int qcow2_alloc_clusters_at(BlockDriverState *bs, > >>uint64_t offset, > >> int64_t qcow2_alloc_bytes(BlockDriverState *bs, int size) > >> { > >> BDRVQcowState *s = bs->opaque; > >>- int64_t offset, cluster_offset; > >>- int free_in_cluster; > >>+ int64_t offset, cluster_offset, new_cluster; > >>+ int free_in_cluster, ret; > >> BLKDBG_EVENT(bs->file, BLKDBG_CLUSTER_ALLOC_BYTES); > >> assert(size > 0 && size <= s->cluster_size); > >>@@ -800,23 +800,32 @@ int64_t qcow2_alloc_bytes(BlockDriverState *bs, int > >>size) > >> free_in_cluster -= size; > >> if (free_in_cluster == 0) > >> s->free_byte_offset = 0; > >>- if (offset_into_cluster(s, offset) != 0) > >>- qcow2_update_cluster_refcount(bs, offset >> s->cluster_bits, 1, > >>- false, QCOW2_DISCARD_NEVER); > >>+ if (offset_into_cluster(s, offset) != 0) { > >>+ ret = qcow2_update_cluster_refcount(bs, offset >> > >>s->cluster_bits, > >>+ 1, false, > >>QCOW2_DISCARD_NEVER); > >>+ if (ret < 0) { > >>+ return ret; > >Not sure how relevant it is, but s->free_byte_offset has already been > >increased, so we're leaving sub-cluster space unused. (It's not really > >leaking as freeing all references still frees the cluster.) > > Right, will fix. > > >>+ } > >>+ } > >> } else { > >>- offset = qcow2_alloc_clusters(bs, s->cluster_size); > >>- if (offset < 0) { > >>- return offset; > >>+ new_cluster = qcow2_alloc_clusters(bs, s->cluster_size); > >>+ if (new_cluster < 0) { > >>+ return new_cluster; > >> } > >offset is the return value of this function, and now there are cases > >where it isn't set to new_cluster any more (I wonder why gcc doesn't > >warn). > > Because @offset is always set. In case the next condition is true, > it is set to s->free_byte_offset, just like it was before. In case > it isn't, s->free_byte_offset will be set to @new_cluster and the > loop will be started again (probably always resulting in size <= > free_in_cluster being true and thus @offset being set to > s->free_byte_offset).
Yes, I misread. Hooray for goto. One more reason for a cleanup of the whole function. Kevin