From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:49490) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1R6M3E-0007uV-Mq for qemu-devel@nongnu.org; Wed, 21 Sep 2011 08:37:21 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1R6M3D-00009S-HW for qemu-devel@nongnu.org; Wed, 21 Sep 2011 08:37:20 -0400 Received: from mail-yx0-f173.google.com ([209.85.213.173]:34513) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1R6M3D-00009L-EV for qemu-devel@nongnu.org; Wed, 21 Sep 2011 08:37:19 -0400 Received: by yxl11 with SMTP id 11so1376529yxl.4 for ; Wed, 21 Sep 2011 05:37:18 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <1315838844-14307-4-git-send-email-devin122@gmail.com> References: <1315838844-14307-1-git-send-email-devin122@gmail.com> <1315838844-14307-4-git-send-email-devin122@gmail.com> Date: Wed, 21 Sep 2011 13:37:18 +0100 Message-ID: From: Stefan Hajnoczi Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable 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: kwolf@redhat.com, qemu-devel@nongnu.org On Mon, Sep 12, 2011 at 3:47 PM, Devin Nakamura wrote: > +static int bdrv_qed_open_conversion_target(BlockDriverState *bs, > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0= =A0 =A0 =A0 BlockConversionOptions *drv_options, > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0= =A0 =A0 =A0 QEMUOptionParameter *usr_options, > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0= =A0 =A0 =A0 bool force) > +{ > + =A0 =A0BDRVQEDState *s =3D bs->opaque; > + =A0 =A0s->bs =3D bs; > + =A0 =A0if (drv_options->encryption_type !=3D BLOCK_CRYPT_NONE) { > + =A0 =A0 =A0 =A0error_report("Encryption not supported"); > + =A0 =A0 =A0 =A0return -ENOTSUP; > + =A0 =A0} > + =A0 =A0if(drv_options->nb_snapshots && !force) { > + =A0 =A0 =A0 =A0error_report("Snapshots are not supported"); > + =A0 =A0 =A0 =A0return -ENOTSUP; > + =A0 =A0} > + =A0 =A0s->header.magic =3D QED_MAGIC; > + =A0 =A0s->header.table_size =3D QED_DEFAULT_TABLE_SIZE; Where does s->header.header_size get initialized? > + =A0 =A0if(qed_is_cluster_size_valid(drv_options->cluster_size)) { > + =A0 =A0 =A0 =A0s->header.cluster_size =3D drv_options->cluster_size; > + =A0 =A0} else { > + =A0 =A0 =A0 =A0error_report("Invalid cluster size"); > + =A0 =A0 =A0 =A0return -EINVAL; > + =A0 =A0} > + =A0 =A0if(qed_is_image_size_valid(drv_options->image_size, s->header.cl= uster_size, > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 s->header.t= able_size)) { > + =A0 =A0 =A0 =A0s->header.image_size =3D drv_options->image_size; > + =A0 =A0} else { > + =A0 =A0 =A0 =A0error_report("Invalid image size"); > + =A0 =A0 =A0 =A0return -EINVAL; > + =A0 =A0} > + =A0 =A0s->file_size =3D 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. > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0= =A0 =A0drv_options->cluster_size -1); > + =A0 =A0s->l1_table =3D qed_alloc_table(s); > + =A0 =A0s->header.l1_table_offset =3D qed_alloc_clusters(s, s->header.ta= ble_size); > + =A0 =A0QSIMPLEQ_INIT(&s->allocating_write_reqs); > + > + > + =A0 =A0if (!qed_check_table_offset(s, s->header.l1_table_offset)) { > + =A0 =A0 =A0 =A0error_report("Invalid L1 table offset"); > + =A0 =A0 =A0 =A0return -EINVAL; > + =A0 =A0} 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? > + =A0 =A0s->table_nelems =3D (s->header.cluster_size * s->header.table_si= ze) / > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0sizeof(uint64_t); > + =A0 =A0s->l2_shift =3D ffs(s->header.cluster_size) - 1; > + =A0 =A0s->l2_mask =3D s->table_nelems - 1; > + =A0 =A0s->l1_shift =3D s->l2_shift + ffs(s->table_nelems) - 1; > + > + =A0 =A0qed_init_l2_cache(&s->l2_cache); > + > + =A0 =A0s->need_check_timer =3D qemu_new_timer_ns(vm_clock, > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0= =A0 =A0 =A0 =A0qed_need_check_timer_cb, s); > + =A0 =A0qed_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