From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37157) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XgsIs-0001i1-Ul for qemu-devel@nongnu.org; Wed, 22 Oct 2014 05:34:09 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XgsIm-00070i-Pj for qemu-devel@nongnu.org; Wed, 22 Oct 2014 05:34:02 -0400 Received: from mx1.redhat.com ([209.132.183.28]:28053) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xgs1K-0001EP-DV for qemu-devel@nongnu.org; Wed, 22 Oct 2014 05:15:54 -0400 Message-ID: <544775C4.7000409@redhat.com> Date: Wed, 22 Oct 2014 11:15:48 +0200 From: Max Reitz MIME-Version: 1.0 References: <1413965324-14541-1-git-send-email-mreitz@redhat.com> <1413965324-14541-11-git-send-email-mreitz@redhat.com> <20141022091444.GC3188@noname.str.redhat.com> In-Reply-To: <20141022091444.GC3188@noname.str.redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v7 10/13] qcow2: Rebuild refcount structure during check List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: qemu-devel@nongnu.org, Stefan Hajnoczi , =?windows-1252?Q?Beno=EEt_Canet?= On 2014-10-22 at 11:14, Kevin Wolf wrote: > Am 22.10.2014 um 10:08 hat Max Reitz geschrieben: >> The previous commit introduced the "rebuild" variable to qcow2's >> implementation of the image consistency check. Now make use of this by >> adding a function which creates a completely new refcount structure >> based solely on the in-memory information gathered before. >> >> The old refcount structure will be leaked, however. This leak will be >> dealt with in a follow-up commit. >> >> Signed-off-by: Max Reitz >> --- >> block/qcow2-refcount.c | 310 ++++++++++++++++++++++++++++++++++++++++++++++++- >> 1 file changed, 304 insertions(+), 6 deletions(-) >> >> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c >> index 101d7bb..8ca2324 100644 >> --- a/block/qcow2-refcount.c >> +++ b/block/qcow2-refcount.c >> @@ -1640,6 +1640,284 @@ static void compare_refcounts(BlockDriverState *bs, BdrvCheckResult *res, >> } >> >> /* >> + * Allocates clusters using an in-memory refcount table (IMRT) in contrast to >> + * the on-disk refcount structures. >> + * >> + * On input, *first_free_cluster tells where to start looking, and need not >> + * actually be a free cluster; the returned offset will not be before that >> + * cluster. On output, *first_free_cluster points to the first gap found, even >> + * if that gap was too small to be used as the returned offset. >> + * >> + * Note that *first_free_cluster is a cluster index whereas the return value is >> + * an offset. >> + */ >> +static int64_t alloc_clusters_imrt(BlockDriverState *bs, >> + int cluster_count, >> + uint16_t **refcount_table, >> + int64_t *imrt_nb_clusters, >> + int64_t *first_free_cluster) >> +{ >> + BDRVQcowState *s = bs->opaque; >> + int64_t cluster = *first_free_cluster, i; >> + bool first_gap = true; >> + int contiguous_free_clusters; >> + >> + /* Starting at *first_free_cluster, find a range of at least cluster_count >> + * continuously free clusters */ >> + for (contiguous_free_clusters = 0; >> + cluster < *imrt_nb_clusters && >> + contiguous_free_clusters < cluster_count; >> + cluster++) >> + { >> + if (!(*refcount_table)[cluster]) { >> + contiguous_free_clusters++; >> + if (first_gap) { >> + /* If this is the first free cluster found, update >> + * *first_free_cluster accordingly */ >> + *first_free_cluster = cluster; >> + first_gap = false; >> + } >> + } else if (contiguous_free_clusters) { >> + contiguous_free_clusters = 0; >> + } >> + } >> + >> + /* If contiguous_free_clusters is greater than zero, it contains the number >> + * of continuously free clusters until the current cluster; the first free >> + * cluster in the current "gap" is therefore >> + * cluster - contiguous_free_clusters */ >> + >> + /* If no such range could be found, grow the in-memory refcount table >> + * accordingly to append free clusters at the end of the image */ >> + if (contiguous_free_clusters < cluster_count) { >> + int64_t old_imrt_nb_clusters = *imrt_nb_clusters; >> + uint16_t *new_refcount_table; >> + >> + /* contiguous_free_clusters clusters are already empty at the image end; >> + * we need cluster_count clusters; therefore, we have to allocate >> + * cluster_count - contiguous_free_clusters new clusters at the end of >> + * the image (which is the current value of cluster; note that cluster >> + * may exceed old_imrt_nb_clusters if *first_free_cluster pointed beyond >> + * the image end) */ >> + *imrt_nb_clusters = cluster + cluster_count - contiguous_free_clusters; >> + new_refcount_table = g_try_realloc(*refcount_table, >> + *imrt_nb_clusters * >> + sizeof(**refcount_table)); >> + if (!new_refcount_table) { >> + return -ENOMEM; > Now the old refcount table is preserved, but nb_clusters has already > been updated. Smells like the next thing will be a buffer overflow, > doesn't it? If the caller continues after ENOMEM, well, yes. I'd call it his own fault, but I'll fix it of course. Max >> + } >> + *refcount_table = new_refcount_table; >> + >> + memset(*refcount_table + old_imrt_nb_clusters, 0, >> + (*imrt_nb_clusters - old_imrt_nb_clusters) * >> + sizeof(**refcount_table)); >> + } > Kevin