Patchwork [v3,1/6] block: add basic conversion api

login
register
mail settings
Submitter Devin Nakamura
Date July 13, 2011, 12:57 p.m.
Message ID <1310561864-18964-1-git-send-email-devin122@gmail.com>
Download mbox | patch
Permalink /patch/104504/
State New
Headers show

Comments

Devin Nakamura - July 13, 2011, 12:57 p.m.
add functions to block driver interface to support inplace image conversion

Signed-off-by: Devin Nakamura <devin122@gmail.com>
---
 block_int.h |   70 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 70 insertions(+), 0 deletions(-)
Kevin Wolf - July 14, 2011, 2:19 p.m.
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 <devin122@gmail.com>
> ---
>  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

Patch

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 */
+
+    /**
+     *
+     * @param bs      Basic Initialization done by bdrv_open_conversion_target()
+     *                Still need to set
+     * @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
+     * @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.
+     * @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);
+
+    /**
+     * Sets a mapping in the image file.
+     *
+     * @param bs               Usualy opened with bdrv_open_conversion_target
+     * @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);
+
+    /**
+     * 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);
+
+
     QLIST_ENTRY(BlockDriver) list;
 };