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

[Stefan Hajnoczi]

On Mon, Sep 12, 2011 at 3:47 PM, Devin Nakamura <address@hidden> wrote:
> +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;

Where does s->header.header_size get initialized?

> +    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 +

Please use bdrv_getlength(bs->file) instead of directly accessing
total_sectors on an unknown BlockDriverState.  We cache size in the
total_sectors field but if we want to modify the way caching works
then it's easiest if users do not reach into the field directly but
call the accessor function instead.

> +                                        drv_options->cluster_size -1);
> +    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;
> +    }

Does this leak s->l1_table?

Why check l1_table_offset just a few lines after carefully creating
it?  Is there a specific case where this could fail?

> +    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);

Did I miss where the L1 table elements are initialized to zero?  At
this point I think we're writing out undefined values from memory just
allocated with qed_alloc_table().

Stefan