| Message ID | 1418647857-3589-8-git-send-email-mreitz@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 <mreitz@redhat.com> > Reviewed-by: Eric Blake <eblake@redhat.com> > Reviewed-by: Stefan Hajnoczi <stefanha@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.) > + } > + } > } 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). Why can't we keep offset where it was used and only assign new_cluster additionally so we can do the cleanup? > cluster_offset = start_of_cluster(s, s->free_byte_offset); > - if ((cluster_offset + s->cluster_size) == offset) { > + if ((cluster_offset + s->cluster_size) == new_cluster) { > /* we are lucky: contiguous data */ > offset = s->free_byte_offset; > - qcow2_update_cluster_refcount(bs, offset >> s->cluster_bits, 1, > - false, QCOW2_DISCARD_NEVER); > + ret = qcow2_update_cluster_refcount(bs, offset >> s->cluster_bits, > + 1, false, QCOW2_DISCARD_NEVER); > + if (ret < 0) { > + qcow2_free_clusters(bs, new_cluster, s->cluster_size, > + QCOW2_DISCARD_NEVER); > + return ret; > + } > s->free_byte_offset += size; > } else { > - s->free_byte_offset = offset; > + s->free_byte_offset = new_cluster; > goto redo; > } > } Kevin
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 <mreitz@redhat.com> >> Reviewed-by: Eric Blake <eblake@redhat.com> >> Reviewed-by: Stefan Hajnoczi <stefanha@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). > Why can't we keep offset where it was used and only assign new_cluster > additionally so we can do the cleanup? Why should we? If I were a reader of this code, I think it would confuse me to have two variables holding the same value but for some reason only using @offset, but once it's been overwritten, suddenly using @new_cluster (whereas one could just use @new_cluster everywhere, and then writing @offset would be superfluous). Also, the idea was having @offset hold the offset of where the compressed data is to be stored, whereas @new_cluster is just the offset of a new cluster, but not necessarily the offset where compressed data will be stored (it won't be if (cluster_offset + s->cluster_size) == new_cluster). Max >> cluster_offset = start_of_cluster(s, s->free_byte_offset); >> - if ((cluster_offset + s->cluster_size) == offset) { >> + if ((cluster_offset + s->cluster_size) == new_cluster) { >> /* we are lucky: contiguous data */ >> offset = s->free_byte_offset; >> - qcow2_update_cluster_refcount(bs, offset >> s->cluster_bits, 1, >> - false, QCOW2_DISCARD_NEVER); >> + ret = qcow2_update_cluster_refcount(bs, offset >> s->cluster_bits, >> + 1, false, QCOW2_DISCARD_NEVER); >> + if (ret < 0) { >> + qcow2_free_clusters(bs, new_cluster, s->cluster_size, >> + QCOW2_DISCARD_NEVER); >> + return ret; >> + } >> s->free_byte_offset += size; >> } else { >> - s->free_byte_offset = offset; >> + s->free_byte_offset = new_cluster; >> goto redo; >> } >> } > Kevin
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 <mreitz@redhat.com> > >>Reviewed-by: Eric Blake <eblake@redhat.com> > >>Reviewed-by: Stefan Hajnoczi <stefanha@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
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; + } + } } 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; } cluster_offset = start_of_cluster(s, s->free_byte_offset); - if ((cluster_offset + s->cluster_size) == offset) { + if ((cluster_offset + s->cluster_size) == new_cluster) { /* we are lucky: contiguous data */ offset = s->free_byte_offset; - qcow2_update_cluster_refcount(bs, offset >> s->cluster_bits, 1, - false, QCOW2_DISCARD_NEVER); + ret = qcow2_update_cluster_refcount(bs, offset >> s->cluster_bits, + 1, false, QCOW2_DISCARD_NEVER); + if (ret < 0) { + qcow2_free_clusters(bs, new_cluster, s->cluster_size, + QCOW2_DISCARD_NEVER); + return ret; + } s->free_byte_offset += size; } else { - s->free_byte_offset = offset; + s->free_byte_offset = new_cluster; goto redo; } }