Re: [Qemu-devel] [RFC 01/24] block: add block conversion api

[Stefan Hajnoczi <stefanha@gmail.com>]

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