On Mon, Nov 26, 2012 at 02:05:02PM +0100, Benoît Canet wrote:
> +/**
> + * Read some data from the QCOW2 file
> + *
> + * @data: the buffer where the data must be stored
> + * @sector_num: the sector number to read in the QCOW2 file
> + * @nb_sectors: the number of sectors to read
> + * @ret: negative on error
Dropping s->lock is important information to document - it means things
can change by the time this function returns and the caller needs to be
prepared.
Perhaps this function can be moved to qcow2.c and given a generic name.
It does nothing dedup-specific.
> + */
> +static int qcow2_dedup_read_missing_cluster_data(BlockDriverState *bs,
> + uint8_t *data,
> + uint64_t sector_num,
> + int nb_sectors)
> +{
> + BDRVQcowState *s = bs->opaque;
> + QEMUIOVector qiov;
> + struct iovec iov;
> + int ret;
> +
> + iov.iov_len = nb_sectors * BDRV_SECTOR_SIZE;
> + iov.iov_base = data;
> + qemu_iovec_init_external(&qiov, &iov, 1);
> + qemu_co_mutex_unlock(&s->lock);
> + ret = bdrv_co_readv(bs, sector_num, nb_sectors, &qiov);
> + qemu_co_mutex_lock(&s->lock);
> + if (ret < 0) {
> + error_report("failed to read %d sectors at offset %" PRIu64 "\n",
> + nb_sectors, sector_num);
> + }
> +
> + return ret;
> +}
> +
> +/*
> + * Prepare a buffer containing all the required data required to compute
> cluster
> + * sized deduplication hashes.
> + * If sector_num and nb_sectors are unaligned cluster wize it read the
> missing
> + * data before and after the qiov.
If sector_num or nb_sectors are not cluster-aligned, missing data
before/after the qiov will be read.
> + *
> + * @qiov: the qiov for which missing data must be read
> + * @sector_num: the first sectors that must be read into the qiov
> + * @nb_sectors: the number of sectors to read into the qiov
> + * @data: the place where the data will be concatenated and
> stored
> + * @nb_data_sectors: the resulting size of the contatenated data (in
> sectors)
> + * @ret: negative on error
> + */
> +int qcow2_dedup_read_missing_and_concatenate(BlockDriverState *bs,
> + QEMUIOVector *qiov,
> + uint64_t sector_num,
> + int nb_sectors,
> + uint8_t **data,
> + int *nb_data_sectors)
> +{
> + BDRVQcowState *s = bs->opaque;
> + int ret;
> + uint64_t cluster_beginning_sector;
> + uint64_t first_sector_after_qiov;
> + int cluster_beginning_nr;
> + int cluster_ending_nr;
> + int unaligned_ending_nr;
> + uint64_t max_cluster_ending_nr;
> +
> + /* compute how much and where to read at the beginning */
> + cluster_beginning_nr = sector_num & (s->cluster_sectors - 1);
> + cluster_beginning_sector = sector_num - cluster_beginning_nr;
> +
> + /* for the ending */
> + first_sector_after_qiov = sector_num + nb_sectors;
> + unaligned_ending_nr = first_sector_after_qiov & (s->cluster_sectors - 1);
> + cluster_ending_nr = unaligned_ending_nr ?
> + s->cluster_sectors - unaligned_ending_nr : 0;
> +
> + /* compute total size in sectors and allocate memory */
> + *nb_data_sectors = cluster_beginning_nr + nb_sectors + cluster_ending_nr;
> + *data = qemu_blockalign(bs, *nb_data_sectors * BDRV_SECTOR_SIZE);
> + memset(*data, 0, *nb_data_sectors * BDRV_SECTOR_SIZE);
Is memset necessary since we either read all data or return an error?
> + /* read beginning */
> + if (cluster_beginning_nr) {
> + ret = qcow2_dedup_read_missing_cluster_data(bs,
> + *data,
> + cluster_beginning_sector,
> + cluster_beginning_nr);
> +
> + if (ret < 0) {
> + goto fail;
> + }
> + }
> +
> + /* append qiov content */
> + qemu_iovec_to_buf(qiov, 0, *data + cluster_beginning_nr *
> BDRV_SECTOR_SIZE,
> + qiov->size);
> +
> + /* Fix cluster_ending_nr if we are at risk of reading outside the image
> + * (Cluster unaligned image size)
> + */
> + max_cluster_ending_nr = bs->total_sectors - first_sector_after_qiov;
> + cluster_ending_nr = max_cluster_ending_nr < (uint64_t) cluster_ending_nr
> ?
> + (int) max_cluster_ending_nr : cluster_ending_nr;
> +
> + /* read and add ending */
> + if (cluster_ending_nr) {
> + ret = qcow2_dedup_read_missing_cluster_data(bs,
> + *data +
> + (cluster_beginning_nr +
> + nb_sectors) *
> + BDRV_SECTOR_SIZE,
> + first_sector_after_qiov,
> + cluster_ending_nr);
> +
> + if (ret < 0) {
> + goto fail;
> + }
> + }
> +
> + return 0;
> +
> +fail:
Is it useful to leave the caller with a failed buffer still allocated?
qemu_vfree(*data);
*data = NULL;
