Re: [Qemu-devel] [patch 2/5][v2] Divide get_cluster_offset()

[Kevin Wolf <kwolf@suse.de>]

Laurent Vivier schrieb:
> -    if (allocate == 1) {
> -        /* allocate a new cluster */
> -        cluster_offset = alloc_clusters(bs, s->cluster_size);
> -
> -        /* we must initialize the cluster content which won't be
> -           written */
> -        if ((n_end - n_start) < s->cluster_sectors) {
> -            uint64_t start_sect;
> -
> -            start_sect = (offset & ~(s->cluster_size - 1)) >> 9;
> -            ret = copy_sectors(bs, start_sect,
> -                               cluster_offset, 0, n_start);
> -            if (ret < 0)
> -                return 0;
> -            ret = copy_sectors(bs, start_sect,
> -                               cluster_offset, n_end, s->cluster_sectors);
> -            if (ret < 0)
> -                return 0;
> -        }
> -        tmp = cpu_to_be64(cluster_offset | QCOW_OFLAG_COPIED);
> -    } else {
> +
> +    if (compressed_size) {
>          int nb_csectors;
>          cluster_offset = alloc_bytes(bs, compressed_size);
>          nb_csectors = ((cluster_offset + compressed_size - 1) >> 9) -
>              (cluster_offset >> 9);
>          cluster_offset |= QCOW_OFLAG_COMPRESSED |
>              ((uint64_t)nb_csectors << s->csize_shift);
> -        /* compressed clusters never have the copied flag */
> -        tmp = cpu_to_be64(cluster_offset);
> +
> +        /* update L2 table
> +         * compressed clusters never have the copied flag
> +         */
> +
> +        l2_table[l2_index] = cpu_to_be64(cluster_offset);
> +        if (bdrv_pwrite(s->hd,
> +                        l2_offset + l2_index * sizeof(uint64_t),
> +                        l2_table + l2_index,
> +                        sizeof(uint64_t)) != sizeof(uint64_t))
> +            return 0;
> +
> +        return cluster_offset;
>      }
> +
> +    /* allocate a new cluster */
> +
> +    cluster_offset = alloc_clusters(bs, s->cluster_size);
> +
> +    /* we must initialize the cluster content which won't be
> +       written */
> +
> +    if ((n_end - n_start) < s->cluster_sectors) {
> +        uint64_t start_sect;
> +
> +        start_sect = (offset & ~(s->cluster_size - 1)) >> 9;
> +        ret = copy_sectors(bs, start_sect, cluster_offset, 0, n_start);
> +        if (ret < 0)
> +            return 0;
> +        ret = copy_sectors(bs, start_sect,
> +                           cluster_offset, n_end, s->cluster_sectors);
> +        if (ret < 0)
> +            return 0;
> +    }
> +
>      /* update L2 table */
> -    l2_table[l2_index] = tmp;
> +
> +    l2_table[l2_index] = cpu_to_be64(cluster_offset | QCOW_OFLAG_COPIED);
>      if (bdrv_pwrite(s->hd,
> -                    l2_offset + l2_index * sizeof(tmp), &tmp, sizeof(tmp)) != sizeof(tmp))
> +                    l2_offset + l2_index * sizeof(uint64_t),
> +                    l2_table + l2_index,
> +                    sizeof(uint64_t)) != sizeof(uint64_t))
>          return 0;
> +
>      return cluster_offset;
>  }


Why are you moving that code around? I don't think it's needed for the
get_cluster_offset/allocate_cluster_offset split. You're just
duplicating the write and return.

Otherwise, the patch looks fine and makes the whole thing much cleaner.

Kevin