From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41124) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XqHtA-00068h-Pu for qemu-devel@nongnu.org; Mon, 17 Nov 2014 03:42:30 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XqHt4-0006JA-Lv for qemu-devel@nongnu.org; Mon, 17 Nov 2014 03:42:24 -0500 Received: from mx1.redhat.com ([209.132.183.28]:60888) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XqHt4-0006IS-FV for qemu-devel@nongnu.org; Mon, 17 Nov 2014 03:42:18 -0500 Message-ID: <5469B4E3.20809@redhat.com> Date: Mon, 17 Nov 2014 09:42:11 +0100 From: Max Reitz MIME-Version: 1.0 References: <1415970374-16811-1-git-send-email-mreitz@redhat.com> <1415970374-16811-8-git-send-email-mreitz@redhat.com> <54678719.40208@redhat.com> In-Reply-To: <54678719.40208@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 07/21] qcow2: Helper function for refcount modification 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-15 at 18:02, Eric Blake wrote: > On 11/14/2014 06:06 AM, Max Reitz wrote: >> Since refcounts do not always have to be a uint16_t, all refcount blocks >> and arrays in memory should not have a specific type (thus they become >> pointers to void) and for accessing them, two helper functions are used >> (a getter and a setter). Those functions are called indirectly through >> function pointers in the BDRVQcowState so they may later be exchanged >> for different refcount orders. >> >> Signed-off-by: Max Reitz >> --- >> block/qcow2-refcount.c | 128 ++++++++++++++++++++++++++++++------------------- >> block/qcow2.h | 8 ++++ >> 2 files changed, 87 insertions(+), 49 deletions(-) >> >> @@ -1216,7 +1249,7 @@ enum { >> * error occurred. >> */ >> static int check_refcounts_l2(BlockDriverState *bs, BdrvCheckResult *res, >> - uint16_t **refcount_table, int64_t *refcount_table_size, int64_t l2_offset, >> + void **refcount_table, int64_t *refcount_table_size, int64_t l2_offset, >> int flags) >> { > Might be worth fixing the indentation here in addition to all the other > places you adjusted. But that's minor. > >> @@ -1933,17 +1967,13 @@ write_refblocks: >> goto fail; >> } >> >> - on_disk_refblock = qemu_blockalign0(bs->file, s->cluster_size); >> - for (i = 0; i < s->refcount_block_size && >> - refblock_start + i < *nb_clusters; i++) >> - { >> - on_disk_refblock[i] = >> - cpu_to_be16((*refcount_table)[refblock_start + i]); >> - } >> + /* The size of *refcount_table is always cluster-aligned, therefore the >> + * write operation will not overflow */ >> + on_disk_refblock = (void *)((uintptr_t)*refcount_table + >> + (refblock_index << s->refcount_block_bits)); > Here is where you are relying on the guarantee that you added in 6/21, > which is why I ask for that one to mention it. > > Nice reduction of a bounce buffer, by the way :) Worth mentioning in > the commit message as an intentional part of this commit? Why not. >> @@ -2087,7 +2117,7 @@ int qcow2_check_refcounts(BlockDriverState *bs, BdrvCheckResult *res, >> /* Because the old reftable has been exchanged for a new one the >> * references have to be recalculated */ >> rebuild = false; >> - memset(refcount_table, 0, nb_clusters * sizeof(uint16_t)); >> + memset(refcount_table, 0, nb_clusters * s->refcount_bits / 8); > Phew; we're safe that this won't overflow; and good that you do the * > first (if you did the /8 first, it would fail for sub-byte refcounts). Thanks for catching this, it is wrong (albeit it does the right thing). It should use refcount_array_byte_size(), which was in this version of the series introduced before this patch, so it's an artifact of swapping patch 6 and 7. Max > Reviewed-by: Eric Blake