From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37924) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WPegF-00015r-I4 for qemu-devel@nongnu.org; Mon, 17 Mar 2014 17:02:49 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WPeg9-0002tK-I4 for qemu-devel@nongnu.org; Mon, 17 Mar 2014 17:02:43 -0400 Received: from mx1.redhat.com ([209.132.183.28]:11669) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WPeg9-0002ss-AK for qemu-devel@nongnu.org; Mon, 17 Mar 2014 17:02:37 -0400 Message-ID: <532762E3.9080907@redhat.com> Date: Mon, 17 Mar 2014 22:02:27 +0100 From: Max Reitz MIME-Version: 1.0 References: <1394916954-21774-1-git-send-email-mreitz@redhat.com> <20140315231604.GA3067@irqsave.net> In-Reply-To: <20140315231604.GA3067@irqsave.net> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH] qcow2: Fix fail path in realloc_refcount_block() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?ISO-8859-1?Q?Beno=EEt_Canet?= Cc: Kevin Wolf , Laszlo Ersek , qemu-devel@nongnu.org, Stefan Hajnoczi On 16.03.2014 00:16, Beno=EEt Canet wrote: > The Saturday 15 Mar 2014 =E0 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_offs= et >> 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". Hm, I'm not sure. Currently, it simply cannot fail. The assertion is=20 there basically only for future code changes and to indicate to=20 developers that the function is really known to succeed. I'll try to make the sentence more specific. >> (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 >> Suggested-by: Laszlo Ersek >> --- >> 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(BlockDri= verState *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. I'll try to reshape it into a nicer form of nested labels. >> - 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_bl= ock); >> - } else { >> - ret =3D qcow2_cache_put(bs, s->refcount_block_cache, &ref= count_block); >> - } >> + /* This should never fail, as it would only do so if the give= n refcount >> + * block cannot be found in the cache. As this is impossible = as long as >> + * there are no bugs, assert the success. */ >> + int tmp =3D qcow2_cache_put(bs, s->refcount_block_cache, &ref= count_block); >> + assert(tmp =3D=3D 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 b= y its > * offset in the image file) and copies the current content there. Thi= s 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 reftabl= e_index, > uint64_t offset) > > > So why are we returning new_offset on success ? Because the comment is wrong. I'll fix it. ;-) Thanks, Max >> } >> >> -- >> 1.9.0 >> >>