Re: [Qemu-devel] [PATCH 3/6] qed: add open_conversion_target()

[Kevin Wolf <kwolf@redhat.com>]

Am 12.09.2011 16:47, schrieb Devin Nakamura:
> Signed-off-by: Devin Nakamura <devin122@gmail.com>
> ---
>  block/qed.c |   57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 57 insertions(+), 0 deletions(-)
> 
> diff --git a/block/qed.c b/block/qed.c
> index 16320f5..93827db 100644
> --- a/block/qed.c
> +++ b/block/qed.c
> @@ -1456,6 +1456,62 @@ static int bdrv_qed_get_conversion_options(BlockDriverState *bs,
>      return 0;
>  }
>  
> +static int bdrv_qed_open_conversion_target(BlockDriverState *bs,
> +                                           BlockConversionOptions *drv_options,
> +                                           QEMUOptionParameter *usr_options,
> +                                           bool force)
> +{
> +    BDRVQEDState *s = bs->opaque;
> +    s->bs = bs;
> +    if (drv_options->encryption_type != BLOCK_CRYPT_NONE) {
> +        error_report("Encryption not supported");
> +        return -ENOTSUP;
> +    }
> +    if(drv_options->nb_snapshots && !force) {
> +        error_report("Snapshots are not supported");
> +        return -ENOTSUP;
> +    }
> +    s->header.magic = QED_MAGIC;
> +    s->header.table_size = QED_DEFAULT_TABLE_SIZE;
> +    if(qed_is_cluster_size_valid(drv_options->cluster_size)) {
> +        s->header.cluster_size = drv_options->cluster_size;
> +    } else {
> +        error_report("Invalid cluster size");
> +        return -EINVAL;
> +    }
> +    if(qed_is_image_size_valid(drv_options->image_size, s->header.cluster_size,
> +                               s->header.table_size)) {
> +        s->header.image_size = drv_options->image_size;
> +    } else {
> +        error_report("Invalid image size");
> +        return -EINVAL;
> +    }
> +    s->file_size = qed_Start_of_cluster(s, bs->file->total_sectors +
> +                                        drv_options->cluster_size -1);

This doesn't look quite right. With the capital S in
qemu_Start_of_cluster it actually shouldn't even compile.

drv->options->cluster_size is in bytes, but bs->file->total_sectors is
in sectors. I don't think the sum of them makes a lot of sense.

> +    s->l1_table = qed_alloc_table(s);
> +    s->header.l1_table_offset = qed_alloc_clusters(s, s->header.table_size);
> +    QSIMPLEQ_INIT(&s->allocating_write_reqs);
> +
> +
> +    if (!qed_check_table_offset(s, s->header.l1_table_offset)) {
> +        error_report("Invalid L1 table offset");
> +        return -EINVAL;

This leaks s->l1_table.

> +    }
> +
> +    s->table_nelems = (s->header.cluster_size * s->header.table_size) /
> +                      sizeof(uint64_t);
> +    s->l2_shift = ffs(s->header.cluster_size) - 1;
> +    s->l2_mask = s->table_nelems - 1;
> +    s->l1_shift = s->l2_shift + ffs(s->table_nelems) - 1;
> +
> +    qed_init_l2_cache(&s->l2_cache);
> +
> +    s->need_check_timer = qemu_new_timer_ns(vm_clock,
> +                                            qed_need_check_timer_cb, s);
> +    qed_write_l1_table_sync(s, 0, s->table_nelems);
> +    return 0;
> +}
> +
>  static QEMUOptionParameter qed_create_options[] = {
>      {
>          .name = BLOCK_OPT_SIZE,
> @@ -1503,6 +1559,7 @@ static BlockDriver bdrv_qed = {
>      .bdrv_change_backing_file = bdrv_qed_change_backing_file,
>      .bdrv_check               = bdrv_qed_check,
>      .bdrv_get_conversion_options = bdrv_qed_get_conversion_options,
> +    .bdrv_open_conversion_target = bdrv_qed_open_conversion_target,
>  };
>  
>  static void bdrv_qed_init(void)

Another question that came to my mind while reviewing this - although
mostly unrelated - is where we handle backing files. Is that completely
contained in the generic conversion code?

Kevin