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 <mre...@redhat.com>
---
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 <ebl...@redhat.com>
