On Mon, Sep 12, 2011 at 3:47 PM, Devin Nakamura <devin...@gmail.com> 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
