Re: [Qemu-devel] [PATCH v3 1/6] block: add basic conversion api

[Kevin Wolf]

Am 13.07.2011 14:57, schrieb Devin Nakamura:
> add functions to block driver interface to support inplace image conversion
> 
> Signed-off-by: Devin Nakamura <address@hidden>
> ---
>  block_int.h |   70 
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 70 insertions(+), 0 deletions(-)
> 
> diff --git a/block_int.h b/block_int.h
> index 1e265d2..050ecf3 100644
> --- a/block_int.h
> +++ b/block_int.h
> @@ -137,6 +137,76 @@ struct BlockDriver {
>       */
>      int (*bdrv_has_zero_init)(BlockDriverState *bs);
>  
> +    /* In-place image conversion */
> +
> +    /**
> +     *

I think this description is a bit short. :-)

> +     * @param bs      Basic Initialization done by 
> bdrv_open_conversion_target()
> +     *                Still need to set

Need to set what?

> +     * @param options Creation options.
> +     * @return        Returns non-zero on failure.
> +     */
> +    int (*bdrv_open_conversion_target)(BlockDriverState *bs,
> +        QEMUOptionParameter *options);
> +
> +    /**
> +     * Gets a mapping in the image file.
> +     *
> +     * The function starts searching for a mapping at
> +     * starting_guest_offset = guest_offset + contiguous_bytes

Add a blank line here

> +     * @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 staring
> +     *                                 guest offset of the mapping.

s/staring/starting/

> +     * @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);

I think it would be less surprising if the function worked like
bdrv_is_allocated(). That is, it doesn't search for mapped clusters, but
always returns information for exactly the given offset. It would return
if the range starting at the given offset is used or free.

If the caller wants to find the next existing mapping, he can just take
contiguous_bytes of the free region and add it to his current offset.

> +
> +    /**
> +     * Sets a mapping in the image file.
> +     *
> +     * @param bs               Usualy opened with bdrv_open_conversion_target

Is it required that the image was opened with
bdrv_open_conversion_target? If yes, say so in the comment. If no, no
reason to have it here.

> +     * @param guest_offset     The starting guest offset of the mapping
> +     *                         (in bytes)
> +     * @param host_offset      The starting image offset of the mapping
> +     *                         (in bytes)
> +     * @param contiguous_bytes The number of bytes for which this mapping 
> exists
> +     * @return                 Returns non-zero on error
> +     */
> +    int (*bdrv_map)(BlockDriverState *bs, uint64_t guest_offset,
> +        uint64_t host_offset, uint64_t contiguous_bytes);

What happens if one of the offsets or contiguous_bytes is not aligned to
the cluster size?

> +
> +    /**
> +     * 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);
> +
> +    /**
> +     * Asks the block driver what options should be used to create a 
> conversion
> +     * target.
> +     * @param options[out]
> +     */
> +    int (*bdrv_get_conversion_options)(BlockDriverState *bs,
> +        QEMUOptionParameter **options);

No description for options? With this description, I'm not sure how this
function is meant to work and which QEMUOptionParameter list it uses.
The one of the source format or the one of the destination format?

Kevin