The Saturday 15 Mar 2014 à 21:55:54 (+0100), Max Reitz wrote : > If qcow2_alloc_clusters() fails, new_offset and ret will both be > negative after the fail label, thus passing the first if condition and > subsequently resulting in a call of qcow2_free_clusters() with an > invalid (negative) offset parameter. Fix this by checking for new_offset > being positive instead. > > While we're at it, clean up the whole fail path. qcow2_cache_put() > actually can never fail, hence the return value can safely be ignored
I would say "actually should never fail". > (aside from asserting that it indeed did not fail). > > Furthermore, there is no reason to give QCOW2_DISCARD_ALWAYS to > qcow2_free_clusters(), a mere QCOW2_DISCARD_OTHER will suffice. > > Signed-off-by: Max Reitz <mre...@redhat.com> > Suggested-by: Laszlo Ersek <ler...@redhat.com> > --- > block/qcow2-refcount.c | 20 +++++++++++--------- > 1 file changed, 11 insertions(+), 9 deletions(-) > > diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c > index 6151148..b111319 100644 > --- a/block/qcow2-refcount.c > +++ b/block/qcow2-refcount.c > @@ -1440,20 +1440,22 @@ static int64_t > realloc_refcount_block(BlockDriverState *bs, int reftable_index, > } > > fail: I don't like the label name because its real meaning is exit: it handle both fail and regular exits. > - if (new_offset && (ret < 0)) { > - qcow2_free_clusters(bs, new_offset, s->cluster_size, > - QCOW2_DISCARD_ALWAYS); > - } > if (refcount_block) { > - if (ret < 0) { > - qcow2_cache_put(bs, s->refcount_block_cache, &refcount_block); > - } else { > - ret = qcow2_cache_put(bs, s->refcount_block_cache, > &refcount_block); > - } > + /* This should never fail, as it would only do so if the given > refcount > + * block cannot be found in the cache. As this is impossible as long > as > + * there are no bugs, assert the success. */ > + int tmp = qcow2_cache_put(bs, s->refcount_block_cache, > &refcount_block); > + assert(tmp == 0); > } > + > if (ret < 0) { > + if (new_offset > 0) { > + qcow2_free_clusters(bs, new_offset, s->cluster_size, > + QCOW2_DISCARD_OTHER); > + } > return ret; > } > + > return new_offset; The function documentation says: /* * Allocates a new cluster for the given refcount block (represented by its * offset in the image file) and copies the current content there. This function * does _not_ decrement the reference count for the currently occupied cluster. * * This function prints an informative message to stderr on error (and returns * -errno); on success, 0 is returned. */ static int64_t realloc_refcount_block(BlockDriverState *bs, int reftable_index, uint64_t offset) So why are we returning new_offset on success ? > } > > -- > 1.9.0 > >