From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41227) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xo6x8-00009C-4r for qemu-devel@nongnu.org; Tue, 11 Nov 2014 03:37:36 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Xo6x1-0002Sa-OK for qemu-devel@nongnu.org; Tue, 11 Nov 2014 03:37:30 -0500 Received: from mx1.redhat.com ([209.132.183.28]:47048) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xo6x1-0002ST-H9 for qemu-devel@nongnu.org; Tue, 11 Nov 2014 03:37:23 -0500 Message-ID: <5461CABC.7020002@redhat.com> Date: Tue, 11 Nov 2014 09:37:16 +0100 From: Max Reitz MIME-Version: 1.0 References: <1415627159-15941-1-git-send-email-mreitz@redhat.com> <1415627159-15941-8-git-send-email-mreitz@redhat.com> <546140E1.1010802@redhat.com> In-Reply-To: <546140E1.1010802@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 07/21] qcow2: Helper for refcount array size calculation List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake , qemu-devel@nongnu.org Cc: Kevin Wolf , Peter Lieven , Stefan Hajnoczi On 2014-11-10 at 23:49, Eric Blake wrote: > On 11/10/2014 06:45 AM, Max Reitz wrote: >> Add a helper function which correctly calculates the byte size of a >> refcount array for any refcount order, and use that function. >> >> Signed-off-by: Max Reitz >> --- >> block/qcow2-refcount.c | 39 ++++++++++++++++++++++++++++----------- >> 1 file changed, 28 insertions(+), 11 deletions(-) >> >> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c >> index 16652da..cfb4807 100644 >> --- a/block/qcow2-refcount.c >> +++ b/block/qcow2-refcount.c >> @@ -1132,6 +1132,20 @@ fail: >> /* refcount checking functions */ >> >> >> +static size_t refcount_array_byte_size(BDRVQcowState *s, uint64_t entries) >> +{ >> + if (s->refcount_order < 3) { >> + /* sub-byte width */ >> + int shift = 3 - s->refcount_order; >> + return (entries + (1 << shift) - 1) >> shift; >> + } else if (s->refcount_order == 3) { >> + /* byte width */ >> + return entries; >> + } else { >> + /* multiple bytes wide */ >> + return entries << (s->refcount_order - 3); >> + } > A comment proving why this can't overflow might be nice (if I analyzed > correctly, entries will be computed by file size / clusters, and in the > worst case, the smallest cluster and largest refcount_order results in > '(size >> 9) << (6 - 3)' which is still safe). Yes, will do. >> @@ -1161,12 +1175,13 @@ static int inc_refcounts(BlockDriverState *bs, >> k = cluster_offset >> s->cluster_bits; >> if (k >= *refcount_table_size) { >> int64_t old_refcount_table_size = *refcount_table_size; >> + size_t new_byte_size; >> void *new_refcount_table; >> >> *refcount_table_size = k + 1; >> - new_refcount_table = g_try_realloc(*refcount_table, >> - *refcount_table_size * >> - s->refcount_bits / 8); >> + new_byte_size = refcount_array_byte_size(s, *refcount_table_size); >> + >> + new_refcount_table = g_try_realloc(*refcount_table, new_byte_size); > Yay - this addresses one of my possible overflow comments on 6/21. > > I wonder if the series would have less churn if you rearranged this > patch to come before 6/21. Why not, I'll add an __attribute__((used)) to it (which should be fine for the duration of a single patch). Max