Am 21.10.2014 um 11:52 hat Max Reitz geschrieben: > On 2014-10-21 at 11:31, Kevin Wolf wrote: > >Am 20.10.2014 um 16:35 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 <mre...@redhat.com> > >>--- > >> block/qcow2-refcount.c | 296 > >> ++++++++++++++++++++++++++++++++++++++++++++++++- > >> 1 file changed, 293 insertions(+), 3 deletions(-)
> >>+/* > >>+ * Creates a new refcount structure based solely on the in-memory > >>information > >>+ * given through *refcount_table. All necessary allocations will be > >>reflected > >>+ * in that array. > >>+ * > >>+ * On success, the old refcount structure is leaked (it will be covered by > >>the > >>+ * new refcount structure). > >>+ */ > >>+static int rebuild_refcount_structure(BlockDriverState *bs, > >>+ BdrvCheckResult *res, > >>+ uint16_t **refcount_table, > >>+ int64_t *nb_clusters) > >>+{ > >>+ BDRVQcowState *s = bs->opaque; > >>+ int64_t first_free_cluster = 0, reftable_offset = -1, cluster = 0; > >>+ int64_t refblock_offset, refblock_start, refblock_index; > >>+ uint32_t reftable_size = 0; > >>+ uint64_t *reftable = NULL; > >refcount_table and reftable? Seriously? > > Reviewing would have been too easy otherwise. *cough* > > One option is s/refcount_table/imrt/. I like it more than the second > option, but it simply goes against the current naming convention. > > The other option would be s/reftable/on_disk_reftable/, following > the convention used in the next line. The second option is probably more consistent with the rest (and less cryptic). > >>+ ret = qcow2_pre_write_overlap_check(bs, 0, refblock_offset, > >>+ s->cluster_size); > >>+ if (ret < 0) { > >>+ fprintf(stderr, "ERROR writing refblock: %s\n", > >>strerror(-ret)); > >>+ goto fail; > >>+ } > >>+ > >>+ on_disk_refblock = g_malloc0(s->cluster_size); > >qemu_blockalign? > > Meh, I think I have to write a qemu_blockalign0() some time... > > If I do so, it's one more patch you have to review; if I just use > qemu_blockalign(), I find the memset() directly afterwards rather > ugly, but would be fine with it. I guess it's up to you, then. A qemu_blockalign0() sounds easy enough to review. It's not the number of patches that makes review hard, it's their content. Anyway, you're the patch author, it's your choice. > >>+ ret = qcow2_pre_write_overlap_check(bs, 0, reftable_offset, > >>+ reftable_size * sizeof(uint64_t)); > >>+ if (ret < 0) { > >>+ fprintf(stderr, "ERROR writing reftable: %s\n", strerror(-ret)); > >>+ goto fail; > >>+ } > >>+ > >>+ ret = bdrv_write(bs->file, reftable_offset / BDRV_SECTOR_SIZE, > >>+ (void *)reftable, > >>+ reftable_size * sizeof(uint64_t) / BDRV_SECTOR_SIZE); > >Why not bdrv_pwrite when you only have byte offset and length? > > I don't like pwrite probably only because it takes an int byte > length. reftable_size * sizeof(uint64_t) should be well below > INT_MAX, but I don't see why we should use pwrite if we are writing > full sectors to a sector-aligned offset. Good point, we should look into fixing the type of the length argument sometime. Other than that, I'd prefer bdrv_pread/pwrite because it doesn't require unit conversion here, and later in block.c back (bdrv_co_do_pwritev is the native block layer interface, and it's byte based). bdrv_write() goes through more emulation layers in block.c. Kevin