From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:52755) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QoAmY-0007nc-0h for qemu-devel@nongnu.org; Tue, 02 Aug 2011 04:56:58 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QoAmX-0006sU-2g for qemu-devel@nongnu.org; Tue, 02 Aug 2011 04:56:57 -0400 Received: from mail-fx0-f45.google.com ([209.85.161.45]:62872) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QoAmW-0006sF-Pu for qemu-devel@nongnu.org; Tue, 02 Aug 2011 04:56:57 -0400 Received: by fxbb27 with SMTP id b27so6006991fxb.4 for ; Tue, 02 Aug 2011 01:56:55 -0700 (PDT) Date: Tue, 2 Aug 2011 09:56:49 +0100 From: Stefan Hajnoczi Message-ID: <20110802085649.GA8912@stefanha-thinkpad.localdomain> References: <1311914994-20482-1-git-send-email-devin122@gmail.com> <1311914994-20482-2-git-send-email-devin122@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1311914994-20482-2-git-send-email-devin122@gmail.com> Subject: Re: [Qemu-devel] [RFC 01/24] block: add block conversion api List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Devin Nakamura Cc: kwolf@redhat.com, qemu-devel@nongnu.org On Fri, Jul 29, 2011 at 12:49:31AM -0400, Devin Nakamura wrote: > + /** > + * Gets a mapping in the image file. > + * > + * The function starts searching for a mapping at > + * starting_guest_offset = guest_offset + contiguous_bytes > + * > + * @param bs[in] The image in which to find mapping. > + * @param guest_offset[in,out] On function entry used to calculate > + * starting search address. > + * On function exit contains the starting > + * guest offset of the mapping. > + * @param host_offset[out] The starting image file offset for the > + * mapping. > + * @param contiguous_bytes[in,out] On function entry used to calculate > + * starting search address. > + * On function exit contains the number of > + * bytes for which this mapping is valid. > + * A value of 0 means there are no more > + * mappings in the image. > + * @return Returns non-zero on error. > + */ > + int (*bdrv_get_mapping)(BlockDriverState *bs, uint64_t *guest_offset, > + uint64_t *host_offset, uint64_t *contiguous_bytes); Will the output value of guest_offset ever be smaller than the input value? It seems like this function is designed to be used as an iterator (hence the starting address calculation). Explicitly stating that it can be used as an iterator with contiguous_bytes starting at 0 would be helpful. > + * @param guest_offset The starting guest offset of the mapping > + * (in bytes). Function fails and returns -EINVAL if > + * not cluster aligned. > + * @param host_offset The starting image offset of the mapping > + * (in bytes). Function fails and returns -EINVAL if > + * not cluster aligned. > + * @param contiguous_bytes The number of bytes for which this mapping exists > + * Function fails and returns -EINVAL if not cluster > + * aligned > + * @return Returns non-zero on error > + */ > + int (*bdrv_map)(BlockDriverState *bs, uint64_t guest_offset, > + uint64_t host_offset, uint64_t contiguous_bytes); What flush semantics does this function have? Do I need to call bdrv_flush() to ensure the metadata updates are on disk? > + > + /** > + * Copies out the header of a conversion target > + * > + * Saves the current header for the image in a temporary file and overwrites > + * it with the header for the new format (at the moment the header is > + * assumed to be 1 sector) > + * > + * @param bs Usualy opened with bdrv_open_conversion_target(). > + * @return Returns non-zero on failure > + */ > + int (*bdrv_copy_header) (BlockDriverState *bs); What is the purpose of the temporary file, what filename does it need to have, etc? (I know some of the answers, but please document them.) > + > + /** > + * Asks the block driver what options should be used to create a conversion > + * target. > + * > + * @param options[out] Block conversion options > + */ > + int (*bdrv_get_conversion_options)(BlockDriverState *bs, > + BlockConversionOptions *options); > + > + > QLIST_ENTRY(BlockDriver) list; > }; > > @@ -263,4 +345,10 @@ static inline unsigned int get_physical_block_exp(BlockConf *conf) > DEFINE_PROP_UINT32("discard_granularity", _state, \ > _conf.discard_granularity, 0) > > +struct BlockConversionOptions { > + int encryption_type; > + uint64_t image_size; > + uint64_t cluster_size; These two fields can be extracted using existing block.h APIs. Does it make sense to add a bdrv_get_encryption_type() instead? That way qemu-img info can also show display the encryption type and you can drop this struct. > + uint64_t allocation_size; What is this? Stefan