From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:59907) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1R5wTp-0002z0-E3 for qemu-devel@nongnu.org; Tue, 20 Sep 2011 05:19:06 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1R5wTo-0002AG-1G for qemu-devel@nongnu.org; Tue, 20 Sep 2011 05:19:05 -0400 Received: from mx1.redhat.com ([209.132.183.28]:49930) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1R5wTn-0002AC-NG for qemu-devel@nongnu.org; Tue, 20 Sep 2011 05:19:04 -0400 Message-ID: <4E785B36.8090804@redhat.com> Date: Tue, 20 Sep 2011 11:21:58 +0200 From: Kevin Wolf MIME-Version: 1.0 References: <1315838844-14307-1-git-send-email-devin122@gmail.com> <1315838844-14307-4-git-send-email-devin122@gmail.com> In-Reply-To: <1315838844-14307-4-git-send-email-devin122@gmail.com> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 3/6] qed: add open_conversion_target() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Devin Nakamura Cc: qemu-devel@nongnu.org Am 12.09.2011 16:47, schrieb Devin Nakamura: > Signed-off-by: Devin Nakamura > --- > 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